test(integration): add Payment + Shipping integration projects (real DB + Wolverine)#53
Merged
Merged
Conversation
WalkthroughAdds comprehensive integration test suites for PaymentService and ShippingService using testcontainers (MS SQL and PostgreSQL), factory classes that bootstrap services with stubbed external transports, test-only authentication handlers, end-to-end tests validating payment/shipment processing, idempotency, concurrency, and authorization. Also refactors the Payment endpoint to derive BuyerId from JWT claims rather than accepting from request body, and documents secret scanning suppression for test-only fake credentials. ChangesPaymentService Integration Tests
ShippingService Integration Tests
Payment Endpoint Request Contract and Secret Scanning Documentation
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/PaymentService.Tests.Integration/PaymentProcessingTests.cs`:
- Line 45: The test should simulate an IDOR by sending a ProcessPaymentCommand
whose BuyerId differs from the authenticated principal and assert the API
returns 404 and no Payment is created; update the integration test in
PaymentProcessingTests.cs to construct the command with Amount/Currency/OrderId
but set BuyerId != TestAuthHandler.BuyerId, call POST /api/v1/payments/process
as the authenticated test user, and assert response.StatusCode == 404 and the
payments store has no payment for the other buyer; to fix server behavior,
ensure PaymentEndpoints (or its request binding pipeline) overrides/binds
request.BuyerId from the authenticated principal (e.g., replace reading
request.BuyerId with the User principal value) or have ProcessPaymentHandler
ignore incoming BuyerId and derive buyerId from the HttpContext/User before
creating the Payment, so ownership checks use the authenticated buyer only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d722e102-2e4f-4d2a-b937-b09c19f575bf
📒 Files selected for processing (9)
NextAurora.slnxtests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/PaymentService.Tests.Integration/PaymentProcessingTests.cstests/PaymentService.Tests.Integration/PaymentService.Tests.Integration.csprojtests/PaymentService.Tests.Integration/TestAuthHandler.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cstests/ShippingService.Tests.Integration/ShippingService.Tests.Integration.csprojtests/ShippingService.Tests.Integration/ShippingTests.cstests/ShippingService.Tests.Integration/TestAuthHandler.cs
3 tasks
emeraldleaf
added a commit
that referenced
this pull request
May 31, 2026
…ml + encode the convention ## What this fixes Gitleaks failed on PR #53 with two `generic-api-key` findings (entropy 4.678): - tests/PaymentService.Tests.Integration/PaymentApiFactory.cs:95 - tests/ShippingService.Tests.Integration/ShippingApiFactory.cs:82 Both contain the byte-identical fake Azure Service Bus connection string that OrderApiFactory.cs has used on main for weeks — the `SharedAccessKey` base64-decodes to the literal `fake-shared-key-for- testing-only`. The scanner is correctly identifying the high entropy, but the value is a self-labeling fake, not a real credential. The OrderApiFactory occurrence had slipped past the scanner on its PR (diff-range quirk); now that two new sites surfaced the latent finding, the convention deserves canonicalizing. ## The fix (.gitleaks.toml) New `.gitleaks.toml` at repo root extends gitleaks' default ruleset (useDefault = true) and adds a narrowly-scoped allowlist requiring BOTH path AND regex to match — meaning: - A real Azure SAS key pasted into any of the three factory files still trips (regex won't match). - The allowlisted fake literal pasted anywhere else still trips (path won't match). - Every other built-in rule (AWS keys, JWTs, private keys, .env) stays active. `.github/workflows/gitleaks.yml` pins `GITLEAKS_CONFIG=.gitleaks.toml` explicitly. gitleaks-action auto-discovers the file at repo root, but pinning is auditable in CI logs ("loaded config from .gitleaks.toml") and prevents silent fallback to defaults if the file is ever renamed. ## The rule (CLAUDE.md "Testing") Added a new bullet codifying the pattern: fake credentials in test fixtures are allowlisted in .gitleaks.toml, not lowered-entropy. Some factory fakes have to be protocol-syntax-valid (the ASB string here is required because Program.cs eagerly parses UseAzureServiceBus(GetConnectionString("messaging")) before DisableAllExternalWolverineTransports() stubs the transport). The convention: keep the high-entropy base64-encoded self-labeling literal + add a narrow path+regex allowlist. The fix is config, not lowering the fake's entropy below scanner thresholds. ## Cross-reference markers All three factories (Order/Payment/Shipping) now have inline `See CLAUDE.md.` comments at the ASB-connection-string site so the paraphrase is greppable per the project's cross-reference convention. ## What this also closes The latent OrderApiFactory finding (scanner missed it earlier; same fake literal). The allowlist's `paths =` list covers all three factories, so the inconsistency where one was tolerated by scanner luck and two were caught is closed in one move. ## Continuous Rule Encoding pass This commit ships the fix AND the rule (per CLAUDE.md "Continuous Rule Encoding — a merged fix PR without the corresponding rule is a half-finished job"): - Fix: .gitleaks.toml + .github/workflows/gitleaks.yml - Rule: CLAUDE.md "Testing" + paraphrase markers in the three factories - Surfaces NOT touched: .coderabbit.yaml path_instructions, the architecture-reviewer Pattern Checklist, .claude/skills/. These could each gain a small entry; deferred because the rule is narrow (one specific scanner + one specific file category) and the path+regex allowlist itself is the enforcement mechanism. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitleaks.toml:
- Around line 24-34: The allowlist block in .gitleaks.toml uses global
[[allowlists]] which defaults to OR semantics so currently paths OR regex will
allow secrets; update the existing [[allowlists]] entry to include condition =
"AND" so both paths and regexes must match for the allowlist to apply (target
the same [[allowlists]] block that contains the paths list and regexes list),
and also update CI to pin the gitleaks/action to >= v8.25.0 to ensure global
allowlists are supported.
- Around line 13-14: Add a top-level minVersion = "v8.25.0" to the
.gitleaks.toml so the [[allowlists]] syntax is supported by the scanner; update
the toml to include minVersion (and keep the existing [extend] useDefault =
true) and optionally align the GITLEAKS_VERSION used by the gitleaks action
(gitleaks/gitleaks-action@v2) to v8.25.0 to ensure the action runs the expected
binary; reference the minVersion key, the existing [[allowlists]] usage, and the
GITLEAKS_VERSION variable when making the change.
In `@CLAUDE.md`:
- Line 219: The CLAUDE.md change updates guidance about fake credentials and
allowlisting; update the repo by (1) modifying .gitleaks.toml to set the
relevant allowlist block's condition = "AND" so path+regex must both match (and
add the narrow path+regex entries used for the factory fakes), (2) update the
three inline paraphrases/comments that end with "See CLAUDE.md." (e.g. in
OrderApiFactory.cs, PaymentApiFactory.cs, ShippingApiFactory.cs) to reflect the
new fake-credential/allowlist wording and add the required note "Run
/check-rules locally to audit paraphrases against this diff.", and (3) include a
short repository note near the CLAUDE.md change saying "Flag: Run /check-rules
locally to audit paraphrases against this diff." so reviewers see it alongside
the doc change; ensure references to DisableAllExternalWolverineTransports()
remain unchanged.
In `@tests/ShippingService.Tests.Integration/ShippingApiFactory.cs`:
- Around line 80-81: Update the inline comment about the test SharedAccessKey to
accurately describe the gitleaks allowlist semantics: state that the secret is
allowed only because the .gitleaks.toml entry matches both the file path and the
regex (per CLAUDE.md), not by an exact-string-only match; mention the
SharedAccessKey literal, .gitleaks.toml, and CLAUDE.md so future readers know
both conditions are required to avoid false assumptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33bc688c-763b-4704-bbc7-69fc7f17d845
📒 Files selected for processing (6)
.github/workflows/gitleaks.yml.gitleaks.tomlCLAUDE.mdtests/OrderService.Tests.Integration/OrderApiFactory.cstests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cs
…DB + Wolverine) Closes the integration coverage gap STATUS.md flagged after the simplicity refactor — Payment and Shipping had no integration projects, so their handlers' EF-touching paths, outbox staging, IDOR predicates, and saga consume-side logic ran un-covered against any real DB. Adds two Testcontainers-backed slices that mirror the proven Catalog (Postgres) and Order (SQL Server + Wolverine-stub) factories. ## PaymentService.Tests.Integration (SQL Server + Wolverine-stub) - ProcessPayment_drives_Acceptor_to_Gateway_and_completes_with_event — end-to-end through the Acceptor + Gateway split: POST → ProcessPaymentHandler (Acceptor) persists Pending + publishes PaymentProcessingRequested → local-queue Gateway handler charges (stubbed IPaymentGateway) → marks Completed + publishes PaymentCompletedEvent through the outbox. Asserts the event flowed and ExternalTransactionId was set. - ProcessPayment_marks_Failed_and_publishes_PaymentFailedEvent_on_gateway_decline - ProcessPayment_is_idempotent_per_order_under_duplicate_requests — Acceptor's OrderId existence check + the unique-index backstop. - Payment_RowVersion_token_rejects_concurrent_write — DbUpdateConcurrencyException on racing writes against the same Pending payment. ## ShippingService.Tests.Integration (Postgres + Wolverine-stub) - GetShipmentByOrder_returns_404_for_another_buyers_shipment_IDOR_safe — buyer X requests buyer Y's shipment, asserts 404 (not 403). The canonical IDOR pattern from CLAUDE.md "Security Requirements → Authorization." Without this test, an IDOR could survive undetected. - GetShipmentByOrder_returns_shipment_for_owning_buyer — affirmative case + read-side projection over real Postgres. - PaymentCompletedHandler_creates_shipment_and_is_idempotent — dispatch the saga event twice through Wolverine's consumer pipeline; assert exactly one Shipment row. Validates the at-least-once idempotency guard. Pattern fidelity with Catalog/Order factories: same teardown ordering (host before container; swallow OperationCanceledException on shutdown), same fake ASB connection string + DisableAllExternalWolverineTransports + Wolverine:AutoProvision=false. Tests use Wolverine.Tracking's TrackActivity().ExecuteAndWaitAsync / PublishMessageAndWaitAsync to settle async cascades before assertion. NotificationService remains unit-only. Stateless event-to-email pump, no DB, no outbox. Integration tier would mainly verify DI composition + the Wolverine event-routing wire, both marginal value-add relative to the cost (factory + auth + Docker container time). Honest skip, not an oversight. ## Gitleaks suppression for fake ASB connection strings The fake Azure Service Bus connection string in OrderApiFactory.cs (existing on main) / PaymentApiFactory.cs (new) / ShippingApiFactory.cs (new) carries a SharedAccessKey that base64-decodes to "fake-shared-key-for-testing-only". Gitleaks' generic-api-key rule fires on its entropy regardless of value content. Suppression mechanism: inline `// gitleaks:allow` markers at the end of the connection-string literal lines in all three factories. Reliable across gitleaks 8.x. Suppresses only the specific finding line — pasting a real key into the same file still trips because the marker only covers THAT line. .gitleaks.toml at repo root documents the project's convention. Its [[allowlists]] block was tried first but does not reliably override extended-default rules in gitleaks 8.24.x; kept as supplementary documentation. The workflow pins GITLEAKS_CONFIG=.gitleaks.toml explicitly for auditability. ## Continuous Rule Encoding pass Per CLAUDE.md "Continuous Rule Encoding — a merged fix PR without the corresponding rule is a half-finished job," this commit ships fix + rule: - Fix: inline `// gitleaks:allow` markers on three factory files + .gitleaks.toml + .github/workflows/gitleaks.yml - Rule: CLAUDE.md "Testing" — fake credentials in test fixtures are suppressed with inline markers, not lowered-entropy. The base64 literal is NOT reproduced in CLAUDE.md prose (the scanner walks docs too). - Paraphrase markers: all three factories have `See CLAUDE.md.` markers on the explanatory comment so the cross-reference is greppable. Verified locally: dotnet restore + build green, integration tests pass (Payment 4/4, Shipping 3/3 against fresh Docker). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7a9175f to
0506085
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.gitleaks.toml (1)
17-33: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueIf keeping the allowlist for documentation, make it technically correct.
The file explicitly states (lines 6-12) that the
[[allowlists]]block doesn't reliably suppress findings and is kept only as "supplementary documentation." However, if documenting intent, the documentation should be technically correct:
- Missing
minVersion:[[allowlists]]syntax requires gitleaks ≥ v8.25.0. Add a top-levelminVersion = "v8.25.0"before[extend].- Missing
condition: The allowlist currently defaults toMatchCondition = "OR"(path OR regex), but the intent appears to be "both path AND regex must match." Addcondition = "AND"to the[[allowlists]]block.Alternatively, if the block is purely vestigial, consider removing it entirely to avoid confusion.
♻️ Proposed fix to make documentation technically correct
+# Minimum gitleaks version required for [[allowlists]] syntax. +minVersion = "v8.25.0" + [extend] useDefault = true # Documentation of intent. The inline `gitleaks:allow` markers on the literal # lines in each of the three factories are what actually suppresses the # findings; this block is kept for the convention-encoding it carries. [[allowlists]] description = "Fake ASB connection string in integration test factories (documentation; primary suppressor is inline `gitleaks:allow`)" +# Require BOTH path AND regex to match (default is OR). +condition = "AND" paths = [ '''tests/OrderService\.Tests\.Integration/OrderApiFactory\.cs''', '''tests/PaymentService\.Tests\.Integration/PaymentApiFactory\.cs''', '''tests/ShippingService\.Tests\.Integration/ShippingApiFactory\.cs''', ] regexes = [ '''SharedAccessKey=[A-Za-z0-9+/=]{20,}''', ] regexTarget = "line"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitleaks.toml around lines 17 - 33, Add a top-level minVersion = "v8.25.0" to the .gitleaks.toml and update the [[allowlists]] block to require both path AND regex by adding condition = "AND"; specifically, preserve the existing [extend] section and the [[allowlists]] entry (including its description, paths, regexes and regexTarget) but insert minVersion = "v8.25.0" before [extend] and add condition = "AND" inside the [[allowlists]] block so the documentation is technically correct for gitleaks ≥ v8.25.0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.gitleaks.toml:
- Around line 17-33: Add a top-level minVersion = "v8.25.0" to the
.gitleaks.toml and update the [[allowlists]] block to require both path AND
regex by adding condition = "AND"; specifically, preserve the existing [extend]
section and the [[allowlists]] entry (including its description, paths, regexes
and regexTarget) but insert minVersion = "v8.25.0" before [extend] and add
condition = "AND" inside the [[allowlists]] block so the documentation is
technically correct for gitleaks ≥ v8.25.0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82b3b705-ba2d-4df2-8459-a83b9bf2f6d1
📒 Files selected for processing (13)
.github/workflows/gitleaks.yml.gitleaks.tomlCLAUDE.mdNextAurora.slnxtests/OrderService.Tests.Integration/OrderApiFactory.cstests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/PaymentService.Tests.Integration/PaymentProcessingTests.cstests/PaymentService.Tests.Integration/PaymentService.Tests.Integration.csprojtests/PaymentService.Tests.Integration/TestAuthHandler.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cstests/ShippingService.Tests.Integration/ShippingService.Tests.Integration.csprojtests/ShippingService.Tests.Integration/ShippingTests.cstests/ShippingService.Tests.Integration/TestAuthHandler.cs
…leading .gitleaks.toml Addresses CodeRabbit's review on PR #53. Two findings, both resolved. ## Finding 1 (critical): BuyerId IDOR on POST /api/v1/payments/process CodeRabbit's claim — verified — was that ProcessPaymentCommand accepts BuyerId from the HTTP body and the handler trusts it. Pre-fix, an authenticated buyer X could submit a JSON body with BuyerId set to buyer Y's Guid; the Payment row got created attributing the charge to Y. The validator only checked NotEmpty(), never compared against the JWT. Classic CWE-639 / mass-assignment. Fix: introduce a ProcessPaymentRequest record (the HTTP body shape) that does NOT expose BuyerId on the wire. The endpoint reads BuyerId from the JWT NameIdentifier claim and constructs the internal ProcessPaymentCommand with the trusted value. Returns 401 Unauthorized if the claim is missing or unparseable. The saga path (OrderPlacedHandler → ProcessPaymentCommand) is unaffected because BuyerId there comes from the trusted OrderPlacedEvent. Why not also cross-check the OrderId against the buyer's owned orders at the handler tier? Because PaymentService doesn't own the Orders table — the proper cross-check would require a gRPC call to OrderService, which is significant scope creep for this PR. The endpoint-tier JWT gate is sufficient to prevent the attribution spoof CodeRabbit flagged; any remaining "X pays for Y's order" issue is bounded (X is paying out of their own pocket) and can be a follow-up. Pattern mirrors OrderEndpoints POST /orders + the canonical CLAUDE.md "Security Requirements → Server-controlled fields" rule. ### New IDOR integration test ProcessPayment_ignores_spoofed_BuyerId_in_request_body_and_uses_JWT_claim posts a JSON body with a spoofed BuyerId field via an anonymous object (ProcessPaymentRequest doesn't declare BuyerId, so System.Text.Json silently drops it during binding). Asserts: 1. The Payment row's BuyerId equals TestAuthHandler.BuyerId, NOT the spoofed value. 2. No Payment row exists attributed to the spoofed BuyerId for that OrderId. Test follows the AAA narrative-comments convention; two numbered ASSERT invariants with rationale per CLAUDE.md "Testing → AAA with narrative comments." The existing 4 tests are unaffected — they post the full ProcessPaymentCommand and the extra BuyerId field is silently ignored, but the BuyerId they use already equals TestAuthHandler.BuyerId, so the JWT-vs-body source change is transparent to them. ## Finding 2 (major+minor): .gitleaks.toml [[allowlists]] doesn't work CodeRabbit caught two real facts I missed: (a) Global [[allowlists]] was introduced in gitleaks v8.25.0; the gitleaks-action ships v8.24.x. The block silently doesn't fire. (b) MatchCondition defaults to OR not AND, so even if the block fired it would over-match. Action: delete .gitleaks.toml entirely. The inline `// gitleaks:allow` markers on the three factories are the load-bearing suppression mechanism and are reliable across all 8.x versions. Drop GITLEAKS_CONFIG from .github/workflows/gitleaks.yml (defaults are sufficient now). ### CLAUDE.md rewrite Rewrites the fake-credentials bullet to: - Name the inline marker as the load-bearing mechanism (was previously described as "supplementary" to the .toml). - Document why no .gitleaks.toml: 8.25+ requirement + OR-default — both real, both caught me, both worth encoding. - Add the diff-range residue trap as a new line: gitleaks scans every commit in the PR range, not just HEAD, so a fix-in-a-later-commit doesn't suppress findings from un-marked earlier commits. Squash the branch if you hit this. (Captured the hard-won lesson from the squash-to-unblock cycle this session.) - Removes the high-entropy base64 literal from prose (was the source of a third gitleaks finding when reproduced in CLAUDE.md). ### Factory inline comments Three factories had `See CLAUDE.md.` paraphrases referencing .gitleaks.toml; updated to reference the inline marker as load-bearing and explain why no config exists (8.25+ requirement). Cross-reference convention is preserved. ## Local verification dotnet build clean (0 warnings under TreatWarningsAsErrors). dotnet test on PaymentService.Tests.Integration is blocked locally by Docker content-store corruption (input/output error on sha256 blob in Docker Desktop's containerd content store — same machine-level issue that cleared after a restart earlier this session). CI runs against a fresh Docker daemon and is the verification surface. Run /check-rules locally to audit paraphrases against this CLAUDE.md diff. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CLAUDE.md (1)
219-219: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd the required CLAUDE.md audit note.
Please add: “Run /check-rules locally to audit paraphrases against this diff.” alongside this rule update so downstream paraphrases are explicitly re-audited.
As per coding guidelines: "Flag the CLAUDE.md change with a note: 'Run /check-rules locally to audit paraphrases against this diff.'"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` at line 219, Add the requested audit note to the CLAUDE.md rule update about gitleaks: insert the exact sentence “Run /check-rules locally to audit paraphrases against this diff.” adjacent to the paragraph that explains the inline `// gitleaks:allow` marker and the rationale (the block that starts “Fake credentials in test fixtures are suppressed with inline `// gitleaks:allow` markers…”), so the CLAUDE.md change is explicitly flagged for local paraphrase auditing; ensure the sentence is placed inline with that rule description and not duplicated elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@CLAUDE.md`:
- Line 219: Add the requested audit note to the CLAUDE.md rule update about
gitleaks: insert the exact sentence “Run /check-rules locally to audit
paraphrases against this diff.” adjacent to the paragraph that explains the
inline `// gitleaks:allow` marker and the rationale (the block that starts “Fake
credentials in test fixtures are suppressed with inline `// gitleaks:allow`
markers…”), so the CLAUDE.md change is explicitly flagged for local paraphrase
auditing; ensure the sentence is placed inline with that rule description and
not duplicated elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2ec9361-5cbe-4a9f-a411-adf454062c4d
📒 Files selected for processing (8)
.github/workflows/gitleaks.ymlCLAUDE.mdPaymentService/Endpoints/PaymentEndpoints.csPaymentService/Features/ProcessPayment.cstests/OrderService.Tests.Integration/OrderApiFactory.cstests/PaymentService.Tests.Integration/PaymentApiFactory.cstests/PaymentService.Tests.Integration/PaymentProcessingTests.cstests/ShippingService.Tests.Integration/ShippingApiFactory.cs
6 tasks
Open
4 tasks
This was referenced Jun 14, 2026
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
Closes the integration-coverage gap STATUS.md flagged after the simplicity refactor — Payment and Shipping previously had no integration projects, so their handlers' EF-touching paths, outbox staging, IDOR predicates, and saga consume-side logic ran un-covered against any real DB. Adds two Testcontainers-backed slices that mirror the proven Catalog (Postgres) and Order (SQL Server + Wolverine-stub) factories.
PaymentService.Tests.Integration (SQL Server + Wolverine-stub) — 4/4 green
ProcessPayment_drives_Acceptor_to_Gateway_and_completes_with_event— validates this session's Acceptor + Gateway split end-to-end. POST →ProcessPaymentHandler(Acceptor) persistsPending+ publishesPaymentProcessingRequested→ local-queue Gateway handler charges (stubbedIPaymentGatewaysuccess) → marksCompleted+ publishesPaymentCompletedEventthrough the outbox. Asserts the event flowed and the row reachedCompletedwith the gateway's external transaction id.ProcessPayment_marks_Failed_and_publishes_PaymentFailedEvent_on_gateway_decline— same flow with stubbed decline; assertsFailed+PaymentFailedEventwith the reason carried verbatim.ProcessPayment_is_idempotent_per_order_under_duplicate_requests— two POSTs for the same OrderId → exactly onePaymentrow. Validates the Acceptor's OrderId existence check + the unique-index backstop.Payment_RowVersion_token_rejects_concurrent_write— two scopes mutate the same Pending payment; secondSaveChangesthrowsDbUpdateConcurrencyException.ShippingService.Tests.Integration (Postgres + Wolverine-stub) — 3/3 green
GetShipmentByOrder_returns_404_for_another_buyers_shipment_IDOR_safe— the canonical IDOR pattern. Seed a shipment for a different buyer, request asTestAuthHandler.BuyerId, assert 404 (not 403). Proves the ownership predicate runs at SQL, not C#. This is exactly the failure mode CLAUDE.md "Testing" warns about: without this test, an IDOR could survive undetected.GetShipmentByOrder_returns_shipment_for_owning_buyer— affirmative case, exercises the IDOR predicate + the read-side projection over real Postgres.PaymentCompletedHandler_creates_shipment_and_is_idempotent— dispatchPaymentCompletedEventtwice through Wolverine's consumer pipeline; assert exactly oneShipmentrow. Validates the saga consume-side handler + at-least-once idempotency over real EF + Postgres.Why no NotificationService integration project
NotificationService is the "no aggregate" service — stateless event-to-email pump, no DB, no outbox. The existing
NotificationService.Tests.Unit(8 tests) cover the business logic (SendNotificationHandlervalidation,NotificationEventHandlersevent translation). An integration test would mainly verify DI composition + the Wolverine event-routing wire, both marginal value-add relative to the real cost (factory + auth + Wolverine wiring + Docker container time). The two services with real DB/outbox/IDOR concerns (Payment, Shipping) are where the integration tier earns its keep; Notification stays unit-only. Honest skip, not an oversight.Pattern fidelity
Both projects mirror the existing Catalog/Order factories deliberately: same teardown ordering (host before container; swallow
OperationCanceledExceptionon shutdown so a slow Wolverine background-agent stop doesn't fail an otherwise-green test run), same fake ASB connection string +DisableAllExternalWolverineTransports+Wolverine:AutoProvision=falseto keep the transport off the wire while exercising the outbox + middleware chain in-process. Tests useWolverine.Tracking'sTrackActivity().ExecuteAndWaitAsync/PublishMessageAndWaitAsyncto settle async cascades before assertion.Test plan
dotnet teston PaymentService.Tests.Integration — 4/4 pass locallydotnet teston ShippingService.Tests.Integration — 3/3 pass locallyNextAurora.slnxso CI picks them up via the solution test stepPairs with PR #52
PR #52 added the deterministic dependency-rule arch tests (the "architecture tests" rung of the enforcement spectrum). This PR adds the integration-tier coverage gap. Together they close most of the STATUS.md "most of it has not been runtime-verified" framing for the services with non-trivial behavior.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security Improvements
Bug Fixes