feat(s4): domain model unit tests — ChainTransfer, WalletBalance, VOs (STA-133)#122
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughAdds extensive unit tests and test fixtures for custody domain models, covering construction, validation, state transitions, equality, immutability, and defensive copying across multiple domain types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainConfigTest.java`:
- Line 1: The test file ChainConfigTest has Spotless formatting violations; run
the formatter and commit the changes by executing ./gradlew spotlessApply (or
apply your IDE's Spotless rules) to update imports/spacing/line endings, then
re-run ./gradlew spotlessCheck; ensure the updated formatting for class
ChainConfigTest is staged and committed so the pipeline passes.
- Around line 26-35: The test creates two identical ChainConfig instances and
compares them tautologically; update the createsChainConfig test to assert each
field via ChainConfig's accessors instead of comparing two constructed objects.
Locate the createsChainConfig method in ChainConfigTest and replace the
recursive comparison with assertions that verify the values passed to the
ChainConfig constructor (e.g., the chain identifier constant BASE, numeric
values 12 and 2, the native symbol "ETH", RPC_ENDPOINTS, and the explorer URL
"https://basescan.org") by calling the corresponding getters on the result
object (the ChainConfig instance) to ensure each field is populated correctly.
- Around line 157-165: The test endpointsDefensivelyCopied currently only
asserts the returned list from config.rpcEndpoints() is unmodifiable; extend it
to also verify the constructor defensively copied the input by mutating the
original mutableList after creating new ChainConfig(BASE, 12, 2, "ETH",
mutableList, null) (e.g., add/remove an element from mutableList) and then
asserting config.rpcEndpoints() still contains only the original RPC and was not
affected by the mutation; keep assertions using config.rpcEndpoints() and the
same test method.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainIdTest.java`:
- Line 1: The file ChainIdTest has Spotless formatting violations; run the
formatter and apply fixes by running ./gradlew spotlessApply (or invoke your
IDE's Java formatter) and then reformat the ChainIdTest class and any affected
files so they comply with spotless rules; ensure imports, spacing, and line
endings in the ChainIdTest class (and surrounding domain.model classes if
flagged) are corrected before committing.
- Around line 20-67: The seven near-identical tests (acceptsEthereum,
acceptsSolana, acceptsStellar, acceptsTron, acceptsBase, acceptsPolygon,
acceptsAvalanche) should be consolidated into a parameterized test to reduce
duplication: replace these individual `@Test` methods with a single
`@ParameterizedTest` using `@ValueSource`(strings =
{"ethereum","solana","stellar","tron","base","polygon","avalanche"}) and a
single test method (e.g., acceptsSupportedChain) that constructs new
ChainId(chain) and asserts chainId.value().isEqualTo(chain); keep the
DisplayName to describe supported chains.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainTransferTest.java`:
- Around line 840-860: Refactor the loop-based tests
canApplyReturnsFalseForConfirmed and canApplyReturnsFalseForFailed to use
`@ParameterizedTest` with a `@MethodSource` that supplies each TransferTrigger
(similar to the InvalidTransitions pattern); replace the for-loop over
TransferTrigger.values() with a single-parameter test method that takes a
TransferTrigger and asserts that aConfirmedTransfer().canApply(trigger) (or
aFailedTransfer().canApply(trigger)) is false, and add a descriptive display
name or assertion message that includes the trigger for clarity.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/TransferLifecycleEventTest.java`:
- Around line 23-31: The test currently compares two events created by the same
factory call (TransferLifecycleEvent.record) which is tautological; update the
tests (createsEvent and createsEventWithParticipant) to assert individual
accessors instead of comparing to an identically-constructed expected: check
transferId() equals TRANSFER_ID, state() equals "PENDING", eventId() is not
null, occurredAt() is not null, and for the createsEventWithParticipant test
also assert participant() equals the expected participant value; reference the
TransferLifecycleEvent.record factory and the TransferLifecycleEvent accessor
methods to locate and change the assertions.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/TransferParticipantTest.java`:
- Around line 56-71: The test createsFeeParticipant currently compares two
identical TransferParticipant.create calls (result vs expected), which is
tautological; update the test method createsFeeParticipant in
TransferParticipantTest to assert actual field values on the produced object
instead: call TransferParticipant.create(...) once into result, then assert
result.getTransferId(), result.getType() (or equivalent enum/value),
result.getAddress(), result.getToAddress() (null), result.getAmount() (using
BigDecimal comparison), and result.getCurrency() match the expected literals,
and assert result.getParticipantId() is not null (or matches the expected
generation rule) while still using BigDecimal::compareTo for amount comparison
and ignoring participantId only where appropriate.
- Around line 29-44: The test currently compares two identical calls to
TransferParticipant.create (result and expected) which is tautological; change
it to assert the domain fields on the created object directly: call
TransferParticipant.create(...) to get result, then assert
result.getTransferId(), result.getInput() (or the appropriate accessor names),
result.getAddress(), result.getWalletId(), and result.getAssetCode() equal the
expected constants and compare result.getAmount() using BigDecimal::compareTo;
you can still ignore or assert that participantId is non-null if desired, but
remove the duplicate expected creation and replace it with explicit assertions
against the known constants.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletBalanceTest.java`:
- Around line 26-35: Tests in WalletBalanceTest use tautological comparisons
(e.g., comparing result to an expected object built by applying the same
operations like aBalanceWith(...).reserve(...)), which only verifies
determinism; update those test cases (including createsZeroBalance and the
ranges referenced: 68-112, 161-189, 236-266, 305-348, 392-403) to assert
concrete field values instead: call the operation under test (e.g.,
WalletBalance.reserve(...), WalletBalance.release(...), etc.) and then assert on
accessors such as availableBalance() and reservedBalance() using assertions like
isEqualByComparingTo(new BigDecimal("...")) rather than building an expected by
replaying the same operations (remove uses of aBalanceWith(...).reserve(...) as
the expected).
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletTest.java`:
- Around line 29-40: The test WalletTest::createsActiveWallet is tautological
because anActiveWallet() returns Wallet.create(...) with the same constants;
update the test to assert individual fields instead of comparing two
factory-created objects. Replace the recursive comparison between
anActiveWallet() and Wallet.create(...) with assertions on the returned Wallet's
properties (e.g., wallet.getChainBase(), getAddress(), getAddressChecksum(),
getTier(), getPurpose(), getCustodian(), getVaultAccountId(), getCurrency()) and
assert non-null/expected behavior for generated fields (walletId, createdAt) as
appropriate; keep references to anActiveWallet() and Wallet.create(...) to
locate the code to change.
- Around line 179-191: The test setsActiveToFalse in WalletTest calls
deactivate() but ignores deactivatedAt in the recursive comparison; add an
explicit assertion that the returned wallet's deactivatedAt is populated (e.g.
assertThat(result.getDeactivatedAt()).isNotNull() or equivalent) after calling
deactivate() on the result to satisfy the DisplayName and verify deactivatedAt
is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2ad1976-b7ac-414b-9c0e-0c7c2a1fce9a
📒 Files selected for processing (11)
blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainConfigTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainIdTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainTransferTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/StablecoinTickerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/TransferLifecycleEventTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/TransferParticipantTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletBalanceTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/ChainTransferFixtures.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/WalletBalanceFixtures.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/WalletFixtures.java
Comment on lines
+26
to
+35
| void createsChainConfig() { | ||
| var result = new ChainConfig( | ||
| BASE, 12, 2, "ETH", RPC_ENDPOINTS, "https://basescan.org" | ||
| ); | ||
|
|
||
| var expected = new ChainConfig( | ||
| BASE, 12, 2, "ETH", RPC_ENDPOINTS, "https://basescan.org" | ||
| ); | ||
| assertThat(result).usingRecursiveComparison().isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tautological test — comparing identically-constructed objects.
Same issue as TransferParticipantTest. Assert on individual accessors to verify field population.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainConfigTest.java`
around lines 26 - 35, The test creates two identical ChainConfig instances and
compares them tautologically; update the createsChainConfig test to assert each
field via ChainConfig's accessors instead of comparing two constructed objects.
Locate the createsChainConfig method in ChainConfigTest and replace the
recursive comparison with assertions that verify the values passed to the
ChainConfig constructor (e.g., the chain identifier constant BASE, numeric
values 12 and 2, the native symbol "ETH", RPC_ENDPOINTS, and the explorer URL
"https://basescan.org") by calling the corresponding getters on the result
object (the ChainConfig instance) to ensure each field is populated correctly.
Comment on lines
+157
to
+165
| @Test | ||
| @DisplayName("rpcEndpoints are defensively copied") | ||
| void endpointsDefensivelyCopied() { | ||
| var mutableList = new ArrayList<>(List.of("https://rpc.base.org")); | ||
| var config = new ChainConfig(BASE, 12, 2, "ETH", mutableList, null); | ||
|
|
||
| assertThatThrownBy(() -> config.rpcEndpoints().add("https://evil.com")) | ||
| .isInstanceOf(UnsupportedOperationException.class); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive copy test is incomplete.
This only verifies the returned list is unmodifiable. A proper defensive copy test should also verify that mutating the input list after construction doesn't affect the internal state.
Suggested enhancement
`@Test`
- `@DisplayName`("rpcEndpoints are defensively copied")
- void endpointsDefensivelyCopied() {
+ `@DisplayName`("rpcEndpoints are defensively copied on input")
+ void inputMutationDoesNotAffectConfig() {
var mutableList = new ArrayList<>(List.of("https://rpc.base.org"));
var config = new ChainConfig(BASE, 12, 2, "ETH", mutableList, null);
+
+ mutableList.add("https://malicious.com");
- assertThatThrownBy(() -> config.rpcEndpoints().add("https://evil.com"))
- .isInstanceOf(UnsupportedOperationException.class);
+ assertThat(config.rpcEndpoints()).containsExactly("https://rpc.base.org");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| @DisplayName("rpcEndpoints are defensively copied") | |
| void endpointsDefensivelyCopied() { | |
| var mutableList = new ArrayList<>(List.of("https://rpc.base.org")); | |
| var config = new ChainConfig(BASE, 12, 2, "ETH", mutableList, null); | |
| assertThatThrownBy(() -> config.rpcEndpoints().add("https://evil.com")) | |
| .isInstanceOf(UnsupportedOperationException.class); | |
| } | |
| `@Test` | |
| `@DisplayName`("rpcEndpoints are defensively copied on input") | |
| void inputMutationDoesNotAffectConfig() { | |
| var mutableList = new ArrayList<>(List.of("https://rpc.base.org")); | |
| var config = new ChainConfig(BASE, 12, 2, "ETH", mutableList, null); | |
| mutableList.add("https://malicious.com"); | |
| assertThat(config.rpcEndpoints()).containsExactly("https://rpc.base.org"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainConfigTest.java`
around lines 157 - 165, The test endpointsDefensivelyCopied currently only
asserts the returned list from config.rpcEndpoints() is unmodifiable; extend it
to also verify the constructor defensively copied the input by mutating the
original mutableList after creating new ChainConfig(BASE, 12, 2, "ETH",
mutableList, null) (e.g., add/remove an element from mutableList) and then
asserting config.rpcEndpoints() still contains only the original RPC and was not
affected by the mutation; keep assertions using config.rpcEndpoints() and the
same test method.
| @@ -0,0 +1,124 @@ | |||
| package com.stablecoin.payments.custody.domain.model; | |||
There was a problem hiding this comment.
Fix Spotless formatting violations.
Pipeline failed with spotlessJavaCheck. Run:
./gradlew spotlessApply🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: spotlessJavaCheck failed. Formatting violations detected in src/test/java/com/stablecoin/payments/custody/domain/model/ChainIdTest.java. Run './gradlew spotlessApply' to fix code style issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainIdTest.java`
at line 1, The file ChainIdTest has Spotless formatting violations; run the
formatter and apply fixes by running ./gradlew spotlessApply (or invoke your
IDE's Java formatter) and then reformat the ChainIdTest class and any affected
files so they comply with spotless rules; ensure imports, spacing, and line
endings in the ChainIdTest class (and surrounding domain.model classes if
flagged) are corrected before committing.
Comment on lines
+20
to
+67
| @Test | ||
| @DisplayName("accepts 'ethereum'") | ||
| void acceptsEthereum() { | ||
| var chainId = new ChainId("ethereum"); | ||
| assertThat(chainId.value()).isEqualTo("ethereum"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("accepts 'solana'") | ||
| void acceptsSolana() { | ||
| var chainId = new ChainId("solana"); | ||
| assertThat(chainId.value()).isEqualTo("solana"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("accepts 'stellar'") | ||
| void acceptsStellar() { | ||
| var chainId = new ChainId("stellar"); | ||
| assertThat(chainId.value()).isEqualTo("stellar"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("accepts 'tron'") | ||
| void acceptsTron() { | ||
| var chainId = new ChainId("tron"); | ||
| assertThat(chainId.value()).isEqualTo("tron"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("accepts 'base'") | ||
| void acceptsBase() { | ||
| var chainId = new ChainId("base"); | ||
| assertThat(chainId.value()).isEqualTo("base"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("accepts 'polygon'") | ||
| void acceptsPolygon() { | ||
| var chainId = new ChainId("polygon"); | ||
| assertThat(chainId.value()).isEqualTo("polygon"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("accepts 'avalanche'") | ||
| void acceptsAvalanche() { | ||
| var chainId = new ChainId("avalanche"); | ||
| assertThat(chainId.value()).isEqualTo("avalanche"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parameterized test for valid chains.
Seven near-identical tests could be consolidated:
Suggested refactor
`@ParameterizedTest`
`@ValueSource`(strings = {"ethereum", "solana", "stellar", "tron", "base", "polygon", "avalanche"})
`@DisplayName`("accepts supported chain")
void acceptsSupportedChain(String chain) {
var chainId = new ChainId(chain);
assertThat(chainId.value()).isEqualTo(chain);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/ChainIdTest.java`
around lines 20 - 67, The seven near-identical tests (acceptsEthereum,
acceptsSolana, acceptsStellar, acceptsTron, acceptsBase, acceptsPolygon,
acceptsAvalanche) should be consolidated into a parameterized test to reduce
duplication: replace these individual `@Test` methods with a single
`@ParameterizedTest` using `@ValueSource`(strings =
{"ethereum","solana","stellar","tron","base","polygon","avalanche"}) and a
single test method (e.g., acceptsSupportedChain) that constructs new
ChainId(chain) and asserts chainId.value().isEqualTo(chain); keep the
DisplayName to describe supported chains.
Comment on lines
+29
to
+44
| @Test | ||
| @DisplayName("creates participant with correct fields") | ||
| void createsParticipant() { | ||
| var result = TransferParticipant.create( | ||
| TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE | ||
| ); | ||
|
|
||
| var expected = TransferParticipant.create( | ||
| TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE | ||
| ); | ||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFields("participantId") | ||
| .isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tautological test — comparing identically-constructed objects.
Both result and expected are created with the same factory call and parameters, so the assertion only confirms the factory is deterministic. This doesn't verify that the domain fields (transferId, address, amount, etc.) are correctly populated.
Assert on individual field accessors to validate actual behavior:
Suggested fix
`@Test`
`@DisplayName`("creates participant with correct fields")
void createsParticipant() {
var result = TransferParticipant.create(
TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE
);
- var expected = TransferParticipant.create(
- TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE
- );
- assertThat(result)
- .usingRecursiveComparison()
- .withComparatorForType(BigDecimal::compareTo, BigDecimal.class)
- .ignoringFields("participantId")
- .isEqualTo(expected);
+ assertThat(result.transferId()).isEqualTo(TRANSFER_ID);
+ assertThat(result.participantType()).isEqualTo(INPUT);
+ assertThat(result.address()).isEqualTo(ADDRESS);
+ assertThat(result.walletId()).isEqualTo(WALLET_ID);
+ assertThat(result.amount()).isEqualByComparingTo(AMOUNT);
+ assertThat(result.assetCode()).isEqualTo(ASSET_CODE);
+ assertThat(result.participantId()).isNotNull();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| @DisplayName("creates participant with correct fields") | |
| void createsParticipant() { | |
| var result = TransferParticipant.create( | |
| TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE | |
| ); | |
| var expected = TransferParticipant.create( | |
| TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE | |
| ); | |
| assertThat(result) | |
| .usingRecursiveComparison() | |
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | |
| .ignoringFields("participantId") | |
| .isEqualTo(expected); | |
| } | |
| `@Test` | |
| `@DisplayName`("creates participant with correct fields") | |
| void createsParticipant() { | |
| var result = TransferParticipant.create( | |
| TRANSFER_ID, INPUT, ADDRESS, WALLET_ID, AMOUNT, ASSET_CODE | |
| ); | |
| assertThat(result.transferId()).isEqualTo(TRANSFER_ID); | |
| assertThat(result.participantType()).isEqualTo(INPUT); | |
| assertThat(result.address()).isEqualTo(ADDRESS); | |
| assertThat(result.walletId()).isEqualTo(WALLET_ID); | |
| assertThat(result.amount()).isEqualByComparingTo(AMOUNT); | |
| assertThat(result.assetCode()).isEqualTo(ASSET_CODE); | |
| assertThat(result.participantId()).isNotNull(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/TransferParticipantTest.java`
around lines 29 - 44, The test currently compares two identical calls to
TransferParticipant.create (result and expected) which is tautological; change
it to assert the domain fields on the created object directly: call
TransferParticipant.create(...) to get result, then assert
result.getTransferId(), result.getInput() (or the appropriate accessor names),
result.getAddress(), result.getWalletId(), and result.getAssetCode() equal the
expected constants and compare result.getAmount() using BigDecimal::compareTo;
you can still ignore or assert that participantId is non-null if desired, but
remove the duplicate expected creation and replace it with explicit assertions
against the known constants.
Comment on lines
+56
to
+71
| @Test | ||
| @DisplayName("creates FEE participant") | ||
| void createsFeeParticipant() { | ||
| var result = TransferParticipant.create( | ||
| TRANSFER_ID, FEE, ADDRESS, null, new BigDecimal("0.50"), "ETH" | ||
| ); | ||
|
|
||
| var expected = TransferParticipant.create( | ||
| TRANSFER_ID, FEE, ADDRESS, null, new BigDecimal("0.50"), "ETH" | ||
| ); | ||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFields("participantId") | ||
| .isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same tautological pattern for FEE participant.
Consider asserting actual field values instead of comparing two identically-created instances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/TransferParticipantTest.java`
around lines 56 - 71, The test createsFeeParticipant currently compares two
identical TransferParticipant.create calls (result vs expected), which is
tautological; update the test method createsFeeParticipant in
TransferParticipantTest to assert actual field values on the produced object
instead: call TransferParticipant.create(...) once into result, then assert
result.getTransferId(), result.getType() (or equivalent enum/value),
result.getAddress(), result.getToAddress() (null), result.getAmount() (using
BigDecimal comparison), and result.getCurrency() match the expected literals,
and assert result.getParticipantId() is not null (or matches the expected
generation rule) while still using BigDecimal::compareTo for amount comparison
and ignoring participantId only where appropriate.
Comment on lines
+26
to
+35
| void createsZeroBalance() { | ||
| var result = aZeroBalance(); | ||
|
|
||
| var expected = WalletBalance.initialize(WALLET_ID, CHAIN_BASE, USDC); | ||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFields("balanceId", "updatedAt") | ||
| .isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Systemic pattern: tautological assertions throughout.
Many tests compare result against expected created with the same operations, e.g.:
var result = balance.reserve(new BigDecimal("200.00"));
var expected = aBalanceWith(...).reserve(new BigDecimal("200.00"));
assertThat(result)...isEqualTo(expected);This only proves determinism, not correctness. For balance operations, assert on actual field values:
var result = balance.reserve(new BigDecimal("200.00"));
assertThat(result.availableBalance()).isEqualByComparingTo(new BigDecimal("300.00"));
assertThat(result.reservedBalance()).isEqualByComparingTo(new BigDecimal("200.00"));Also applies to: 68-112, 161-189, 236-266, 305-348, 392-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletBalanceTest.java`
around lines 26 - 35, Tests in WalletBalanceTest use tautological comparisons
(e.g., comparing result to an expected object built by applying the same
operations like aBalanceWith(...).reserve(...)), which only verifies
determinism; update those test cases (including createsZeroBalance and the
ranges referenced: 68-112, 161-189, 236-266, 305-348, 392-403) to assert
concrete field values instead: call the operation under test (e.g.,
WalletBalance.reserve(...), WalletBalance.release(...), etc.) and then assert on
accessors such as availableBalance() and reservedBalance() using assertions like
isEqualByComparingTo(new BigDecimal("...")) rather than building an expected by
replaying the same operations (remove uses of aBalanceWith(...).reserve(...) as
the expected).
Comment on lines
+29
to
+40
| void createsActiveWallet() { | ||
| var result = anActiveWallet(); | ||
|
|
||
| var expected = Wallet.create( | ||
| CHAIN_BASE, ADDRESS, ADDRESS_CHECKSUM, | ||
| TIER_HOT, PURPOSE_ON_RAMP, CUSTODIAN, VAULT_ACCOUNT_ID, USDC | ||
| ); | ||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .ignoringFields("walletId", "createdAt") | ||
| .isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tautological test — anActiveWallet() returns Wallet.create() with same constants.
Looking at WalletFixtures.anActiveWallet(), it calls Wallet.create() with the same constants. This test compares two identical factory calls.
Assert on individual field values to validate the factory behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletTest.java`
around lines 29 - 40, The test WalletTest::createsActiveWallet is tautological
because anActiveWallet() returns Wallet.create(...) with the same constants;
update the test to assert individual fields instead of comparing two
factory-created objects. Replace the recursive comparison between
anActiveWallet() and Wallet.create(...) with assertions on the returned Wallet's
properties (e.g., wallet.getChainBase(), getAddress(), getAddressChecksum(),
getTier(), getPurpose(), getCustodian(), getVaultAccountId(), getCurrency()) and
assert non-null/expected behavior for generated fields (walletId, createdAt) as
appropriate; keep references to anActiveWallet() and Wallet.create(...) to
locate the code to change.
Comment on lines
+179
to
+191
| @Test | ||
| @DisplayName("sets active to false and deactivatedAt") | ||
| void setsActiveToFalse() { | ||
| var active = anActiveWallet(); | ||
|
|
||
| var result = active.deactivate(); | ||
|
|
||
| var expected = anActiveWallet().deactivate(); | ||
| assertThat(result) | ||
| .usingRecursiveComparison() | ||
| .ignoringFields("walletId", "createdAt", "deactivatedAt") | ||
| .isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
Missing assertion: verify deactivatedAt is populated.
The test ignores deactivatedAt in comparison but the DisplayName says "sets active to false and deactivatedAt". Add an assertion to verify deactivatedAt is non-null:
Suggested fix
`@Test`
`@DisplayName`("sets active to false and deactivatedAt")
void setsActiveToFalse() {
var active = anActiveWallet();
var result = active.deactivate();
- var expected = anActiveWallet().deactivate();
- assertThat(result)
- .usingRecursiveComparison()
- .ignoringFields("walletId", "createdAt", "deactivatedAt")
- .isEqualTo(expected);
+ assertThat(result.active()).isFalse();
+ assertThat(result.deactivatedAt()).isNotNull();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| @DisplayName("sets active to false and deactivatedAt") | |
| void setsActiveToFalse() { | |
| var active = anActiveWallet(); | |
| var result = active.deactivate(); | |
| var expected = anActiveWallet().deactivate(); | |
| assertThat(result) | |
| .usingRecursiveComparison() | |
| .ignoringFields("walletId", "createdAt", "deactivatedAt") | |
| .isEqualTo(expected); | |
| } | |
| `@Test` | |
| `@DisplayName`("sets active to false and deactivatedAt") | |
| void setsActiveToFalse() { | |
| var active = anActiveWallet(); | |
| var result = active.deactivate(); | |
| assertThat(result.active()).isFalse(); | |
| assertThat(result.deactivatedAt()).isNotNull(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/model/WalletTest.java`
around lines 179 - 191, The test setsActiveToFalse in WalletTest calls
deactivate() but ignores deactivatedAt in the recursive comparison; add an
explicit assertion that the returned wallet's deactivatedAt is populated (e.g.
assertThat(result.getDeactivatedAt()).isNotNull() or equivalent) after calling
deactivate() on the result to satisfy the DisplayName and verify deactivatedAt
is set.
…etBalance invariants, value objects (STA-133) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-133-s4-domain-unit-tests
branch
from
8c22c12 to
fa26ec9
Compare
5 tasks
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
Test Breakdown
Test plan
./gradlew :blockchain-custody:blockchain-custody:test— BUILD SUCCESSFULisEqualByComparingTo()orwithComparatorForTypeCloses STA-133
🤖 Generated with Claude Code
Summary by CodeRabbit