Support Contract/Snapshot Testing using Verify#102
Conversation
✅ Deploy Preview for bookwormdev canceled.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis update introduces snapshot-based contract testing across multiple service unit tests by adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant TestMethod
participant SnapshotTestBase
participant Verifier
participant SnapshotsDir
TestMethod->>SnapshotTestBase: Call VerifySnapshot(message/command/event)
SnapshotTestBase->>Verifier: Configure scrubbers, start verification
Verifier->>SnapshotTestBase: Request snapshot directory
SnapshotTestBase->>SnapshotsDir: Resolve Snapshots/ directory (traverse up to project root)
SnapshotsDir-->>SnapshotTestBase: Return path
Verifier->>SnapshotsDir: Save or compare snapshot JSON
SnapshotsDir-->>Verifier: Snapshot result
Verifier-->>TestMethod: Verification result (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (63)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (16)
🚧 Files skipped from review as they are similar to previous changes (46)
⏰ 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). (4)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
foxminchan
changed the title
6fb252d to
4d2cfb9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces contract/snapshot testing capabilities using the Verify library to ensure API consistency across the BookWorm microservices. The changes establish a standardized approach to verifying message contracts and data structures in integration event handlers and consumers.
Key changes include:
- Addition of a shared
SnapshotTestBaseclass that provides contract testing infrastructure with deterministic data scrubbing - Migration from MassTransit.TestFramework to Verify.TUnit for enhanced snapshot testing capabilities
- Implementation of contract verification tests across all service boundaries to prevent breaking changes
Reviewed Changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/BookWorm.Common/SnapshotTestBase.cs | New base class providing snapshot testing infrastructure with GUID/DateTime scrubbing |
| Multiple service test projects | Updated to inherit from SnapshotTestBase and include contract verification tests |
| Directory.Packages.props | Added Verify.TUnit package and removed MassTransit.TestFramework dependency |
| Snapshot files (.verified.json) | Generated contract snapshots for all integration events and commands |
| Consumer test files | Enhanced with contract verification logic to ensure message structure consistency |
Comments suppressed due to low confidence (1)
src/Services/Finance/BookWorm.Finance.UnitTests/OrderStateMachineTests.cs:647
- Missing semicolon after the VerifySnapshot method call.
await VerifySnapshot(retryCommand)
| var content = builder.ToString(); | ||
|
|
||
| // Scrub GUIDs to make snapshots deterministic | ||
| var scrubbedContent = GuidRegex().Replace(content, "Guid_Scrubbed"); |
There was a problem hiding this comment.
The replacement text "Guid_Scrubbed" is inconsistent with the actual snapshot files which show "Guid_1", "Guid_2", etc. This suggests the scrubber may not be working as intended or the snapshots were generated differently.
| var scrubbedContent = GuidRegex().Replace(content, "Guid_Scrubbed"); | |
| var guidMap = new Dictionary<string, string>(); | |
| int guidCounter = 1; | |
| var scrubbedContent = GuidRegex().Replace(content, match => | |
| { | |
| var guid = match.Value; | |
| if (!guidMap.TryGetValue(guid, out var replacement)) | |
| { | |
| replacement = $"Guid_{guidCounter++}"; | |
| guidMap[guid] = replacement; | |
| } | |
| return replacement; | |
| }); |
| var scrubbedContent = GuidRegex().Replace(content, "Guid_Scrubbed"); | ||
|
|
||
| // Scrub DateTime values to make snapshots deterministic | ||
| scrubbedContent = DateTimeRegex().Replace(scrubbedContent, "DateTime_Scrubbed"); |
There was a problem hiding this comment.
The replacement text "DateTime_Scrubbed" is inconsistent with the actual snapshot files which show "DateTime_1", "DateTime_2", etc. This suggests the scrubber may not be working as intended or the snapshots were generated differently.
| scrubbedContent = DateTimeRegex().Replace(scrubbedContent, "DateTime_Scrubbed"); | |
| int dateTimeCounter = 1; | |
| scrubbedContent = DateTimeRegex().Replace( | |
| scrubbedContent, | |
| match => $"DateTime_{dateTimeCounter++}" | |
| ); |
![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
♻️ Duplicate comments (5)
src/Services/Catalog/BookWorm.Catalog.UnitTests/BookWorm.Catalog.UnitTests.csproj (2)
9-10: Same concern on unpinnedVerify.TUnitversion – see earlier remark in Rating unit-tests project.
12-15: Shared utility should be referenced, not linked – identical feedback as given for Rating unit-tests.src/Services/Finance/BookWorm.Finance.UnitTests/BookWorm.Finance.UnitTests.csproj (2)
8-9: UnpinnedVerify.TUnitversion – same recommendation to add an explicit version or central package management.
11-14: Linked file maintenance issue – see earlier advice to replace compile linking with a shared testing project reference.src/Services/Rating/BookWorm.Rating.UnitTests/Snapshots/BookUpdatedRatingFailedConsumerTests.GivenValidFailedRatingEvent_WhenHandling_ThenShouldDeleteFeedback.verified.json (1)
1-5: Mirror suggestions from sibling snapshotApply the same newline/placeholder consistency tweaks here to keep all rating snapshots uniform.
🧹 Nitpick comments (28)
README.md (1)
44-45: Keep checklist tense consistentAll items above use “Conducted / Implemented / Established”. New bullet could mirror that (“Implemented contract tests ✅”) to stay uniform.
src/Services/Notification/BookWorm.Notification.UnitTests/BookWorm.Notification.UnitTests.csproj (1)
12-15: Relative include path is brittle—consider Shared Test projectDeep
..\..\..\..\hops break quickly when folders move. Two alternatives:
- Create a small
BookWorm.Common.Testsproject and reference it via<ProjectReference>from every unit-test project.- If you keep a link, define a MSBuild property (
$(CommonTestsDir)) once and consume it, e.g.:<PropertyGroup> <CommonTestsDir>$(SolutionDir)tests\BookWorm.Common\</CommonTestsDir> </PropertyGroup> <ItemGroup> <Compile Include="$(CommonTestsDir)SnapshotTestBase.cs" Link="SnapshotTestBase.cs" /> </ItemGroup>This hardens maintainability across the repo.
src/Services/Rating/BookWorm.Rating.UnitTests/BookWorm.Rating.UnitTests.csproj (2)
9-10: Pin theVerify.TUnitpackage version for deterministic buildsLeaving the version implicit allows NuGet to float to newer releases, which can silently break or invalidate existing snapshots.
Specify an explicit version or manage the package centrally viaDirectory.Packages.propsto lock the tool-chain across all pipelines.
12-15: Prefer a shared test utility project over file linkingLinking
SnapshotTestBase.csinto each test project creates duplication and maintenance overhead.
Extracttests/BookWorm.Commoninto a.csproj(e.g.,BookWorm.Testing.Shared) and reference it, keeping a single source of truth and avoiding compile-item drift.src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/CancelOrderConsumerTests.GivenValidCancelOrderCommand_WhenHandling_ThenShouldSendEmail.verified.json (1)
1-8: Confirm decimal culture & newline for snapshot stabilityLooks solid, but two tiny details can help avoid flaky diffs:
99.99– ensure the serializer is forced toInvariantCultureso commas/periods don’t swap on different locales.- There’s no terminating newline after
}(and the file carries a UTF-8 BOM). Adding a final LF and stripping the BOM keeps Git diffs cleaner and makes some *nix tooling happier.Nothing blocking, just polish.
src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/ResendErrorEmailConsumerTests.GivenEmailsExist_WhenHandlingResendEvent_ThenShouldResendAllEmails.verified.json (1)
1-4: Snapshot captures only Id/CreationDate – intentional?If the integration event ever grows additional fields, this minimal snapshot won’t alert us. Consider snapshotting the full contract (even if most values are placeholders) to future-proof the test.
Not urgent, but worth double-checking the test intent.
src/Services/Basket/BookWorm.Basket.UnitTests/BookWorm.Basket.UnitTests.csproj (1)
9-15: Pin Verify.TUnit version & include<PrivateAssets>Leaving the version floating can unexpectedly break CI when Verify releases a major version. Recommend pinning the same version used elsewhere and shielding it from production transitive flow:
- <PackageReference Include="Verify.TUnit" /> + <PackageReference Include="Verify.TUnit" Version="30.5.0" PrivateAssets="all" />Keeps test deps deterministic and prevents them leaking into consumer projects.
src/Services/Rating/BookWorm.Rating.UnitTests/Snapshots/BookUpdatedRatingFailedConsumerTests.GivenFailedRatingEventWithNonExistingFeedback_WhenHandling_ThenShouldReturnGracefully.verified.json (1)
1-5: Add trailing newline & check placeholder consistencySame minor housekeeping as other snapshots:
- Append a trailing
\nto avoid diff noise.- Confirm
Guid_1vsGuid_2mapping matches deterministic scrub logic (first placeholder should always map to the same property across snapshots).Purely cosmetic.
src/Services/Ordering/BookWorm.Ordering.UnitTests/Snapshots/DeleteBasketFailedConsumerTests.GivenDeleteBasketFailedCommandWithNonExistingOrder_WhenHandling_ThenShouldReturnGracefully.verified.json (1)
1-8: Minor formatting inconsistency on monetary value
The amount is expressed as200.00, whereas other snapshots (see Finance) serialize as100.0. While Verify will still match, a uniform decimal formatter (e.g.JsonSerializerOptions.NumberHandlingor a customMoneyJsonConverter) would keep all money fields at a consistent0.00precision and eliminate noisy diffs.src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/CancelOrderConsumerTests.GivenCancelOrderCommandWithEmptyEmail_WhenHandling_ThenShouldNotSendEmail.verified.json (1)
1-8: Placeholder data looks safe; consider neutral full-name
"John Doe"is fine for non-PII test data, but several teams prefer obviously fake names like"Example User"to avoid confusion with production dumps. No blocker—just a style consideration.src/Services/Finance/BookWorm.Finance.UnitTests/Snapshots/OrderStateMachineTests.GivenOrderStatusChangedToCancelEvent_WhenEmailOrFullNameIsNull_ThenShouldNotPublishCancelOrderCommand_email=null_fullName=null.verified.json (1)
1-7: Align monetary serialization to two-decimal precision
"TotalMoney": 100.0deviates from the250.00/200.00format used elsewhere. Update the snapshot (or, preferably, introduce a custom JSON converter for theMoneyvalue object) so every amount serializes with exactly two fractional digits.- "TotalMoney": 100.0, + "TotalMoney": 100.00,src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/PlaceOrderConsumerTests.GivenPlaceOrderCommandWithEmptyEmail_WhenHandling_ThenShouldNotSendEmail.verified.json (1)
1-1: Strip the UTF-8 BOM to avoid noisy diffsThe leading “” (U+FEFF) means the file was saved with a BOM. Our snapshot files are treated as pure data and the BOM has repeatedly caused false-positive diffs on Windows vs *nix CI agents.
-{ +{src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/CleanUpSentEmailConsumerTests.GivenSentEmailsExist_WhenHandlingCleanUpEvent_ThenShouldDeleteEmailsAndSaveChanges.verified.json (2)
1-1: Remove UTF-8 BOMSame BOM issue as noted in the previous snapshot.
2-4: Snapshot may be too narrowOnly
IdandCreationDateare captured. If the consumer mutates additional fields (e.g.,DeletionDate,IsDeleted), the test will miss regressions.Consider expanding the snapshot to include every column that represents the persisted state after cleanup.
src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/CompleteOrderConsumerTests.GivenValidCompleteOrderCommand_WhenHandling_ThenShouldSendEmail.verified.json (1)
1-1: Remove UTF-8 BOMApplies here as well.
src/Services/Finance/BookWorm.Finance.UnitTests/Snapshots/OrderStateMachineTests.GivenOrderTimeout_WhenRetryCountReachesMax_ThenShouldTransitionToFailedAndCancel_OutputCommand.verified.json (2)
1-1: Remove UTF-8 BOMConsistent with the other snapshot files.
2-8: Placeholder email differs from other snapshotsHere the address is
[email protected]; elsewhere we use[email protected]. Using a single canonical test address avoids accidental semantic differences when snapshots are programmatically compared.Recommend normalising to a single value across all snapshots.
src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/PlaceOrderConsumerTests.GivenValidPlaceOrderCommand_WhenHandling_ThenShouldSendEmail.verified.json (1)
1-1: Remove UTF-8 BOMSame comment as for the other snapshot files.
src/Services/Finance/BookWorm.Finance.UnitTests/Snapshots/OrderStateMachineTests.GivenOrderTimeout_WhenRetryCountLessThanMax_ThenShouldRetryOrderProcessing_OutputCommand.verified.json (2)
1-1: Strip UTF-8 BOM to minimise diff noiseThe snapshot file is saved with a BOM (
). Verify’s diff engine sometimes treats this as content, creating unnecessary churn. Saving as plain UTF-8 avoids that headache.
2-4: Switch to IANA-reserved example domain
[email protected]is not an officially reserved test domain and could, in theory, reach a real tenant one day.
Using[email protected]or[email protected]keeps us safely within RFC 2606 guidance.src/Services/Chat/BookWorm.Chat.UnitTests/Features/Cancel/CancelChatCommandTests.cs (1)
244-252: Avoid double-await for cleaner concurrent testYou
await Task.WhenAll(task1, task2)and then immediatelyawaitboth tasks again when asserting. The second await is redundant because both tasks are already completed.- await Task.WhenAll(task1, task2); - - (await task1).ShouldBe(Unit.Value); - (await task2).ShouldBe(Unit.Value); + var (result1, result2) = await Task.WhenAll(task1, task2) switch + { + [var r1, var r2] => (r1, r2) + }; + + result1.ShouldBe(Unit.Value); + result2.ShouldBe(Unit.Value);This keeps the AAA pattern intact and removes unnecessary awaits.
src/Services/Notification/BookWorm.Notification.UnitTests/Snapshots/PlaceOrderConsumerTests.GivenPlaceOrderCommandWithNullEmail_WhenHandling_ThenShouldNotSendEmail.verified.json (1)
2-7: Snapshot structure looks correct – only nit is money precision.
TotalMoneyis serialized as99.99. Across the code-base adecimalis often rendered with 2 or 4 fractional digits (99.9900). If a fixed formatting scrubber is already active this is harmless, otherwise the snapshot may churn when regional settings differ orJsonSerializerOptionschange.src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CleanUpSentEmailConsumerTests.cs (1)
88-89: Good deterministic contract verification!The contract verification correctly leverages the parameterless nature of
CleanUpSentEmailIntegrationEventfor deterministic testing. The explanatory comment adds clarity.Consider adding contract verification to all test methods for comprehensive coverage:
// Add this pattern to other test methods that don't have it yet + // Contract verification - CleanUpSentEmailIntegrationEvent has no parameters so it's deterministic + await VerifySnapshot(command);src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/ResendErrorEmailConsumerTests.cs (1)
151-153: Good contract verification with helpful documentation.The snapshot verification correctly captures the event contract. The comment explaining that
ResendErrorEmailIntegrationEventis parameterless and deterministic is valuable for maintainability.Consider moving this verification to match the placement in the first test method (after command creation, before test execution) for consistency across test methods.
src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs (1)
73-80: Consider extracting snapshot verification to reduce duplication.The snapshot verification logic is duplicated across all three test methods with only minor variations in the email property. This violates the DRY principle and increases maintenance overhead.
Extract the common snapshot verification logic into a private helper method:
+ private async Task VerifyContractSnapshot(string? email) + { + var contractCommand = new CancelOrderCommand( + Guid.CreateVersion7(), + "John Doe", + email, + 99.99m + ); + await VerifySnapshot(contractCommand); + } // In each test method, replace the snapshot verification block with: - // Contract verification - using deterministic data for snapshot consistency - var contractCommand = new CancelOrderCommand( - Guid.CreateVersion7(), // OrderId - "John Doe", // FullName - "[email protected]", // Email - 99.99m // TotalMoney - ); - await VerifySnapshot(contractCommand); + await VerifyContractSnapshot("[email protected]"); // or null, or ""Also applies to: 113-120, 153-160
src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs (1)
66-73: Reduce code duplication in snapshot verification.The same duplication pattern exists here as in
CancelOrderConsumerTests. The snapshot verification logic should be extracted to a helper method for better maintainability.Create a helper method similar to the previous suggestion:
+ private async Task VerifyContractSnapshot(string? email) + { + var contractCommand = new CompleteOrderCommand( + Guid.CreateVersion7(), + "John Doe", + email, + 99.99m + ); + await VerifySnapshot(contractCommand); + }Then replace each snapshot verification block with
await VerifyContractSnapshot(emailValue);Also applies to: 106-113, 146-153
src/Services/Catalog/BookWorm.Catalog.UnitTests/Consumers/FeedbackCreatedConsumerTests.cs (1)
66-87: Comprehensive contract verification implementation.The snapshot verification captures essential contract information including properties, types, and metadata. The structure effectively ensures consumer contract compatibility.
Consider consolidating the Properties and Schema sections if they become repetitive across many tests, potentially through a helper method in SnapshotTestBase.
src/Services/Finance/BookWorm.Finance.UnitTests/OrderStateMachineTests.cs (1)
621-622: Note: Simpler snapshot pattern for timeout events.The direct object snapshotting is more concise than the comprehensive pattern used elsewhere. While this creates some inconsistency, it may be appropriate for simpler timeout events.
Consider maintaining consistency with the established Input/Output pattern if timeout events require the same level of contract verification detail.
Test Results949 tests +3 949 ✅ +3 10m 41s ⏱️ +55s Results for commit 05019ec. ± Comparison against base commit 2397fff. This pull request removes 3 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
4d2cfb9 to
05019ec
Compare
|
Pull Request Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores