Migrate all assertions to Shouldly syntax; use visual regression checks #624
Conversation
📝 WalkthroughWalkthroughTests migrated from explicit hash and Record.Exception assertions to Shouldly-style and approval-based checks; new image/SVG/ASCII approval helpers and approved assets added; two RussiaPaymentOrder approved payload files added; CI workflow and demo project targets adjusted; .gitignore updated to ignore received approval files. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Renderer
participant Output
participant Approvals
Note over Test,Renderer: Generate QR output in test
Test->>Renderer: Generate(payload/options)
Renderer-->>Output: Bitmap / SVG / Text
Test->>Approvals: ShouldMatchApproved(Output)
Approvals->>Approvals: normalize/encode (GIF/monochrome/string)
Approvals->>Approvals: resolve approved/received paths via stack trace
Approvals-->>Test: pass or throw ShouldAssertException (with approve instructions)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)QRCoderTests/SvgQRCodeRendererTests.cs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs (1)
34-34: Remove.NoDiff()to surface diff details on failure
Baseline files for bothkoi8-randwindows-1251encodings are in place and string-based approval is suitable for text payloads. Omitting.NoDiff()will display diffs on assertion failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)QRCoderTests/ArtQRCodeRendererTests.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs(21 hunks)QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs(10 hunks)QRCoderTests/PayloadGeneratorTests/MoneroTransactionTests.cs(2 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs(7 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_koi8.approved.txt(1 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_win1251.approved.txt(1 hunks)QRCoderTests/PayloadGeneratorTests/ShadowSocksConfigTests.cs(2 hunks)QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (1)
QRCoder/PayloadGenerator/BezahlCode.cs (4)
BezahlCodeException(565-592)BezahlCodeException(570-572)BezahlCodeException(578-581)BezahlCodeException(588-591)
QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs (1)
QRCoder/PayloadGenerator/SwissQrCode.cs (25)
SwissQrCodeReferenceException(233-260)SwissQrCodeReferenceException(238-240)SwissQrCodeReferenceException(246-249)SwissQrCodeReferenceException(256-259)SwissQrCodeAdditionalInformationException(121-148)SwissQrCodeAdditionalInformationException(126-128)SwissQrCodeAdditionalInformationException(134-137)SwissQrCodeAdditionalInformationException(144-147)PayloadGenerator(8-654)SwissQrCode(13-653)SwissQrCode(50-79)Iban(266-346)Iban(276-286)SwissQrCodeIbanException(318-345)SwissQrCodeIbanException(323-325)SwissQrCodeIbanException(331-334)SwissQrCodeIbanException(341-344)Contact(351-542)Contact(389-390)Contact(395-396)Contact(399-470)SwissQrCodeContactException(514-541)SwissQrCodeContactException(519-521)SwissQrCodeContactException(527-530)SwissQrCodeContactException(537-540)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
.gitignore (1)
224-224: LGTM!The addition of
QRCoderTests/*.received.*correctly ignores temporary approval test artifacts that should not be committed to the repository.QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_win1251.approved.txt (1)
1-1: LGTM!This approved payload file correctly establishes the baseline for Win1251-encoded Russian payment order tests. The payload structure with pipe-delimited fields (ST00011|Name|PersonalAcc|BankName|BIC|CorrespAcc) is consistent with the payment order format.
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_koi8.approved.txt (1)
1-1: LGTM!This approved payload file correctly establishes the baseline for KOI8-encoded Russian payment order tests. The format version ST00013 and the use of angle brackets (< >) instead of guillemets properly reflect the KOI8 encoding differences.
QRCoderTests/PayloadGeneratorTests/MoneroTransactionTests.cs (2)
70-71: LGTM!The migration from xUnit assertions to Shouldly assertions is correct and consistent with the PR objective. The test logic and exception validation remain unchanged.
83-84: LGTM!The assertion migration is consistent with the pattern used throughout this file and maintains the same test validation logic.
QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs (3)
22-23: LGTM!All exception assertions have been consistently migrated to Shouldly syntax. The pattern of using
ShouldNotBeNull()followed byShouldBeOfType<T>()correctly validates both exception presence and type.Also applies to: 35-36, 50-51, 65-66, 80-81, 95-96, 108-109, 175-176, 188-189, 201-202, 253-254, 271-272, 289-290, 307-308, 325-326, 343-344, 360-361, 378-379, 396-397, 414-415, 432-433, 555-556, 573-574, 592-593, 611-612
626-626: LGTM!The use of
ShouldBeNull()correctly validates that no exception is thrown for valid country codes (DE, de, XK), which is the expected behavior for these test cases.Also applies to: 631-631, 637-637
644-645: LGTM!The final exception validation for the invalid country code (ZZ) correctly uses Shouldly assertions to verify both exception presence and type.
QRCoderTests/ArtQRCodeRendererTests.cs (1)
72-73: LGTM!The exception assertions have been correctly migrated to Shouldly syntax, maintaining the same validation logic while improving test readability.
QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs (1)
142-143: LGTM!All Girocode exception assertions have been consistently migrated to Shouldly syntax. The test validation logic remains unchanged, and the migration is complete and correct.
Also applies to: 163-164, 184-185, 205-206, 225-226, 247-248, 268-269, 288-289, 308-309, 328-329
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (1)
334-335: LGTM!All BezahlCode exception assertions have been successfully migrated to Shouldly syntax. The changes are consistent with the PR objective and maintain the same test validation logic across all test methods.
Also applies to: 349-350, 365-366, 387-388, 403-404, 426-427, 443-444, 461-462, 478-479, 494-495, 511-512, 527-528, 544-545, 566-567, 588-589, 609-610, 624-625, 641-642, 662-663, 685-686, 708-709
QRCoderTests/PayloadGeneratorTests/ShadowSocksConfigTests.cs (1)
49-50: LGTM! Consistent Shouldly migration.The exception assertions have been correctly migrated to Shouldly syntax. The pattern of checking both non-null and type is appropriate for exception verification tests.
Also applies to: 65-66
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs (1)
75-76: LGTM! Exception assertions correctly migrated to Shouldly.All exception assertions have been consistently migrated to Shouldly syntax:
ShouldNotBeNull()andShouldBeOfType<T>()for positive exception casesShouldBeNull()for the negative test case (line 118)The migration maintains functional equivalence with the previous assertion style.
Also applies to: 93-94, 118-118, 131-132, 146-147
Shane32
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (2)
86-94: Consider cross-platform copy command.The error message provides a Windows-specific
copy /Ycommand. This won't work on Linux/macOS CI runners or developer machines.Consider detecting the platform and providing the appropriate command:
- throw new ShouldAssertException($""" - Approval file {approvedFile} - does not exist - - To approve the received file, run: - - copy /Y "{receivedFile}" "{approvedFile}" - - """); + var copyCommand = OperatingSystem.IsWindows() + ? $"""copy /Y "{receivedFile}" "{approvedFile}" """ + : $"""cp "{receivedFile}" "{approvedFile}" """; + throw new ShouldAssertException($""" + Approval file {approvedFile} + does not exist + + To approve the received file, run: + + {copyCommand} + + """);
104-116: Consider cross-platform copy command in mismatch message.Similar to the missing file case, the copy command here is also Windows-specific.
Apply the same platform detection for consistency:
- var copyCommand = $""" - - - To approve the received file, run: - - copy /Y "{receivedFile}" "{approvedFile}" - - """; + var copyCmd = OperatingSystem.IsWindows() + ? $"""copy /Y "{receivedFile}" "{approvedFile}" """ + : $"""cp "{receivedFile}" "{approvedFile}" """; + var copyCommand = $""" + + + To approve the received file, run: + + {copyCmd} + + """;QRCoderTests/PostscriptQRCodeRendererTests.cs (1)
61-68: Regex pattern could be more precise.The pattern
@"%%CreationDate:.*\r?\n?"works but makes the line ending entirely optional with\r?\n?. This could theoretically match content without consuming the line ending, though it's unlikely to cause issues in practice with PostScript files.Consider making the pattern more explicit:
- string pattern = @"%%CreationDate:.*\r?\n?"; + string pattern = @"%%CreationDate:.*(?:\r?\n|$)";This explicitly matches either a line ending OR end-of-string, making the intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (39)
QRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_with_background.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_with_custom_finder.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_without_quietzone.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_render_artqrcode_from_helper.approved.gifis excluded by!**/*.gifQRCoderTests/Base64QRCodeRendererTests.can_render_base64_qrcode_jpeg.approved.gifis excluded by!**/*.gifQRCoderTests/BitmapByteQRCodeRendererTests.can_render_bitmapbyte_qrcode.approved.gifis excluded by!**/*.gifQRCoderTests/BitmapByteQRCodeRendererTests.can_render_bitmapbyte_qrcode_color_bytearray.approved.gifis excluded by!**/*.gifQRCoderTests/BitmapByteQRCodeRendererTests.can_render_bitmapbyte_qrcode_drawing_color.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_blackwhite.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_color.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_color_with_alpha.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_color_without_quietzones.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_drawing_color.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_from_helper.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_from_helper_2.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_standard_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_standard_graphic_hex.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_standard_graphic_without_quietzones.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_logo_and_with_custom_border.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_logo_and_with_standard_border.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_logo_and_with_transparent_border.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_non_transparent_logo_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_transparent_logo_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_render_qrcode_from_helper.approved.gifis excluded by!**/*.gifQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_from_helper.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_simple.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_viewbox_mode.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_viewbox_mode_viewboxattr.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_background.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bytearray.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_svg_logo_embedded.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_svg_logo_image_tag.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_without_quietzones_hex.approved.svgis excluded by!**/*.svgQRCoderTests/XamlQRCodeRendererTests.can_create_xaml_qrcode_standard_graphic.approved.gifis excluded by!**/*.gif
📒 Files selected for processing (22)
.github/workflows/wf-build-test.yml(3 hunks)QRCoderDemo/QRCoderDemo.csproj(1 hunks)QRCoderTests/ArtQRCodeRendererTests.cs(6 hunks)QRCoderTests/AsciiQRCodeRendererTests.cs(2 hunks)QRCoderTests/Base64QRCodeRendererTests.cs(1 hunks)QRCoderTests/BitmapByteQRCodeRendererTests.cs(3 hunks)QRCoderTests/Helpers/HelperFunctions.cs(3 hunks)QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs(7 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_koi8.approved.txt(1 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_win1251.approved.txt(1 hunks)QRCoderTests/PngByteQRCodeRendererTests.cs(6 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.cs(5 hunks)QRCoderTests/QRCodeRendererTests.cs(9 hunks)QRCoderTests/QRCoderTests.csproj(2 hunks)QRCoderTests/SvgQRCodeRendererTests.cs(13 hunks)QRCoderTests/XamlQRCodeRendererTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- QRCoderTests/ArtQRCodeRendererTests.cs
- QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs
- QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_koi8.approved.txt
🧰 Additional context used
🧬 Code graph analysis (10)
QRCoderTests/AsciiQRCodeRendererTests.cs (1)
QRCoder/ASCIIQRCode.cs (8)
AsciiQRCode(11-127)AsciiQRCode(16-16)AsciiQRCode(18-18)GetGraphic(30-35)GetGraphicSmall(80-126)AsciiQRCodeHelper(133-150)GetQRCode(135-141)GetQRCode(143-149)
QRCoderTests/Base64QRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApprovedImage(20-25)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
QRCoderTests/Helpers/HelperFunctions.cs (2)
Bitmap(30-39)Bitmap(93-99)
QRCoderTests/XamlQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-33)
QRCoderTests/QRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-33)
QRCoderTests/SvgQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-33)
QRCoderTests/PngByteQRCodeRendererTests.cs (2)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApprovedImage(20-25)QRCoder/PngByteQRCode.cs (9)
PngByteQRCode(14-355)PngByteQRCode(20-20)PngByteQRCode(26-28)GetGraphic(37-49)GetGraphic(60-61)GetGraphic(72-85)PngByteQRCodeHelper(360-401)GetQRCode(376-382)GetQRCode(394-400)
QRCoderTests/PostscriptQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-33)
QRCoderTests/BitmapByteQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApprovedImage(20-25)
QRCoderTests/Helpers/HelperFunctions.cs (1)
QRCoder/Extensions/StreamExtensions.cs (1)
CopyTo(9-17)
🔇 Additional comments (24)
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_win1251.approved.txt (1)
1-1: Win1251 approval payload reads correctly.The new approval snapshot preserves the expected field ordering and delimiter layout for the Win1251 scenario. Looks good.
.github/workflows/wf-build-test.yml (1)
35-38: Artifact retention tightened appropriately.
Setting the workflow artifact to a one-day retention keeps fresh approval payloads available without accumulating stale snapshots.QRCoderTests/Base64QRCodeRendererTests.cs (1)
56-57: LGTM! Cleaner approval-based testing.The migration from manual bitmap creation and Base64 encoding to direct approval-based image comparison simplifies the test and aligns with the PR's goal of standardizing on Shouldly assertions.
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt (1)
1-122: LGTM! Well-formed PostScript EPS approval baseline.The PostScript file is properly structured with correct EPS headers, constant definitions, helper procedures, and QR code rendering logic. This serves as an approved baseline for color rendering tests.
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (2)
26-33: LGTM! Smart use of GIF encoding for platform consistency.Using KGySoft.Drawing to encode bitmaps as GIF before comparison is a clever solution to avoid platform-specific compression differences that could cause spurious test failures.
38-60: LGTM! Robust stack trace parsing.The stack frame analysis correctly skips helper methods marked with
[ShouldlyMethods]to identify the actual test method. This ensures approval files are named correctly based on the test method rather than the extension method.QRCoderTests/SvgQRCodeRendererTests.cs (1)
21-21: LGTM! Consistent migration to approval-based testing.All SVG rendering tests have been successfully migrated from hash-based assertions to approval-based snapshot testing using
ShouldMatchApproved. The consistent use ofNoDiff().WithFileExtension("svg")ensures proper SVG file comparison.Also applies to: 31-31, 41-41, 51-51, 61-61, 71-71, 88-88, 104-104, 120-120, 137-137, 153-153, 168-168, 184-184
QRCoderTests/BitmapByteQRCodeRendererTests.cs (1)
15-15: LGTM! Clean migration to approval-based image testing.All bitmap byte rendering tests now use
ShouldMatchApprovedImage()for validation, which is more maintainable than computing and comparing hashes.Also applies to: 25-25, 34-34
QRCoderTests/XamlQRCodeRendererTests.cs (1)
20-20: LGTM! XAML renderer aligned with approval testing pattern.The XAML QR code test now uses the same approval-based approach as other renderers, ensuring consistency across the test suite.
QRCoderTests/QRCodeRendererTests.cs (1)
18-18: LGTM! Comprehensive migration of QR code renderer tests.All QR code rendering tests have been successfully migrated to approval-based testing. The migration covers a wide range of scenarios including standard graphics, color variations, quiet zones, logos with different transparency settings, and custom borders. This ensures consistent validation across all rendering configurations.
Also applies to: 27-27, 37-37, 50-50, 61-61, 74-74, 87-87, 100-100, 116-116
QRCoderTests/QRCoderTests.csproj (1)
21-22: KGySoft.Drawing packages are up-to-date and have no known vulnerabilities
Both KGySoft.Drawing (9.2.0) and KGySoft.Drawing.Core (9.1.1) match the latest releases and have no active security advisories.QRCoderTests/PngByteQRCodeRendererTests.cs (1)
1-27: LGTM! Excellent migration to approval-based image testing.The file successfully migrates from platform-specific hash-based verification to
ShouldMatchApprovedImage(), which correctly handles visual equivalence across different Deflate implementations. This approach is more maintainable and aligns with the PR's objective.QRCoderTests/PostscriptQRCodeRendererTests.cs (6)
11-19: LGTM! Clean migration to approval-based PostScript verification.The test correctly uses
RemoveCreationDate(ps).ShouldMatchApproved(x => x.NoDiff())to strip dynamic timestamps before exact text comparison. This pattern is consistently applied across all PostScript rendering tests.
12-19: LGTM!The migration to approval testing is appropriate for verifying PostScript output, and the use of
RemoveCreationDateensures deterministic comparisons by stripping timestamps.
22-29: LGTM!The EPS format test correctly applies the same approval pattern. The migration is consistent with the other test methods.
32-39: LGTM!The size-based rendering test correctly applies the approval pattern, maintaining consistency across the test suite.
42-49: LGTM!The test for rendering without quiet zones correctly applies the approval pattern, maintaining consistency.
52-59: LGTM!The color customization test correctly applies the approval pattern, completing the consistent migration across all test methods.
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt (1)
1-122: LGTM! Well-formed PostScript test resource.This approved test file is properly structured as a PostScript document with correct syntax, helper procedures, and a 33x33 QR matrix pattern. It aligns with the approval-based testing strategy for PostScript rendering.
QRCoderTests/AsciiQRCodeRendererTests.cs (1)
9-50: LGTM! Excellent use of raw string literals and platform-agnostic assertions.The test now uses triple-quoted raw string literals for
targetCode, which greatly improves readability. The assertion correctly usesStringCompareShould.IgnoreLineEndingsto handle platform differences in line endings.QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt (1)
1-122: LGTM! Valid EPS test resource.This approved test file correctly uses the EPS format (EPSF-3.0) with appropriate constants (sz=165, sc=5) and follows the standard PostScript structure. It properly supports approval-based testing for EPS rendering.
QRCoderTests/Helpers/HelperFunctions.cs (3)
93-99: LGTM! Simplified resource handling with single using pattern.The refactored
GetIconBitmapcorrectly uses a singleusing varstatement with null-coalescing throw, improving readability while maintaining proper resource disposal.
104-112: LGTM! Streamlined byte array conversion.The refactored
GetIconBytescorrectly uses twousing varstatements andCopyTofor efficient stream copying, properly returning the byte array.
117-124: LGTM! Simplified SVG resource loading.The refactored
GetIconSvgcorrectly uses nestedusing varstatements with aStreamReaderto read and return the SVG content.
… readability and consistency
…mproved accuracy in tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
37-124: Consider cross-platform copy commands.Lines 94 and 112 use Windows-specific
copy /Ycommands. This will not work on Unix-based systems (Linux, macOS) wherecpis the standard command.Consider detecting the OS and providing the appropriate command:
+ var copyCommand = System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Windows) + ? $"copy /Y \"{receivedFile}\" \"{approvedFile}\"" + : $"cp \"{receivedFile}\" \"{approvedFile}\""; + throw new ShouldAssertException($""" Approval file {approvedFile} does not exist To approve the received file, run: - copy /Y "{receivedFile}" "{approvedFile}" + {copyCommand} """);Similarly for lines 107-113.
Alternatively, you could provide both commands or just use the cross-platform path syntax and let users figure out their shell's copy command.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (39)
QRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_with_background.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_with_custom_finder.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_without_quietzone.approved.gifis excluded by!**/*.gifQRCoderTests/ArtQRCodeRendererTests.can_render_artqrcode_from_helper.approved.gifis excluded by!**/*.gifQRCoderTests/Base64QRCodeRendererTests.can_render_base64_qrcode_jpeg.approved.gifis excluded by!**/*.gifQRCoderTests/BitmapByteQRCodeRendererTests.can_render_bitmapbyte_qrcode.approved.gifis excluded by!**/*.gifQRCoderTests/BitmapByteQRCodeRendererTests.can_render_bitmapbyte_qrcode_color_bytearray.approved.gifis excluded by!**/*.gifQRCoderTests/BitmapByteQRCodeRendererTests.can_render_bitmapbyte_qrcode_drawing_color.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_blackwhite.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_color.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_color_with_alpha.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_color_without_quietzones.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_drawing_color.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_from_helper.approved.gifis excluded by!**/*.gifQRCoderTests/PngByteQRCodeRendererTests.can_render_pngbyte_qrcode_from_helper_2.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_standard_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_standard_graphic_hex.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_standard_graphic_without_quietzones.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_logo_and_with_custom_border.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_logo_and_with_standard_border.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_logo_and_with_transparent_border.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_non_transparent_logo_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_create_qrcode_with_transparent_logo_graphic.approved.gifis excluded by!**/*.gifQRCoderTests/QRCodeRendererTests.can_render_qrcode_from_helper.approved.gifis excluded by!**/*.gifQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_from_helper.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_simple.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_viewbox_mode.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_viewbox_mode_viewboxattr.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_background.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bytearray.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_svg_logo_embedded.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_svg_logo_image_tag.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_without_quietzones_hex.approved.svgis excluded by!**/*.svgQRCoderTests/XamlQRCodeRendererTests.can_create_xaml_qrcode_standard_graphic.approved.gifis excluded by!**/*.gif
📒 Files selected for processing (27)
.github/workflows/wf-build-test.yml(3 hunks)QRCoderDemo/QRCoderDemo.csproj(1 hunks)QRCoderTests/ArtQRCodeRendererTests.cs(6 hunks)QRCoderTests/AsciiQRCodeRendererTests.cs(2 hunks)QRCoderTests/Base64QRCodeRendererTests.cs(1 hunks)QRCoderTests/BitmapByteQRCodeRendererTests.cs(3 hunks)QRCoderTests/Helpers/HelperFunctions.cs(3 hunks)QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs(21 hunks)QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs(10 hunks)QRCoderTests/PayloadGeneratorTests/MoneroTransactionTests.cs(2 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs(7 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_koi8.approved.txt(1 hunks)QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_win1251.approved.txt(1 hunks)QRCoderTests/PayloadGeneratorTests/ShadowSocksConfigTests.cs(2 hunks)QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs(26 hunks)QRCoderTests/PngByteQRCodeRendererTests.cs(6 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt(1 hunks)QRCoderTests/PostscriptQRCodeRendererTests.cs(5 hunks)QRCoderTests/QRCodeRendererTests.cs(9 hunks)QRCoderTests/QRCoderTests.csproj(2 hunks)QRCoderTests/SvgQRCodeRendererTests.cs(13 hunks)QRCoderTests/XamlQRCodeRendererTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- QRCoderTests/PayloadGeneratorTests/MoneroTransactionTests.cs
- QRCoderTests/PayloadGeneratorTests/ShadowSocksConfigTests.cs
- QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_koi8.approved.txt
- QRCoderTests/ArtQRCodeRendererTests.cs
🧰 Additional context used
🧬 Code graph analysis (10)
QRCoderTests/QRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-35)
QRCoderTests/Helpers/HelperFunctions.cs (1)
QRCoder/Extensions/StreamExtensions.cs (1)
CopyTo(9-17)
QRCoderTests/BitmapByteQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApprovedImage(20-25)
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (1)
QRCoder/PayloadGenerator/BezahlCode.cs (10)
BezahlCodeException(565-592)BezahlCodeException(570-572)BezahlCodeException(578-581)BezahlCodeException(588-591)PayloadGenerator(6-594)BezahlCode(11-593)BezahlCode(35-37)BezahlCode(56-58)BezahlCode(80-82)BezahlCode(110-248)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
QRCoderTests/Helpers/HelperFunctions.cs (2)
Bitmap(30-39)Bitmap(93-99)
QRCoderTests/SvgQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-35)
QRCoderTests/XamlQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApproved(26-35)
QRCoderTests/PngByteQRCodeRendererTests.cs (2)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (1)
ShouldMatchApprovedImage(20-25)QRCoder/PngByteQRCode.cs (9)
PngByteQRCode(14-355)PngByteQRCode(20-20)PngByteQRCode(26-28)GetGraphic(37-49)GetGraphic(60-61)GetGraphic(72-85)PngByteQRCodeHelper(360-401)GetQRCode(376-382)GetQRCode(394-400)
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs (1)
QRCoder/PayloadGenerator/RussiaPaymentOrder.cs (1)
ToBytes(85-118)
QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs (1)
QRCoder/PayloadGenerator/SwissQrCode.cs (33)
SwissQrCodeReferenceException(233-260)SwissQrCodeReferenceException(238-240)SwissQrCodeReferenceException(246-249)SwissQrCodeReferenceException(256-259)PayloadGenerator(8-654)SwissQrCode(13-653)SwissQrCode(50-79)Reference(154-261)Reference(165-184)SwissQrCodeAdditionalInformationException(121-148)SwissQrCodeAdditionalInformationException(126-128)SwissQrCodeAdditionalInformationException(134-137)SwissQrCodeAdditionalInformationException(144-147)AdditionalInformation(84-149)AdditionalInformation(93-100)Iban(266-346)Iban(276-286)SwissQrCodeIbanException(318-345)SwissQrCodeIbanException(323-325)SwissQrCodeIbanException(331-334)SwissQrCodeIbanException(341-344)Contact(351-542)Contact(389-390)Contact(395-396)Contact(399-470)SwissQrCodeContactException(514-541)SwissQrCodeContactException(519-521)SwissQrCodeContactException(527-530)SwissQrCodeContactException(537-540)SwissQrCodeException(625-652)SwissQrCodeException(630-632)SwissQrCodeException(638-641)SwissQrCodeException(648-651)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: additional-tests
🔇 Additional comments (67)
QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs (2)
139-143: LGTM! Clean migration to Shouldly syntax.The conversion from
Record.ExceptiontoShould.Throw<GirocodeException>is correct and idiomatic. The exception message assertion is properly preserved.
158-162: Consistent migration pattern across all exception tests.All remaining exception tests correctly apply the same Shouldly pattern. Each test properly captures the exception type and preserves message assertions.
Also applies to: 177-181, 196-200, 214-218, 234-238, 253-257, 271-275, 289-293, 307-311
QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs (3)
20-23: LGTM! Proper Shouldly migration for exception testing.The migration from
Record.ExceptiontoShould.Throw<SwissQrCodeReferenceException>is correct. Exception message validation is maintained.
575-592: Excellent use of Should.NotThrow for positive cases.The migration correctly uses
Should.NotThrowfor valid country codes (DE, de, XK) andShould.Throwfor the invalid case (ZZ). This is more expressive than the previousRecord.Exceptionpattern.
31-34: Consistent exception testing migration across all validators.All remaining exception tests properly migrate to
Should.Throw<T>with the appropriate exception types:
- SwissQrCodeReferenceException for reference validation
- SwissQrCodeAdditionalInformationException for additional info validation
- SwissQrCodeIbanException for IBAN validation
- SwissQrCodeContactException for contact validation
- SwissQrCodeException for top-level SwissQrCode validation
Exception messages are preserved in all cases.
Also applies to: 44-47, 57-60, 70-73, 83-86, 94-97, 159-162, 170-173, 181-184, 231-234, 247-250, 263-266, 279-282, 295-298, 311-314, 326-329, 342-345, 358-361, 374-377, 390-393, 511-514, 527-530, 544-547, 561-564
QRCoderTests/PayloadGeneratorTests/BezahlCodeTests.cs (21)
332-334: LGTM!The migration to Shouldly's
Should.Throw<T>pattern is correct and eliminates redundant null/type checks.
345-347: LGTM!Consistent Shouldly migration for exception assertion.
359-361: LGTM!Consistent Shouldly migration for exception assertion.
379-381: LGTM!Consistent Shouldly migration for exception assertion.
393-395: LGTM!Consistent Shouldly migration for exception assertion.
414-416: LGTM!Consistent Shouldly migration for exception assertion.
429-431: LGTM!Consistent Shouldly migration for exception assertion.
445-447: LGTM!Consistent Shouldly migration for exception assertion.
460-462: LGTM!Consistent Shouldly migration for exception assertion.
474-476: LGTM!Consistent Shouldly migration for exception assertion.
489-491: LGTM!Consistent Shouldly migration for exception assertion.
503-505: LGTM!Consistent Shouldly migration for exception assertion.
518-520: LGTM!Consistent Shouldly migration for exception assertion.
538-540: LGTM!Consistent Shouldly migration for exception assertion.
558-560: LGTM!Consistent Shouldly migration for exception assertion.
577-579: LGTM!Consistent Shouldly migration for exception assertion.
590-592: LGTM!Consistent Shouldly migration for exception assertion.
605-607: LGTM!Consistent Shouldly migration for exception assertion.
624-626: LGTM!Consistent Shouldly migration for exception assertion.
645-647: LGTM!Consistent Shouldly migration for exception assertion.
666-668: LGTM!Consistent Shouldly migration for exception assertion.
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt (1)
1-106: LGTM! PostScript test asset is well-formed.The PostScript rendering asset follows the correct structure with proper header declarations, constants, helper functions, and a valid QR module grid. The file correctly represents a 50×50 QR code with no quiet zones (25×25 modules with scale factor 2).
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt (1)
1-122: LGTM! EPS variant correctly formatted.The EPS (Encapsulated PostScript) test asset properly declares
EPSF-3.0format in the header and follows the standard structure for rendering a 165×165 QR code with scale factor 5.QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt (1)
1-122: LGTM! Size-specific rendering asset is correct.The PostScript asset correctly renders a 33×33 QR code with scale factor 1, producing a 1:1 size output. The structure and format are consistent with the other test assets.
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt (1)
1-122: LGTM! Color customization correctly applied.The PostScript asset correctly renders QR modules in red (RGB 1,0,0) on a blue background (RGB 0,0,1), matching the test parameters
Color.Red, Color.Bluespecified in the test harness.QRCoderTests/PostscriptQRCodeRendererTests.cs (2)
11-59: LGTM! Test migration to Shouldly is consistent.All five test methods correctly migrated to use
ShouldMatchApprovedwithNoDiff()option, ensuring exact matching against approved PostScript output. TheRemoveCreationDatepreprocessing step correctly strips timestamp metadata to prevent spurious failures.
61-68: LGTM! Regex pattern correctly removes CreationDate metadata.The regex pattern
%%CreationDate:.*\r?\n?correctly matches and removes PostScript timestamp comments. The optional line-ending handling (\r?\n?) accommodates both Windows (CRLF) and Unix (LF) line endings.QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt (1)
1-122: LGTM! Baseline PostScript test asset is well-formed.The simple PostScript rendering test asset follows the standard structure with default black modules (RGB 0,0,0) on a white background (RGB 1,1,1). The 165×165 output with scale factor 5 correctly represents the baseline QR code rendering.
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.russiapayment_generator_can_generate_payload_encoding_win1251.approved.txt (1)
1-1: LGTM!This approved test data file correctly represents the Win1251-encoded payload output. The encoding identifier (ST00011) matches the expected character set, and the structure follows the Russia Payment Order format.
QRCoderTests/PayloadGeneratorTests/RussiaPaymentOrderTests.cs (6)
74-76: LGTM!The exception assertion correctly migrated to Shouldly's
Should.Throw<>()pattern. Both exception type and message are properly verified.
90-92: LGTM!Exception handling properly migrated to Shouldly. Using
ShouldStartWithis appropriate for verifying the error message prefix while allowing flexibility in the exact byte count reported.
113-113: LGTM!The migration to
Should.NotThrow()correctly asserts that the 300-byte limit applies only to mandatory fields, allowing optional fields to exceed this limit.
125-127: LGTM!Constructor validation exception properly migrated to Shouldly pattern with exact message verification.
138-140: LGTM!Pattern validation exception correctly migrated to Shouldly, with the exception message properly verifying both the field name and the invalid value that failed validation.
34-34: Encoding is handled correctly by ShouldMatchApproved(byte[])
Shouldly’s byte[] overload performs a binary comparison, so the raw Win1251 bytes in the approved file are matched without any encoding conversion.QRCoderDemo/QRCoderDemo.csproj (1)
4-4: LGTM! Framework upgrade is appropriate.The upgrade from .NET Framework 4.5 to 4.6.2, addition of
GenerateResourceUsePreserializedResources, and the correspondingSystem.Resources.Extensionspackage reference are all consistent and appropriate changes for improving resource handling in the demo project.Also applies to: 8-8, 16-16, 20-22
QRCoderTests/QRCoderTests.csproj (1)
21-22: LGTM! Dependencies support approval-based testing.The addition of
KGySoft.DrawingandKGySoft.Drawing.Corepackages enables platform-independent image encoding for visual regression testing, which aligns with the PR's objective to migrate to approval-based checks.QRCoderTests/Helpers/HelperFunctions.cs (3)
97-98: LGTM! Modern using declaration with explicit null check.The refactor to using declarations and null-coalescing throw improves readability and makes resource unavailability explicit.
108-111: LGTM! Consistent pattern with proper stream handling.The refactored method correctly copies the resource stream to a MemoryStream and returns the byte array, with explicit error handling for missing resources.
121-123: LGTM! Consistent refactoring with other helper methods.The using declaration and null-coalescing throw pattern is consistent with the other refactored helpers in this file.
QRCoderTests/XamlQRCodeRendererTests.cs (1)
20-20: LGTM! Approval-based testing migration.The migration from hash-based assertion to
ShouldMatchApproved()aligns with the PR's objective to adopt visual regression testing with Shouldly.QRCoderTests/BitmapByteQRCodeRendererTests.cs (1)
15-15: LGTM! Consistent migration to approval-based image testing.All three tests correctly migrate from hash-based assertions to
ShouldMatchApprovedImage(), which is the appropriate extension method for validating byte array image data through visual regression checks.Also applies to: 25-25, 34-34
QRCoderTests/QRCodeRendererTests.cs (1)
18-18: LGTM! Comprehensive migration to approval-based testing.All QR code renderer tests consistently migrate from hash-based assertions to
ShouldMatchApproved(), covering various rendering scenarios (standard graphics, hex colors, logos with borders, transparency). This aligns perfectly with the PR's objective to adopt visual regression testing.Also applies to: 27-27, 37-37, 50-50, 61-61, 74-74, 87-87, 100-100, 116-116
QRCoderTests/Base64QRCodeRendererTests.cs (1)
56-57: LGTM! Smart approach for JPEG compression handling.Converting to monochrome before approval comparison is an effective strategy to eliminate JPEG compression artifacts and ensure stable visual regression checks.
QRCoderTests/SvgQRCodeRendererTests.cs (1)
21-21: LGTM! Comprehensive SVG approval testing migration.All SVG renderer tests consistently migrate to
ShouldMatchApproved(x => x.NoDiff().WithFileExtension("svg")). The use ofNoDiff()is appropriate for text-based SVG content where exact matching is expected, and the explicit file extension ensures proper approval file naming.Also applies to: 31-31, 41-41, 51-51, 61-61, 71-71, 88-88, 104-104, 120-120, 137-137, 153-153, 168-168, 184-184
QRCoderTests/PngByteQRCodeRendererTests.cs (8)
1-4: LGTM!The import changes are appropriate for the migration to approval-based testing with image types.
20-27: LGTM!The migration from hash-based assertion to
ShouldMatchApprovedImageproperly enables visual regression testing for the black/white PNG QR code.
30-37: LGTM!The approval-based assertion properly handles colored PNG validation.
40-47: LGTM!The test correctly uses
System.Drawing.Colortypes and migrates to approval-based testing.
50-57: LGTM!The alpha channel transparency test properly migrates to approval-based validation.
60-67: LGTM!The quiet zones parameter variation is properly tested via approval-based validation.
78-83: LGTM!The helper method test consistently uses the approval-based assertion pattern.
86-91: LGTM!The second helper method overload test completes the migration to approval-based testing.
QRCoderTests/AsciiQRCodeRendererTests.cs (6)
10-50: LGTM!The migration to triple-quoted raw string literals significantly improves readability of ASCII art expectations. The line-ending-tolerant comparison is essential for cross-platform test stability.
52-79: LGTM!The compact ASCII rendering test properly uses raw string literals and line-ending-tolerant comparison.
81-103: LGTM!The quiet zones variation test follows the consistent pattern established in other tests.
105-131: LGTM!The inverted rendering test properly validates the invert parameter using the consistent assertion pattern.
133-159: LGTM!The custom end-of-line test validates that the
endOfLineparameter is accepted. UsingIgnoreLineEndingsin the assertion ensures cross-platform stability while still exercising the parameter.
161-343: LGTM!All remaining ASCII QR code tests consistently use raw string literals and line-ending-tolerant assertions, properly testing various rendering options (quiet zones, custom symbols, helper methods).
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (3)
20-25: LGTM!The byte-array-to-image conversion properly handles resource disposal and delegates to the core Bitmap comparison method.
40-62: Robust stack trace analysis.The stack frame walking logic properly skips
[ShouldlyMethods]-attributed classes to find the actual test method. This ensures accurate file naming for approval files.
99-123: LGTM!The byte-wise comparison using
AsSpan().SequenceEqualis efficient, and the cleanup logic (deleting received file on match) keeps the test directory tidy.
|
Finally, tests pass on both of my dev computers! Apparently PNGs were not being encoded the same way by GDI+ across various PCs or frameworks so there would be differences in hashes. (This has been a constant battle over time.) Now, primary comparison across all files are based on a third-party library which uses its own logic to create gif files (not OS dependent). So long as we don't update the dependency, the gif files will always be created identical regardless of platform. Saving approvals as GIF files versus hashes makes it is very easy to review tests to ensure that images are created correctly or what changed. In a few cases there is special logic:
|
|
I am also testing the debug build now, due to problems with the tests in release mode. |
|
Note: this comes up in the GitHub diff due to the logo image removed from the SVG by the test (and verified separately): |
| var result = HelperFunctions.StringToHash(svg); | ||
| result.ShouldBe("04b12051632549cbb1879a0fe1353731"); | ||
| // remove PNG encoded bitmap from SVG and verify it separately (to avoid diffs due to different encoding settings of System.Drawing) | ||
| var regex = new Regex( | ||
| @"<image\b[^>]*\bxlink:href\s*=\s*[""']data:[^;""']+;base64,([A-Za-z0-9+/=]+)[""']", | ||
| RegexOptions.IgnoreCase); | ||
| var match = regex.Match(svg); | ||
| if (!match.Success || match.Groups.Count < 2) | ||
| throw new Exception("Could not find embedded image data in SVG output."); | ||
| var base64Data = match.Groups[1].Value; | ||
| var imageData = Convert.FromBase64String(base64Data); | ||
| imageData.ShouldMatchApprovedImage(discriminator: "embeddedLogo"); | ||
| svg = svg.Replace(base64Data, "=====BASE64DATA====="); | ||
| svg.ShouldMatchApproved(x => x.NoDiff().WithFileExtension("svg")); |
|
This PR has NO changes to the QRCoder base library; it's just the tests |
There was a problem hiding this comment.
Hooray for raw string literals.
Summary by CodeRabbit
Tests
Chores