Fix ECC level Q for micro QR codes; add various tests to improve code coverage #649
Conversation
📝 WalkthroughWalkthroughAdds extensive unit tests for OneTimePassword, Slovenian UPN QR, Swiss QR Code, and micro/QR generator behaviors; introduces a guard in QRCodeGenerator.GenerateMicroQrCode to auto-select Micro version M4 for ECCLevel.Q when no version is requested. No API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant QRCodeGenerator
Caller->>QRCodeGenerator: GenerateMicroQrCode(data, ECCLevel = Q, requestedVersion = 0)
alt requestedVersion == 0 and ECCLevel == Q
QRCodeGenerator->>QRCodeGenerator: set requestedVersion = -4 (select M4)
Note right of QRCodeGenerator#lightgreen: Implicit M4 selection for ECC Q
end
QRCodeGenerator->>QRCodeGenerator: validate version vs ECC
QRCodeGenerator-->>Caller: generate Micro QR (M4) or throw if invalid
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/SlovenianUpnQrTests.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
QRCoderTests/PayloadGeneratorTests/SlovenianUpnQrTests.cs (1)
QRCoder/QRCodeGenerator.cs (1)
ECCLevel(268-276)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (1)
QRCoder/PayloadGenerator/OneTimePassword.cs (3)
PayloadGenerator(3-236)OneTimePassword(8-235)ToString(125-130)
QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs (1)
QRCoder/PayloadGenerator/SwissQrCode.cs (24)
PayloadGenerator(3-650)SwissQrCode(8-649)SwissQrCode(46-75)Contact(347-538)Contact(385-386)Contact(391-392)Contact(395-466)Iban(262-342)Iban(272-282)Reference(150-257)Reference(161-180)ToString(293-294)ToString(480-490)ToString(544-598)SwissQrCodeContactException(510-537)SwissQrCodeContactException(515-517)SwissQrCodeContactException(523-526)SwissQrCodeContactException(533-536)SwissQrCodeException(621-648)SwissQrCodeException(626-628)SwissQrCodeException(634-637)SwissQrCodeException(644-647)AdditionalInformation(80-145)AdditionalInformation(89-96)
🪛 GitHub Actions: Format (Pull Request)
QRCoderTests/PayloadGeneratorTests/SlovenianUpnQrTests.cs
[error] 445-445: FINALNEWLINE: Fix final newline. Insert '\n'.
⏰ 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 Framework 4.6.2
- GitHub Check: additional-tests
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
🔇 Additional comments (10)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (3)
240-324: LGTM! Comprehensive validation test coverage.The validation tests thoroughly cover all error scenarios for the OneTimePassword generator, including null/empty/whitespace secrets, invalid characters in issuer/label, and missing required fields. The tests properly validate both the exception type and message.
326-417: LGTM! Thorough edge case coverage.These tests effectively validate edge cases such as:
- Only issuer provided (no label)
- Neither issuer nor label provided
- Zero values for counter, period, and digits
This ensures the generator handles minimal and boundary inputs correctly.
419-515: LGTM! Excellent coverage of encoding and large values.These tests validate:
- Proper URI encoding of special characters (
+,&,/) in labels and issuers- Correct space stripping from secrets
- Handling of large values (999999999 for counter, 3600 for period, 10 for digits)
- SHA512 algorithm support for HOTP
This ensures robust handling of real-world inputs and edge cases.
QRCoderTests/PayloadGeneratorTests/SwissQrCodeTests.cs (3)
589-668: LGTM! Comprehensive combined address testing.These tests thoroughly validate the combined address functionality:
- Proper generation with "K" type indicator
- Name validation (empty, length > 70 chars)
- Address line validation (length > 70 chars)
- Country code validation
This ensures the combined address type works correctly alongside the existing structured address type.
670-725: LGTM! Thorough reference type and IBAN validation.These tests validate critical reference type and IBAN interactions:
- NON (no reference) minimal payload
- SCOR (ISO 11649 creditor reference) formatting
- Liechtenstein IBAN support (LI prefix)
- QRR reference type must use QR-IBAN (not regular IBAN)
This ensures compliance with Swiss QR code specifications for reference handling.
727-781: LGTM! Complete additional information handling.These tests validate all combinations of additional information:
- Unstructured message only (with EPD trailer)
- Bill information only (empty message, EPD, then bill info)
- Empty additional information (EPD trailer only)
- QRR reference with default (empty) reference text
This ensures proper formatting of the optional additional information section according to Swiss QR code specifications.
QRCoderTests/PayloadGeneratorTests/SlovenianUpnQrTests.cs (4)
5-104: LGTM! Solid foundation tests for Slovenian UPN QR.These tests establish the core functionality:
- Basic payload structure with all required fields
- Deadline formatting (31.12.2024 format)
- Custom code handling (GDSV)
- SI model and reference combination (SI12 with reference number)
The tests validate proper line-by-line output formatting with newline separators.
106-244: LGTM! Comprehensive validation and formatting coverage.These tests validate critical data handling:
- Field length truncation (payer name: 33 chars, description: 42 chars)
- Amount formatting (11-digit zero-padded cents format)
- Checksum calculation (3-digit value at end)
- Whitespace trimming for all text fields
This ensures robust data normalization and validation.
246-293: LGTM! Proper normalization and QR configuration.These tests validate:
- Uppercasing of code (othr → OTHR), IBAN, and SI model
- QR code properties specific to Slovenian UPN QR standard:
- Version: 15
- ECC Level: M
- ECI Mode: ISO-8859-2 (Latin-2 character encoding)
This ensures compliance with the Slovenian UPN QR specification.
295-444: LGTM! Excellent edge case coverage.These tests validate critical edge cases:
- Zero amount handling (00000000000 format)
- Length limits (IBAN: 34 chars, reference: 22 chars after SI model prefix)
- Null deadline produces empty line
- Amount rounding (10.555 → 10.56, 10.554 → 10.55)
This ensures robust handling of boundary conditions and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (1)
241-487: Consider parameterized tests to reduce duplication.While the current test structure is clear and follows existing patterns, you could optionally consolidate similar tests using xUnit's
[Theory]and[InlineData]attributes. For example, the validation tests (lines 241-324) could be combined into a single theory, and the encoding tests (lines 420-444) could be parameterized.Example for validation tests:
[Theory] [InlineData(null, "Secret must be a filled out base32 encoded string")] [InlineData("", "Secret must be a filled out base32 encoded string")] [InlineData(" ", "Secret must be a filled out base32 encoded string")] public void one_time_password_generator_should_throw_for_invalid_secret(string? secret, string expectedMessage) { var pg = new PayloadGenerator.OneTimePassword { Secret = secret!, Issuer = "Google", Label = "test@google.com", }; Should.Throw<InvalidOperationException>(() => pg.ToString()) .Message.ShouldBe(expectedMessage); }This is entirely optional—the current approach is perfectly valid and may be preferred for test readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (1)
QRCoder/PayloadGenerator/OneTimePassword.cs (3)
PayloadGenerator(3-236)OneTimePassword(8-235)ToString(125-130)
⏰ 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 Framework 4.6.2
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
🔇 Additional comments (1)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (1)
240-487: Excellent test coverage!The new tests comprehensively cover validation paths, edge cases, encoding behavior, and boundary conditions for the OneTimePassword payload generator. All assertions correctly match the expected behavior from the production code.
Shane32
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoderTests/QRGeneratorTests.cs (1)
767-773: Test validates the new auto-selection behavior.This test correctly verifies that calling
GenerateMicroQrCodewithECCLevel.Qand no explicit version automatically selects M4. The assertions confirm both the resulting matrix size (17×17 for M4) and the version property (-4).Optional: Consider adding a test for auto-selection with data too long.
While not critical (the existing validation will handle it), you could add a test verifying that when Q level is used without an explicit version and the data exceeds M4's capacity, a
DataTooLongExceptionis still thrown. This would document the expected behavior more completely.Example:
[Fact] public void micro_qr_throws_when_q_auto_select_but_data_too_long() { // M4 with Q can hold max 13 alphanumeric characters var input = new string('A', 14); var ex = Should.Throw<QRCoder.Exceptions.DataTooLongException>(() => QRCodeGenerator.GenerateMicroQrCode(input, ECCLevel.Q)); ex.Message.ShouldContain("exceeds the maximum size", Case.Insensitive); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/QRCodeGenerator.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/SlovenianUpnQrTests.cs(1 hunks)QRCoderTests/QRGeneratorTests.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoderTests/PayloadGeneratorTests/SlovenianUpnQrTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/QRGeneratorTests.cs (3)
QRCoder/QRCodeGenerator.cs (2)
ECCLevel(270-278)QRCodeGenerator(17-19)QRCoder/Exceptions/DataTooLongException.cs (3)
DataTooLongException(6-30)DataTooLongException(14-17)DataTooLongException(26-29)QRCoder/QRCodeGenerator/CapacityTables.cs (1)
QRCodeGenerator(3-395)
⏰ 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 Framework 4.6.2
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: additional-tests
🔇 Additional comments (4)
QRCoder/QRCodeGenerator.cs (2)
177-178: LGTM! Smart usability improvement.The auto-selection of M4 for ECCLevel.Q when no explicit version is provided improves the user experience. The placement before the validation check at line 179 ensures that users can now call
GenerateMicroQrCode(text, ECCLevel.Q)without having to know that Q is only supported on M4. The logic correctly setsrequestedVersion = -4only whenrequestedVersion == 0, preserving the existing behavior for explicit version requests.
177-178: LGTM! Good UX improvement.Auto-selecting M4 when Q level is specified without an explicit version is a sensible default, since Q is only supported on M4. This improves the developer experience by avoiding an exception that would otherwise require the user to read the error message and manually specify M4.
QRCoderTests/QRGeneratorTests.cs (2)
44-45: Clarifying comments improve test readability.The added comments explicitly document the Micro QR version (M4) and ECC levels being tested, making the test cases easier to understand.
663-746: Excellent comprehensive edge case coverage.These 10 new test methods thoroughly validate:
- Data-too-long scenarios for both Micro QR and standard QR
- Invalid version range checks (too low/too high)
- ECC level constraints (H not supported, Q requires M4, M1 requires Default)
- Null input validation
- Invalid ECC level casting
The capacity numbers referenced in comments are accurate per the capacity tables (e.g., M4 with L holds 35 alphanumeric chars, M2 with L holds 10). All assertions correctly verify exception types, parameter names, and error messages.
Shane32
added
bug
Summary by CodeRabbit
Tests
Bug Fixes