OUT-3773: integration tests for payment.succeeded webhook event#256
Open
SandipBajracharya wants to merge 6 commits into
Open
OUT-3773: integration tests for payment.succeeded webhook event#256SandipBajracharya wants to merge 6 commits into
SandipBajracharya wants to merge 6 commits into
Conversation
…ion tests Webhook pre-claim sleeps (INVOICE_UPDATED, INVOICE_VOIDED, PAYMENT_SUCCEEDED) would add ≥7s per test; we don't exercise race ordering at this layer. afterIfAvailable is mocked because direct imports of SyncLogService transitively load next/server's AsyncLocalStorage shim and corrupt next-test-api-route-handler's request lifecycle. The shim calls the callback directly, keeping next/server out of the module graph entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add TEST_COPILOT_PAYMENT_ID and TEST_QB_PURCHASE_ID seed constants. - Add createPurchase and deletePurchase defaults to createMockIntuitAPI. - Make getAnAccount echo the id back so asset/expense lookups return the ref that was passed in; name-only callers still get the income ref. - Flip getInvoice default from undefined to a real invoice whose number matches TEST_INVOICE_NUMBER. No existing test calls getInvoice. - New setupPaymentSucceededTest helper and paymentSucceeded fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight tests pinning every branch of WebhookService.handlePaymentSucceeded: happy path, absorbed-fee flag off, no platform-paid fee, idempotent re-delivery, Copilot invoice not found, local invoice-sync miss, QB createPurchase failure, and the revert-on-log-failure rollback. copilotInvoiceNotFound pins a production quirk: the !invoice throw at webhook.service.ts:518 sits outside the try/catch on line 524, so the handler returns 404 with a leaked PENDING claim row instead of logging FAILED. A follow-up ticket should move the check inside the try. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r wrap Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
SandipBajracharya
changed the title
Greptile SummaryAdds 8 integration tests covering every branch of
Confidence Score: 5/5Pure test-and-infrastructure addition with no production code changes; safe to merge. Every changed file is in the test tree. The mock changes are backward-compatible and confirmed non-regressive by the full 40-test integration suite. The revertOnLogFailure spy is correctly scoped with try/finally to avoid leakage under isolate: false. The copilotInvoiceNotFound test deliberately pins a known production bug with clear documentation. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "test(OUT-3773): use TEST_COPILOT_INVOICE..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+65
to
+69
| // payment.succeeded needs a real invoice object to proceed past the getInvoice guard (OUT-3773) | ||
| getInvoice: vi.fn().mockResolvedValue({ | ||
| id: 'inv-cop-0001', | ||
| number: TEST_INVOICE_NUMBER, | ||
| }), |
There was a problem hiding this comment.
id field in the getInvoice default is hardcoded as 'inv-cop-0001' while every other seed value in this file uses the named constant. TEST_COPILOT_INVOICE_ID already equals that string, so if the constant changes the mock silently drifts.
Suggested change
| // payment.succeeded needs a real invoice object to proceed past the getInvoice guard (OUT-3773) | |
| getInvoice: vi.fn().mockResolvedValue({ | |
| id: 'inv-cop-0001', | |
| number: TEST_INVOICE_NUMBER, | |
| }), | |
| // payment.succeeded needs a real invoice object to proceed past the getInvoice guard (OUT-3773) | |
| getInvoice: vi.fn().mockResolvedValue({ | |
| id: TEST_COPILOT_INVOICE_ID, | |
| number: TEST_INVOICE_NUMBER, | |
| }), |
Comment on lines
+87
to
+95
| getAnAccount: vi | ||
| .fn() | ||
| .mockImplementation(async (_name?: string, id?: string) => ({ | ||
| Id: id ?? TEST_INCOME_ACCOUNT_REF, | ||
| Name: 'Sales of Product Income', | ||
| SyncToken: '0', | ||
| Active: true, | ||
| AccountType: 'Income', | ||
| })), |
There was a problem hiding this comment.
AccountType is always 'Income' for every account — the id-echo mock returns the same type regardless of which account id is passed. checkAndUpdateAccountStatus doesn't validate AccountType today so tests pass, but a future call site or test inspecting this field for an asset or expense account will silently receive the wrong type. Consider accepting AccountType as a parameter or adding a comment noting the field is a stub.
Greptile flagged that AccountType is always 'Income' regardless of which account id is passed. No current caller of getAnAccount inspects the field, so behavior is unchanged — adding a comment so a future test author knows to parameterize before relying on it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile flagged that the id field was hardcoded as 'inv-cop-0001' while the rest of the file uses the named seed constant. The value matches today but would silently drift if the constant is renamed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
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/integration/quickbooks/paymentSucceeded/pinning every branch ofWebhookService.handlePaymentSucceeded— happy path, absorbed-fee flag off, no platform-paid fee, idempotent re-delivery, Copilot invoice not found, local invoice-sync miss, QBcreatePurchasefailure, revert-on-log-failure rollback.@/utils/sleepandafterIfAvailablemocks in the integration setup, two new seed constants, expandedcreateMockIntuitAPIdefaults (createPurchase/deletePurchase), id-echoinggetAnAccount, realgetInvoicedefault, newsetupPaymentSucceededTesthelper, new webhook fixture.Production finding (separate follow-up worth filing)
webhook.service.ts:518throwsAPIError(404)outside the try/catch on line 524. When Copilot returns no invoice, the handler responds 404 and leaves a PENDING claim row instead of writing FAILED. ThecopilotInvoiceNotFoundtest pins this current behavior; the fix is to move the check inside the try.Test plan
npx vitest run --project integration test/integration/quickbooks/paymentSucceeded/→ 8 files / 9 tests passnpx vitest run --project integration→ 29 files / 40 tests pass (no regressions)npx vitest run --project unit→ 15 files / 238 tests passyarn lint:checkclean (0 errors, pre-existing useEffect warnings only)yarn prettier:checkcleanLinear: https://linear.app/assemblycom/issue/OUT-3773
🤖 Generated with Claude Code