feat(s1): business tests — 5 E2E payment lifecycle flows (STA-116)#116
Merged
Conversation
Puneethkumarck
added
phase-2
WalkthroughAdds a business E2E test base and a comprehensive PaymentLifecycleTest exercising Temporal workflows, TestContainers-backed Postgres/Kafka, and mocked downstream services; gates Temporal client config with a property and corrects event-type labels in FX-failure paths. Changes
Sequence Diagram(s)mermaid 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java (1)
167-179:⚠️ Potential issue | 🟠 MajorInconsistent state labeling in FX failure events.
At line 148,
currentStateis set to"FX_LOCKING", yet failure events at lines 168 and 178 use"COMPLIANCE_CHECK". This creates:
- Semantic confusion: Event consumers see
COMPLIANCE_CHECKsource withFX_LOCK_ERROR/FX_LOCK_REJECTEDevent types- State tracking mismatch: Internal state says
FX_LOCKING, events sayCOMPLIANCE_CHECKIf
PaymentStateenum lacksFX_LOCKING, add it rather than reusing an unrelated state label.Proposed fix: Use consistent FX_LOCKING label
publishEvent(PaymentEventRequest.failed(request.paymentId(), - request.correlationId(), "COMPLIANCE_CHECK", reason, "FX_LOCK_ERROR")); + request.correlationId(), "FX_LOCKING", reason, "FX_LOCK_ERROR")); return PaymentResult.failed(request.paymentId(), reason); } if (fxResult.status() != FxLockResult.FxLockStatus.LOCKED) { currentState = "FAILED"; var reason = "FX rate lock failed: " + fxResult.failureReason(); log.info("FX lock rejected for paymentId={}: {}", request.paymentId(), fxResult.failureReason()); publishEvent(PaymentEventRequest.failed(request.paymentId(), - request.correlationId(), "COMPLIANCE_CHECK", reason, "FX_LOCK_REJECTED")); + request.correlationId(), "FX_LOCKING", reason, "FX_LOCK_REJECTED"));Then add
FX_LOCKINGtoPaymentStateenum and updateEventPublishingActivityImpl.mapToEvent()accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java` around lines 167 - 179, The FX failure event code in PaymentWorkflowImpl sets currentState = "FAILED" while publishing events with source "COMPLIANCE_CHECK", causing mismatch with the earlier currentState "FX_LOCKING"; update the flow to use a consistent FX_LOCKING state: add FX_LOCKING to the PaymentState enum (if missing) and ensure EventPublishingActivityImpl.mapToEvent() maps that new FX_LOCKING value to the correct event source, then change the PaymentEventRequest.failed calls (and any related uses in PaymentWorkflowImpl) to use the FX_LOCKING source instead of "COMPLIANCE_CHECK" so events and internal state align.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.java`:
- Around line 428-433: The extractPaymentId method currently does fragile manual
string parsing; replace it with a proper JSON parser (e.g., JsonPath or Jackson
ObjectMapper) to reliably extract the paymentId. Update the
extractPaymentId(String jsonResponse) implementation to parse jsonResponse (for
example call JsonPath.read(jsonResponse, "$.paymentId") or use new
ObjectMapper().readTree(jsonResponse).get("paymentId").asText()) and return that
value, ensuring it handles missing/null fields appropriately (throw or return
null) and add any necessary imports for JsonPath or
com.fasterxml.jackson.databind.ObjectMapper.
---
Outside diff comments:
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java`:
- Around line 167-179: The FX failure event code in PaymentWorkflowImpl sets
currentState = "FAILED" while publishing events with source "COMPLIANCE_CHECK",
causing mismatch with the earlier currentState "FX_LOCKING"; update the flow to
use a consistent FX_LOCKING state: add FX_LOCKING to the PaymentState enum (if
missing) and ensure EventPublishingActivityImpl.mapToEvent() maps that new
FX_LOCKING value to the correct event source, then change the
PaymentEventRequest.failed calls (and any related uses in PaymentWorkflowImpl)
to use the FX_LOCKING source instead of "COMPLIANCE_CHECK" so events and
internal state align.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c9dd813-91ce-4d10-b5e9-a7a146746f9c
📒 Files selected for processing (5)
payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/AbstractBusinessTest.javapayment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/config/TemporalConfig.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowTest.java
Comment on lines
+428
to
+433
| private static String extractPaymentId(String jsonResponse) { | ||
| var pattern = "\"paymentId\":\""; | ||
| var start = jsonResponse.indexOf(pattern) + pattern.length(); | ||
| var end = jsonResponse.indexOf("\"", start); | ||
| return jsonResponse.substring(start, end); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using JsonPath or ObjectMapper for JSON extraction.
Manual string parsing is fragile if JSON formatting changes (e.g., whitespace, field ordering).
Alternative using JsonPath
private static String extractPaymentId(String jsonResponse) {
return JsonPath.read(jsonResponse, "$.paymentId");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.java`
around lines 428 - 433, The extractPaymentId method currently does fragile
manual string parsing; replace it with a proper JSON parser (e.g., JsonPath or
Jackson ObjectMapper) to reliably extract the paymentId. Update the
extractPaymentId(String jsonResponse) implementation to parse jsonResponse (for
example call JsonPath.read(jsonResponse, "$.paymentId") or use new
ObjectMapper().readTree(jsonResponse).get("paymentId").asText()) and return that
value, ensuring it handles missing/null fields appropriately (throw or return
null) and add any necessary imports for JsonPath or
com.fasterxml.jackson.databind.ObjectMapper.
Add PaymentLifecycleTest with Temporal TestWorkflowEnvironment, real DB (TestContainers), and mocked S2/S6 Feign clients. Covers happy path, compliance rejection, FX lock failure, cancel-after-lock compensation, and idempotency. Fix latent bug: PaymentWorkflowImpl used invalid "FX_LOCKING" state string in failed event requests (no such PaymentState enum value), causing silent event publishing failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-116-s1-business-tests
branch
from
741102f to
c8789c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java (1)
167-179:⚠️ Potential issue | 🟠 MajorUse
FAILEDstate for FX lock failures, notCOMPLIANCE_CHECK.The event publishing uses
"COMPLIANCE_CHECK"as thefailedStateparameter for FX lock failures (lines 168, 178), but this is semantically misleading. ThePaymentStateenum containsFX_LOCKED(notFX_LOCKING), while the workflow code at line 148 attempts to use"FX_LOCKING". When FX lock fails, the payment transitions toFAILEDstate (see line 175); the event should reflect this final state rather than a previous checkpoint.Pass
"FAILED"as the failedState parameter to accurately represent the payment's terminal state:publishEvent(PaymentEventRequest.failed(request.paymentId(), request.correlationId(), "FAILED", reason, "FX_LOCK_ERROR"));This eliminates the disconnect between the actual state transition and the event metadata. If cross-step error classification is required, extend the event structure instead of reusing stale state names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java` around lines 167 - 179, When FX locking fails in PaymentWorkflowImpl (check around FxLockResult handling), update the publishEvent calls that currently pass "COMPLIANCE_CHECK" as the failedState to instead pass "FAILED" so the event matches the actual terminal state; specifically change the failed-state argument in the two PaymentEventRequest.failed(...) invocations that follow the FX error and FX rejection branches (related symbols: publishEvent, PaymentEventRequest.failed, currentState, FxLockResult, PaymentResult.failed) to "FAILED".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.java`:
- Line 73: The static TEST_ENV field (TestWorkflowEnvironment TEST_ENV) in
PaymentLifecycleTest creates a shared environment that can conflict across test
classes; move its instantiation into the test lifecycle by removing the static
initializer and creating TEST_ENV inside the `@BeforeAll` method (and
nulling/closing it in `@AfterAll`), or alternatively document/annotate that
parallel test execution is unsupported; specifically update the
PaymentLifecycleTest to allocate TestWorkflowEnvironment in the `@BeforeAll` setup
and shut it down in `@AfterAll` to ensure clear ownership and avoid cross-test
interference.
- Around line 289-292: The test in PaymentLifecycleTest currently verifies
fxEngineClient.getQuote(...) was called but doesn't assert that
fxEngineClient.lockRate(...) was attempted; add a verification line such as
then(fxEngineClient).should().lockRate(any(), any()); (placed alongside the
existing then(...) assertions after the action) to ensure lockRate interaction
is covered by the test.
---
Outside diff comments:
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.java`:
- Around line 167-179: When FX locking fails in PaymentWorkflowImpl (check
around FxLockResult handling), update the publishEvent calls that currently pass
"COMPLIANCE_CHECK" as the failedState to instead pass "FAILED" so the event
matches the actual terminal state; specifically change the failed-state argument
in the two PaymentEventRequest.failed(...) invocations that follow the FX error
and FX rejection branches (related symbols: publishEvent,
PaymentEventRequest.failed, currentState, FxLockResult, PaymentResult.failed) to
"FAILED".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7e500fd-9fbb-42fb-88cf-6321e6b56fb8
📒 Files selected for processing (5)
payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/AbstractBusinessTest.javapayment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/config/TemporalConfig.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowImpl.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/workflow/PaymentWorkflowTest.java
| @DisplayName("Payment Lifecycle — Business Tests") | ||
| class PaymentLifecycleTest extends AbstractBusinessTest { | ||
|
|
||
| private static final TestWorkflowEnvironment TEST_ENV = TestWorkflowEnvironment.newInstance(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Static TestWorkflowEnvironment requires careful lifecycle management.
TEST_ENV is a static field shared across test instances. With @TestInstance(Lifecycle.PER_CLASS), the @BeforeAll/@AfterAll methods correctly manage lifecycle per test class instance. However, if multiple test classes inherit this pattern, parallel execution could conflict.
Consider moving TestWorkflowEnvironment instantiation into the @BeforeAll method for clearer ownership, or document that parallel test execution is unsupported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.java`
at line 73, The static TEST_ENV field (TestWorkflowEnvironment TEST_ENV) in
PaymentLifecycleTest creates a shared environment that can conflict across test
classes; move its instantiation into the test lifecycle by removing the static
initializer and creating TEST_ENV inside the `@BeforeAll` method (and
nulling/closing it in `@AfterAll`), or alternatively document/annotate that
parallel test execution is unsupported; specifically update the
PaymentLifecycleTest to allocate TestWorkflowEnvironment in the `@BeforeAll` setup
and shut it down in `@AfterAll` to ensure clear ownership and avoid cross-test
interference.
Comment on lines
+289
to
+292
| // Both S2 and S6 were called | ||
| then(complianceCheckClient).should().initiateCheck(any()); | ||
| then(fxEngineClient).should().getQuote(any(), any(), any()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying lockRate was attempted.
The test verifies getQuote was called but doesn't explicitly assert lockRate was invoked before failing. Add:
then(fxEngineClient).should().lockRate(any(), any());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/business-test/java/com/stablecoin/payments/orchestrator/PaymentLifecycleTest.java`
around lines 289 - 292, The test in PaymentLifecycleTest currently verifies
fxEngineClient.getQuote(...) was called but doesn't assert that
fxEngineClient.lockRate(...) was attempted; add a verification line such as
then(fxEngineClient).should().lockRate(any(), any()); (placed alongside the
existing then(...) assertions after the action) to ensure lockRate interaction
is covered by the test.
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
TestWorkflowEnvironment, real DB (TestContainers PostgreSQL), and mocked S2/S6 Feign clientsPaymentWorkflowImplused invalid"FX_LOCKING"state in failed event requests —PaymentStatehas no such value, causing silentIllegalArgumentExceptioninEventPublishingActivityImpl.mapToEvent()Test plan
./gradlew :payment-orchestrator:payment-orchestrator:businessTest)Closes STA-116
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores