Add transposition tests #644
Conversation
📝 WalkthroughWalkthroughAdds a new transpose verification test suite with multiple renderer tests and matrix assertions, two approved ASCII snapshot files, and a test project NuGet dependency on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TR as Test Runner
participant TV as TransposeVerificationTests
participant R as Renderer (Bitmap/Png/Base64/Svg/Pdf/PostScript/Art/ASCII)
participant A as Approval Baseline
rect rgb(235,245,255)
note over TR,TV: Render & approve flow
TR->>TV: run test method
TV->>R: generate QR output (bytes/text/bitmap)
R-->>TV: rendered output
TV->>A: compare with approved snapshot
A-->>TV: match / diff
TV-->>TR: pass / fail
end
rect rgb(245,235,255)
note over TR,TV: Matrix position assertion
TR->>TV: run black_module_position()
TV->>TV: inspect QR matrix indices (row,column)
TV-->>TR: assert position not transposed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (8)
QRCoderTests/QRCoderTests.csproj (2)
16-16: Scope Svg to the TFMs that actually run it (optional).Svg is only used under SYSTEM_DRAWING. Consider scoping to Windows TFMs to reduce restore surface.
Apply this if you want to scope it:
- <PackageReference Include="Svg" Version="3.4.7" /> + <PackageReference Include="Svg" Version="3.4.7" + Condition="'$(TargetFramework)' == 'net462' or '$(TargetFramework)' == 'net5.0-windows' or '$(TargetFramework)' == 'net6.0-windows'" />Also please verify 3.4.7 supports all your target TFMs.
62-66: Guard global using System.Drawing to avoid TFM surprises (optional).Global using System.Drawing can fail on TFMs without a reference. Consider conditioning it to Windows TFMs or where SYSTEM_DRAWING is defined.
Apply if desired:
- <Using Include="System.Drawing" /> + <Using Include="System.Drawing" + Condition="'$(TargetFramework)' == 'net462' or '$(TargetFramework)' == 'net5.0-windows' or '$(TargetFramework)' == 'net6.0-windows'" />Alternatively, define SYSTEM_DRAWING in those TFMs and put this Using under that condition.
QRCoderTests/TransposeVerificationTests.ascii_renderer.Small.approved.txt (1)
1-15: Snapshot looks good. Ensure stable line endings and trailing newline.To avoid flaky diffs across OSes, ensure LF line endings and a trailing newline at EOF. Consider a .gitattributes rule for *.approved.txt.
QRCoderTests/TransposeVerificationTests.ascii_renderer.FullSize.approved.txt (1)
1-29: Approved baseline OK. Normalize EOL/EOF (optional).Same as the Small snapshot: ensure consistent LF EOLs and trailing newline to keep approvals stable across platforms.
QRCoderTests/TransposeVerificationTests.cs (4)
47-49: Dispose Bitmap to avoid leaks in tests.Wrap bitmap in using to release unmanaged resources.
- var bitmap = qrCode.GetGraphic(10, Color.Black, Color.White, Color.White, null, 1, true, ArtQRCode.QuietZoneStyle.Flat, ArtQRCode.BackgroundImageStyle.Fill, null); - bitmap.ShouldMatchApproved(); + using var bitmap = qrCode.GetGraphic(10, Color.Black, Color.White, Color.White, null, 1, true, ArtQRCode.QuietZoneStyle.Flat, ArtQRCode.BackgroundImageStyle.Fill, null); + bitmap.ShouldMatchApproved();
53-58: Dispose Bitmap (and optionally QRCode).Prevent GDI handle leaks during repeated test runs.
- var qrCode = new QRCode(_sharedQrCodeData); - var bitmap = qrCode.GetGraphic(10); - using var ms = new MemoryStream(); - bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); + using var qrCode = new QRCode(_sharedQrCodeData); + using var bitmap = qrCode.GetGraphic(10); + using var ms = new MemoryStream(); + bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); return ms.ToArray();
79-90: Dispose Bitmap; optionally dispose SvgDocument.Ensure deterministic cleanup.
- var svgDoc = Svg.SvgDocument.FromSvg<Svg.SvgDocument>(svgString); - var bitmap = svgDoc.Draw(bitmapSize, bitmapSize); - using var ms = new MemoryStream(); - bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); + using var svgDoc = Svg.SvgDocument.FromSvg<Svg.SvgDocument>(svgString); + using var bitmap = svgDoc.Draw(bitmapSize, bitmapSize); + using var ms = new MemoryStream(); + bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); return ms.ToArray();
22-22: Ensure SYSTEM_DRAWING is defined where intended.This guard relies on a symbol not defined in this csproj. If it’s defined elsewhere, ignore; otherwise, define it for Windows TFMs to include these tests.
Example (in csproj):
<PropertyGroup Condition="'$(TargetFramework)' == 'net462' or '$(TargetFramework)' == 'net5.0-windows' or '$(TargetFramework)' == 'net6.0-windows'"> <DefineConstants>$(DefineConstants);SYSTEM_DRAWING</DefineConstants> </PropertyGroup>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
QRCoderTests/TransposeVerificationTests.artqrcode_renderer.approved.gifis excluded by!**/*.gifQRCoderTests/TransposeVerificationTests.black_module_reference.approved.gifis excluded by!**/*.gifQRCoderTests/TransposeVerificationTests.image_renderers.approved.gifis excluded by!**/*.gifQRCoderTests/TransposeVerificationTests.pdf_renderer.approved.pdfis excluded by!**/*.pdf
📒 Files selected for processing (4)
QRCoderTests/QRCoderTests.csproj(2 hunks)QRCoderTests/TransposeVerificationTests.ascii_renderer.FullSize.approved.txt(1 hunks)QRCoderTests/TransposeVerificationTests.ascii_renderer.Small.approved.txt(1 hunks)QRCoderTests/TransposeVerificationTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/TransposeVerificationTests.cs (2)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (2)
ShouldMatchApprovedImage(17-22)ShouldMatchApproved(23-32)QRCoder/ASCIIQRCode.cs (1)
GetGraphicSmall(81-127)
⏰ 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). (7)
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 6.0
- GitHub Check: Test .NET 5.0
🔇 Additional comments (2)
QRCoderTests/TransposeVerificationTests.cs (2)
111-114: Baseline files present; no action needed. Verified PDF and PostScript approval files exist, so tests will pass.
119-122: Remove incorrect missing PS baseline note The file QRCoderTests/TransposeVerificationTests.postscript_renderer.approved.ps exists, so the PostScript test will pass as intended.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
QRCoderTests/TransposeVerificationTests.cs (2)
125-146: Fix ModuleMatrix indexing; currently asserting transposed coordinates.ModuleMatrix is indexed as
[x][y]. The current code treats the first index as row, so it looks at(y, x)and fails even when the QR code is oriented correctly. Compute quiet-zone offset and assert using[column][row].- var expectedRow = 4 * version + 9 + 4; - var expectedCol = 8 + 4; - var transposedRow = expectedCol; - var transposedCol = expectedRow; - - // Verify the black module is at the expected position - _sharedQrCodeData.ModuleMatrix[expectedRow][expectedCol].ShouldBeTrue( - $"Black module should be at position ({expectedRow}, {expectedCol})"); - - // Verify it's NOT at the transposed position (unless they happen to be the same) - if (expectedRow != transposedCol || expectedCol != transposedRow) - { - _sharedQrCodeData.ModuleMatrix[transposedRow][transposedCol].ShouldBeFalse( - $"Black module should NOT be at transposed position ({transposedRow}, {transposedCol})"); - } + var symbolSize = 21 + 4 * (version - 1); + var quietZone = (_sharedQrCodeData.ModuleMatrix.Count - symbolSize) / 2; + var expectedX = quietZone + 8; + var expectedY = quietZone + (4 * version + 9); + var transposedX = expectedY; + var transposedY = expectedX; + + _sharedQrCodeData.ModuleMatrix[expectedX][expectedY].ShouldBeTrue( + $"Black module should be at position (x={expectedX}, y={expectedY})"); + + if (expectedX != transposedX || expectedY != transposedY) + { + _sharedQrCodeData.ModuleMatrix[transposedX][transposedY].ShouldBeFalse( + $"Black module should NOT be at transposed position (x={transposedX}, y={transposedY})"); + }Based on relevant code snippet.
151-169: Correct reference matrix orientation and dispose renderers.This block repeats the row/column swap, so the lone true module is placed at the transposed coordinates. It also leaves
QRCodeData,QRCode, andBitmapundisposed. Align indices with[x][y], compute the quiet zone, and scope the disposables.- var blackModuleRow = 4 * version + 9 + 4; - var blackModuleCol = 8 + 4; - - // Create a minimal QRCodeData with just the black module - var referenceData = new QRCodeData(version, addPadding: true); - - // Set only the black module position to true - referenceData.ModuleMatrix[blackModuleRow][blackModuleCol] = true; - - // Render this reference image for human verification - var qrCode = new QRCode(referenceData); - var bitmap = qrCode.GetGraphic(10); - bitmap.ShouldMatchApproved(); + var symbolSize = 21 + 4 * (version - 1); + var quietZone = (_sharedQrCodeData.ModuleMatrix.Count - symbolSize) / 2; + var blackModuleX = quietZone + 8; + var blackModuleY = quietZone + (4 * version + 9); + + using var referenceData = new QRCodeData(version, addPadding: true); + referenceData.ModuleMatrix[blackModuleX][blackModuleY] = true; + + using var qrCode = new QRCode(referenceData); + using var bitmap = qrCode.GetGraphic(10); + bitmap.ShouldMatchApproved();Based on relevant code snippet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoderTests/TransposeVerificationTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/TransposeVerificationTests.cs (3)
QRCoder/QRCodeGenerator.cs (1)
ECCLevel(260-268)QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (2)
ShouldMatchApprovedImage(17-22)ShouldMatchApproved(23-32)QRCoder/ASCIIQRCode.cs (1)
GetGraphicSmall(81-127)
⏰ 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). (8)
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 6.0
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET 5.0
- GitHub Check: additional-tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
QRCoderTests/TransposeVerificationTests.cs (4)
44-83: Dispose QRCode instances to prevent resource leaks.The past review flagged that
QRCode,BitmapByteQRCode,Base64QRCode,PngByteQRCode, andSvgQRCodeall implementIDisposablebut are not being disposed in the helper methods. This can lead to resource leaks across test runs.
85-91: Dispose ArtQRCode to prevent GDI handle leaks.The past review flagged that
ArtQRCodeat line 88 implementsIDisposablebut is not being disposed. Without disposal, repeated test runs will leak GDI handles.
125-147: Incorrect ModuleMatrix indexing; fix coordinate order.The past review flagged that
ModuleMatrixis indexed as[x][y](column-first), but this test uses[row][col]ordering at lines 138 and 144. Additionally, the quiet zone is hardcoded as 4 instead of being computed from the matrix size.
149-169: Fix transposed indices and add disposal for QRCode.The past review flagged two issues that are still present:
- Line 163 uses transposed indices
[blackModuleRow][blackModuleCol]instead of[blackModuleCol][blackModuleRow]- Line 166 creates
QRCodewithout disposal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoderTests/TransposeVerificationTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/TransposeVerificationTests.cs (3)
QRCoder/QRCodeGenerator.cs (1)
ECCLevel(260-268)QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (2)
ShouldMatchApprovedImage(17-22)ShouldMatchApproved(23-32)QRCoder/ASCIIQRCode.cs (1)
GetGraphicSmall(81-127)
⏰ 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: Test .NET 5.0 Windows
🔇 Additional comments (2)
QRCoderTests/TransposeVerificationTests.cs (2)
1-20: LGTM!The test class structure and shared QR code data setup are well-designed. Using a single QR code instance across tests is efficient and appropriate for transpose verification.
94-107: LGTM!The ASCII renderer test correctly handles both full-size and small QR code variants with appropriate discriminators for approval testing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
QRCoderTests/TransposeVerificationTests.cs (3)
46-51: Dispose QR renderer instances in helpers to avoid GDI/resource leaks.Wrap all
*QRCodeinstances (and keep Bitmaps/streams) inusing var. This prevents handle leaks over repeated runs.private byte[] GetQRCodeBytes() { - var qrCode = new QRCode(_sharedQrCodeData); - using var bitmap = qrCode.GetGraphic(10); + using var qrCode = new QRCode(_sharedQrCodeData); + using var bitmap = qrCode.GetGraphic(10); using var ms = new MemoryStream(); bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); return ms.ToArray(); } private byte[] GetBitmapByteQRCodeBytes() { - var qrCode = new BitmapByteQRCode(_sharedQrCodeData); - return qrCode.GetGraphic(10); + using var qrCode = new BitmapByteQRCode(_sharedQrCodeData); + return qrCode.GetGraphic(10); } private byte[] GetBase64QRCodeBytes() { - var qrCode = new Base64QRCode(_sharedQrCodeData); - var base64String = qrCode.GetGraphic(10); + using var qrCode = new Base64QRCode(_sharedQrCodeData); + var base64String = qrCode.GetGraphic(10); return Convert.FromBase64String(base64String); } private byte[] GetPngByteQRCodeBytes() { - var qrCode = new PngByteQRCode(_sharedQrCodeData); - return qrCode.GetGraphic(10); + using var qrCode = new PngByteQRCode(_sharedQrCodeData); + return qrCode.GetGraphic(10); } private byte[] GetSvgQRCodeBytes() { - var qrCode = new SvgQRCode(_sharedQrCodeData); - var svgString = qrCode.GetGraphic(10); + using var qrCode = new SvgQRCode(_sharedQrCodeData); + var svgString = qrCode.GetGraphic(10); var bitmapSize = _sharedQrCodeData.ModuleMatrix.Count * 10; // use Svg.Net to render SVG to bitmap for comparison - var svgDoc = Svg.SvgDocument.FromSvg<Svg.SvgDocument>(svgString); + var svgDoc = Svg.SvgDocument.FromSvg<Svg.SvgDocument>(svgString); using var bitmap = svgDoc.Draw(bitmapSize, bitmapSize); using var ms = new MemoryStream(); bitmap.Save(ms, System.Drawing.Imaging.ImageFormat.Png); return ms.ToArray(); }Also applies to: 55-56, 61-63, 68-69, 74-83
88-91: Also dispose ArtQRCode to prevent leaks.Use
using varforArtQRCode.- var qrCode = new ArtQRCode(_sharedQrCodeData); - using var bitmap = qrCode.GetGraphic(10, Color.Black, Color.White, Color.White, null, 1, true, ArtQRCode.QuietZoneStyle.Flat, ArtQRCode.BackgroundImageStyle.Fill, null); + using var qrCode = new ArtQRCode(_sharedQrCodeData); + using var bitmap = qrCode.GetGraphic(10, Color.Black, Color.White, Color.White, null, 1, true, ArtQRCode.QuietZoneStyle.Flat, ArtQRCode.BackgroundImageStyle.Fill, null); bitmap.ShouldMatchApproved();
112-115: Dispose PDF and PostScript renderer instances.Wrap
PdfByteQRCodeandPostscriptQRCodeinusing var.public void pdf_renderer() { - var qrCode = new PdfByteQRCode(_sharedQrCodeData); + using var qrCode = new PdfByteQRCode(_sharedQrCodeData); var pdfBytes = qrCode.GetGraphic(10); pdfBytes.ShouldMatchApproved("pdf"); } public void postscript_renderer() { - var qrCode = new PostscriptQRCode(_sharedQrCodeData); + using var qrCode = new PostscriptQRCode(_sharedQrCodeData); var postscript = qrCode.GetGraphic(10); postscript.ShouldMatchApproved(x => x.NoDiff().WithFileExtension("ps")); }Also applies to: 120-123
🧹 Nitpick comments (2)
QRCoderTests/TransposeVerificationTests.cs (2)
128-147: Compute quiet-zone offset instead of hard-coding +4; keep row/col indexing.Future‑proofs the test if padding behavior changes.
- var version = _sharedQrCodeData.Version; - var expectedRow = 4 * version + 9 + 4; - var expectedCol = 8 + 4; + var version = _sharedQrCodeData.Version; + var symbolSize = 21 + 4 * (version - 1); + var quiet = (_sharedQrCodeData.ModuleMatrix.Count - symbolSize) / 2; + var expectedRow = quiet + (4 * version + 9); + var expectedCol = quiet + 8; var transposedRow = expectedCol; var transposedCol = expectedRow; // Verify the black module is at the expected position _sharedQrCodeData.ModuleMatrix[expectedRow][expectedCol].ShouldBeTrue( $"Black module should be at position ({expectedRow}, {expectedCol})");Based on QRCoder/ASCIIQRCode.cs showing ModuleMatrix[row][col] in use. [Based on relevant code snippet]
155-169: Reference image: derive quiet zone and dispose created objects.Use computed quiet zone and dispose
QRCode(and optionallyQRCodeDataif IDisposable).- var version = _sharedQrCodeData.Version; - var blackModuleRow = 4 * version + 9 + 4; - var blackModuleCol = 8 + 4; + var version = _sharedQrCodeData.Version; + var symbolSize = 21 + 4 * (version - 1); + // quiet zone size per side from matrix size + var quiet = (_sharedQrCodeData.ModuleMatrix.Count - symbolSize) / 2; + var blackModuleRow = quiet + (4 * version + 9); + var blackModuleCol = quiet + 8; @@ - var referenceData = new QRCodeData(version, addPadding: true); + using var referenceData = new QRCodeData(version, addPadding: true); @@ - var qrCode = new QRCode(referenceData); - using var bitmap = qrCode.GetGraphic(10); + using var qrCode = new QRCode(referenceData); + using var bitmap = qrCode.GetGraphic(10); bitmap.ShouldMatchApproved();Based on QRCoder/ASCIIQRCode.cs showing ModuleMatrix[row][col] in use. [Based on relevant code snippet]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
QRCoderTests/TransposeVerificationTests.ascii_renderer.Small.approved.txt(1 hunks)QRCoderTests/TransposeVerificationTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoderTests/TransposeVerificationTests.ascii_renderer.Small.approved.txt
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/TransposeVerificationTests.cs (2)
QRCoderTests/Helpers/ShouldMatchApprovedBinaryExtensions.cs (2)
ShouldMatchApprovedImage(15-20)ShouldMatchApproved(21-30)QRCoder/ASCIIQRCode.cs (1)
GetGraphicSmall(81-127)
⏰ 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). (6)
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Framework 4.6.2
🔇 Additional comments (1)
QRCoderTests/TransposeVerificationTests.cs (1)
78-79: SvgDocument does not require disposal
Svg.SvgDocument does not implement IDisposable in v3.4.7, so nousingwrapper is needed.
The ASCII "small" renderer is currently rendering QR codes transposed along the x/y diagonal. This suite of tests will help to catch these problems.
As many as possible of the renderers (png, base64, bmp, Bitmap, svg) will compare against the same reference image; in so doing, it will be easier to verify that changes do not cause transposition, and that someone does not accidentally save a transposed image as valid. However, some renderers (art, pdf, postscript, ascii) will require human verification at this time.
All tests will render the exact same QR code, so any test can be simply verified against any other of the tests.
There is also a 'reference' image showing the correct black-module location; the transposed location is white. Additionally, there is a test to ensure that the transposed location is white (since this is not enforced by the QR specification, but merely the chosen QR code is white there to discern from the black-module position).
Summary by CodeRabbit
Tests
Chores