Make Postscript renderer deterministic #643
Conversation
📝 WalkthroughWalkthroughRefactors PostScript generation in QRCoder/PostscriptQRCode.cs to remove date/time from headers, reformat header/functions via multiline strings with reordered placeholders, and adjust call sites accordingly. Updates tests to remove date-stripping helper and assert directly against approved outputs. Public API remains unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
| private const string PS_HEADER = @"%!PS-Adobe-3.0 {3} | ||
| %%Creator: QRCoder.NET | ||
| %%Title: QRCode | ||
| %%CreationDate: {0} |
There was a problem hiding this comment.
This "CreationDate" line is the only removed line; the constant is otherwise identical. (although {1} becomes {0} etc)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
QRCoder/PostscriptQRCode.cs(2 hunks)QRCoderTests/PostscriptQRCodeRendererTests.cs(5 hunks)
⏰ 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 Core 3.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 5.0
- GitHub Check: additional-tests
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET 6.0
- GitHub Check: Test .NET Core 2.1
🔇 Additional comments (9)
QRCoder/PostscriptQRCode.cs (4)
103-106: LGTM! Date parameter correctly removed.The format call has been updated to remove the DateTime.Now parameter, achieving the goal of deterministic output. The placeholder indices {0}, {1}, {2} correctly map to width, pointsPerModule, and EPSF string respectively.
135-155: LGTM! PS_HEADER format string is correct.The raw string literal format is clean and the placeholder indices are correctly positioned:
- {2} for EPSF version token (line 136)
- {0} for width/size values (lines 141, 142, 148)
- {1} for scale/pointsPerModule (line 149)
157-193: LGTM! PS_FUNCTIONS format string is correct.The raw string literal format is clean and all placeholders {0}-{6} are correctly positioned for RGB colors and module count.
195-201: LGTM! PS_FOOTER format string is correct.The raw string literal format is clean and requires no placeholders.
QRCoderTests/PostscriptQRCodeRendererTests.cs (5)
12-12: LGTM! Test simplified correctly.The test now directly asserts against approved output since the production code no longer includes a creation date, eliminating the need for the
RemoveCreationDatehelper.
22-22: LGTM! Test simplified correctly.Consistent with the removal of date-dependent output from the PostScript generator.
32-32: LGTM! Test simplified correctly.Consistent with the removal of date-dependent output from the PostScript generator.
42-42: LGTM! Test simplified correctly.Consistent with the removal of date-dependent output from the PostScript generator.
52-52: LGTM! Test simplified correctly.Consistent with the removal of date-dependent output from the PostScript generator.
| // Note: line terminations here will encode differently based on which platform QRCoder was compiled on (CRLF vs LF); | ||
| // however, PostScript interpreters should handle both equally well. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify cross-platform test determinism.
The comment correctly notes that line terminations will differ based on compilation platform (CRLF on Windows, LF on Unix). While PostScript interpreters handle both, this could cause approval test failures when developers compile on different platforms and the tests perform exact string comparisons.
Consider one of these approaches:
- Normalize line endings in the generated PostScript to always use LF (more deterministic)
- Update approval tests to ignore line ending differences
- Document that developers should use consistent line endings in their development environment
Run this script to check how the approval tests handle line ending differences:
🏁 Script executed:
#!/bin/bash
# Check if approval tests normalize line endings
# Search for the ShouldMatchApproved implementation and configuration
rg -n "ShouldMatchApproved|NoDiff" --type cs -A 5 -B 5
# Look for any line ending normalization in test utilities
rg -n "LineEnding|CRLF|\\\\r\\\\n|normalize" --type csLength of output: 66105
Ensure deterministic LF line endings in PostScript output
Approval tests use exact NoDiff comparisons, so CRLF vs LF variations break on different platforms.
- Normalize GetGraphic to emit LF-only line endings
- Update PostscriptQRCodeRendererTests to ignore CRLF/LF differences
🤖 Prompt for AI Agents
In QRCoder/PostscriptQRCode.cs around lines 133-134, the PostScript output
currently may contain platform-dependent CRLF line endings; modify GetGraphic so
it deterministically returns LF-only line endings by normalizing the final
string (replace "\r\n" and any lone "\r" with "\n") before returning; also
update PostscriptQRCodeRendererTests to either compare normalized strings
(replace CRLF with LF on both expected and actual) or use a test helper that
normalizes line endings so comparisons ignore CRLF vs LF differences.
| var data = gen.CreateQrCode("This is a quick test! 123#?", QRCodeGenerator.ECCLevel.L); | ||
| var ps = new PostscriptQRCode(data).GetGraphic(5); | ||
| RemoveCreationDate(ps).ShouldMatchApproved(x => x.NoDiff()); | ||
| ps.ShouldMatchApproved(x => x.NoDiff()); |
There was a problem hiding this comment.
Tests previously removed the creation date from the verified files. So all tests continue to match with no change to test result data.
Removes the optional creation date from the created postscript instructions. Should have no effect on output. Makes it easier for developers that want to write tests for their code that uses QRCoder to create postscript output. Also converted constants to raw string literals.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests