feat(s3): infrastructure persistence tests — 21 adapter ITs (STA-123)#125
Merged
Merged
Conversation
CollectionOrderPersistenceAdapterIT (10 tests): CRUD, findByPaymentId, unique constraint, full happy path, refund path, failure path, amount mismatch + manual review, nullable fields. RefundPersistenceAdapterIT (6 tests): CRUD, findByCollectionId, completion path, failure path. PspTransactionPersistenceAdapterIT (5 tests): CRUD, append-only, multiple txns, JSONB round-trip, direction enum values. Closes STA-123 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThree new integration test classes are added to verify persistence layer behavior for CollectionOrder, PspTransaction, and Refund domain models. Tests validate CRUD operations, state machine transitions, constraint enforcement (unique paymentId), JSON/enum serialization round-trips, and null field handling using Spring repositories and AssertJ assertions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/PspTransactionPersistenceAdapterIT.java`:
- Around line 101-115: The test shouldPersistAllDirectionEnumValues currently
only checks row count; update it to verify the persisted direction values
round-trip correctly by fetching transactions via
adapter.findByCollectionId(order.collectionId()), mapping each returned
PspTransaction's direction (e.g., txn.direction() or getDirection()) into a set,
and asserting that this set equals EnumSet.allOf(PspTransactionDirection.class)
(or the list of PspTransactionDirection.values()), ensuring every enum constant
was persisted distinctly; use the existing PspTransaction.create and
PspTransactionDirection symbols to build the expected values and compare against
the persisted directions.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/RefundPersistenceAdapterIT.java`:
- Around line 49-63: The test should verify the actual refunds returned by
adapter.findByCollectionId(...) rather than only the count: after saving
refund1/refund2 (saved1, saved2) in shouldFindByCollectionId, assert that
results contains those saved entities (e.g., compare IDs from
saved1.getId()/saved2.getId() or use object equality) and optionally validate
key fields (collectionId/paymentId/amount) to prove proper filtering; update the
assertion on results to check presence/equality of saved1 and saved2 instead of
only hasSize(2).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76830086-cb62-4a59-a2bc-d2db27240ba3
📒 Files selected for processing (3)
fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/CollectionOrderPersistenceAdapterIT.javafiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/PspTransactionPersistenceAdapterIT.javafiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/RefundPersistenceAdapterIT.java
Comment on lines
+101
to
+115
| @Test | ||
| @DisplayName("should persist all direction enum values") | ||
| void shouldPersistAllDirectionEnumValues() { | ||
| var order = saveCollectionOrder(); | ||
|
|
||
| for (var direction : PspTransactionDirection.values()) { | ||
| var txn = PspTransaction.create( | ||
| order.collectionId(), "Stripe", "ref_" + direction.name(), | ||
| direction, "event_" + direction.name(), aMoney(), | ||
| "completed", "{}"); | ||
| adapter.save(txn); | ||
| } | ||
|
|
||
| assertThat(adapter.findByCollectionId(order.collectionId())) | ||
| .hasSize(PspTransactionDirection.values().length); |
There was a problem hiding this comment.
Assert persisted directions, not only row count.
Line 114 only proves inserts happened. If the enum mapping collapses every value to the same persisted value, this test still passes. Extract the direction field so the round-trip is actually covered.
Patch
- assertThat(adapter.findByCollectionId(order.collectionId()))
- .hasSize(PspTransactionDirection.values().length);
+ assertThat(adapter.findByCollectionId(order.collectionId()))
+ .extracting(PspTransaction::direction)
+ .containsExactlyInAnyOrder(PspTransactionDirection.values());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/PspTransactionPersistenceAdapterIT.java`
around lines 101 - 115, The test shouldPersistAllDirectionEnumValues currently
only checks row count; update it to verify the persisted direction values
round-trip correctly by fetching transactions via
adapter.findByCollectionId(order.collectionId()), mapping each returned
PspTransaction's direction (e.g., txn.direction() or getDirection()) into a set,
and asserting that this set equals EnumSet.allOf(PspTransactionDirection.class)
(or the list of PspTransactionDirection.values()), ensuring every enum constant
was persisted distinctly; use the existing PspTransaction.create and
PspTransactionDirection symbols to build the expected values and compare against
the persisted directions.
Comment on lines
+49
to
+63
| @Test | ||
| @DisplayName("should find by collection id") | ||
| void shouldFindByCollectionId() { | ||
| var order = saveCollectionOrder(); | ||
| var refund1 = Refund.initiate(order.collectionId(), order.paymentId(), aRefundAmount(), REFUND_REASON); | ||
| var saved1 = adapter.save(refund1); | ||
|
|
||
| var refund2 = Refund.initiate( | ||
| order.collectionId(), order.paymentId(), | ||
| new Money(new BigDecimal("500.00"), "USD"), "Partial refund"); | ||
| var saved2 = adapter.save(refund2); | ||
|
|
||
| var results = adapter.findByCollectionId(order.collectionId()); | ||
| assertThat(results).hasSize(2); | ||
| } |
There was a problem hiding this comment.
Assert the returned refunds, not just the count.
Line 62 only validates cardinality. A broken repository predicate that returns any two refunds would still pass. Use the saved IDs or full object comparison so this test proves findByCollectionId(...) is filtering correctly.
Patch
- var results = adapter.findByCollectionId(order.collectionId());
- assertThat(results).hasSize(2);
+ assertThat(adapter.findByCollectionId(order.collectionId()))
+ .extracting(Refund::refundId)
+ .containsExactlyInAnyOrder(saved1.refundId(), saved2.refundId());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/infrastructure/persistence/RefundPersistenceAdapterIT.java`
around lines 49 - 63, The test should verify the actual refunds returned by
adapter.findByCollectionId(...) rather than only the count: after saving
refund1/refund2 (saved1, saved2) in shouldFindByCollectionId, assert that
results contains those saved entities (e.g., compare IDs from
saved1.getId()/saved2.getId() or use object equality) and optionally validate
key fields (collectionId/paymentId/amount) to prove proper filtering; update the
assertion on results to check presence/equality of saved1 and saved2 instead of
only hasSize(2).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Total: 235 tests (214 unit + 21 IT)
Test plan
Closes STA-123
🤖 Generated with Claude Code
Summary by CodeRabbit