Split the Stripe and Square test files into focused suites - #1694
Merged
Conversation
…s to fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CuriW8NroWQjt96b644uwC
The oversized test/lib/stripe.test.ts and test/lib/square.test.ts were excluded from the noExcessiveLinesPerFile line-count rule. Split each into a folder of focused suites under the 1,000-line ceiling and drop both from the biome.json exclusion list. Each folder gets a harness.ts wrapping the shared environment/database setup (describeStripe/describeSquare, plus the Square mock client), and a fixtures.ts of shared builders and helpers. Splitting surfaced duplication that jscpd could not see inside the monoliths, so the repeated setup, mock-session/payment builders, signature-header signing, and assertion sequences were factored into those helpers to keep jscpd at 0%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CuriW8NroWQjt96b644uwC
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR replaces aggregate Square and Stripe test files with focused suites covering clients, payment flows, providers, transports, connections, refunds, and webhooks. Shared fixtures and lifecycle harnesses support the suites, and the lint override excludes the removed aggregate files. ChangesPayment integration test suites
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@test/lib/square/client.test.ts`:
- Around line 26-53: Strengthen the tests for getSquareClient: in “returns
cached client on second call with same token,” assert client2 is the same object
as client1; in “returns client in sandbox mode when sandbox setting enabled,”
verify the returned client is configured for sandbox; and in “recreates client
when sandbox setting changes,” assert client2 differs from client1 and reflects
sandbox configuration.
In `@test/lib/square/provider.test.ts`:
- Around line 267-277: Rename the test describing verifyWebhookSignature to
explicitly indicate it covers missing webhook-key behavior, since it does not
verify notification URL delegation. Alternatively, configure a valid webhook
key/signature pair and assert that the notification URL is forwarded, updating
the test name and expectations accordingly.
In `@test/lib/square/rest-transport.test.ts`:
- Around line 336-352: The HTTP error test uses a try/catch that can swallow its
own failure assertion. Replace the try/catch and expect(true) pattern in the
“throws error with status code and body for HTTP errors” test with a direct
async rejection assertion, verifying the rejection message contains both “Status
code: 400” and “BAD_REQUEST”.
- Around line 20-30: Replace the local jsonResponse and installMockFetch helpers
with the shared setupFetchStub/stubFetch utility from
test/test-utils/fetch-stub.ts, updating the tests to use its response and
automatic restoration behavior. Remove the redundant local fetch mocking and
related setup.
In `@test/lib/square/retrieve-refund.test.ts`:
- Around line 201-208: Configure a Square access token before calling
refundPayment in the “returns false when payment retrieval returns null” test,
then retain the retrievePayment null stub and assert it was called. Use the
existing credential/configuration setup and the withMocks test structure to
ensure execution reaches the null-retrieval branch rather than the no-token
path.
In `@test/lib/square/webhook.test.ts`:
- Around line 98-109: The valid webhook tests using constructTestWebhookEvent
and verify are tautologically coupled through shared signing or payload logic.
Update the valid-path cases to use a fixed, independently maintained payload and
known-good signature fixture, while testing constructTestWebhookEvent separately
for output correctness; apply this to both referenced test sections and retain
assertions for the expected listing ID and type.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d7091661-b397-42fe-8ae2-12d54f1f7106
📒 Files selected for processing (18)
biome.jsontest/lib/square.test.tstest/lib/square/client.test.tstest/lib/square/fixtures.tstest/lib/square/harness.tstest/lib/square/payment-link-validation.test.tstest/lib/square/payment-link.test.tstest/lib/square/provider.test.tstest/lib/square/rest-transport.test.tstest/lib/square/retrieve-refund.test.tstest/lib/square/webhook.test.tstest/lib/stripe.test.tstest/lib/stripe/connection.test.tstest/lib/stripe/core.test.tstest/lib/stripe/fixtures.tstest/lib/stripe/harness.tstest/lib/stripe/provider.test.tstest/lib/stripe/webhook.test.ts
💤 Files with no reviewable changes (2)
- test/lib/stripe.test.ts
- test/lib/square.test.ts
Close the coverage gaps that failed CI after the test-file split and address the CodeRabbit review findings: - Square mock harness: replace the uncovered `noop` default with an `asSpy` helper whose branches are both exercised. - Stripe fixtures: drop the unreachable client-null guard for a non-null assertion (the key was just stored). - getSquareClient tests: assert cached identity, and drive a request to prove sandbox vs production host selection on create and on toggle. - rest-transport HTTP-error test: assert the rejection outside the try/catch so a missing throw can't be swallowed. - refundPayment null-retrieval test: assert the stub was called so the test provably reaches the null branch. - Square provider webhook test: rename to describe the missing-key behaviour it actually verifies and assert the error message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CuriW8NroWQjt96b644uwC
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/lib/square/provider.test.ts (1)
267-279: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
result.validdirectly instead of gating in anif.The
if (!result.valid)guard makes the error-message check conditional — ifvalidwere ever unexpectedlytrue, this test would pass vacuously instead of failing. Assertexpect(result.valid).toBe(false)unconditionally, then checkresult.error.🔧 Proposed fix
- if (!result.valid) { - expect(result.error).toBe("Webhook signature key not configured"); - } + expect(result.valid).toBe(false); + expect(result.error).toBe("Webhook signature key not configured");🤖 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 `@test/lib/square/provider.test.ts` around lines 267 - 279, Remove the conditional guard around the error assertion in the verifyWebhookSignature failure test. After the existing expect(result.valid).toBe(false), assert result.error directly using the expected message, leveraging the test’s narrowed failure result without wrapping it in an if statement.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/lib/square/provider.test.ts`:
- Around line 267-279: Remove the conditional guard around the error assertion
in the verifyWebhookSignature failure test. After the existing
expect(result.valid).toBe(false), assert result.error directly using the
expected message, leveraging the test’s narrowed failure result without wrapping
it in an if statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02a26548-d7ea-477c-bd13-f3735d4fcaed
📒 Files selected for processing (6)
test/lib/square/client.test.tstest/lib/square/harness.tstest/lib/square/provider.test.tstest/lib/square/rest-transport.test.tstest/lib/square/retrieve-refund.test.tstest/lib/stripe/fixtures.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad1e6deb46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+49
to
+51
| beforeEach(async () => { | ||
| resetSquareClient(); | ||
| await createTestDb(); |
There was a problem hiding this comment.
When the Square monolith is split into several files, each file has fewer than RECLAIM_FDS_EVERY DB setups, so createTestDb()'s amortized GC never runs for these suites; unlike describeWithEnv, this custom harness also has no afterAll call to reclaimLeakedFdsNow. Under the standard deno test --parallel harness on a low file-descriptor limit, the libsql descriptors documented in test/test-utils/reclaim-fds.ts can therefore accumulate across the new Square test files and reintroduce flaky Too many open files failures.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Good catch — fixed in d798039. The Square harness's describeSquare now calls reclaimLeakedFdsNow() in afterAll, exactly as describeWithEnv does, so each split file hands back its leaked libsql descriptors at teardown even though it runs fewer than RECLAIM_FDS_EVERY DB setups. (The Stripe harness already routes through describeWithEnv, so it was already covered.)
Generated by Claude Code
The split Square suites each run fewer than RECLAIM_FDS_EVERY DB setups, so createTestDb's amortised GC never fires for them. Unlike the Stripe harness (which uses describeWithEnv), the Square harness uses a plain describe with no afterAll reclaim, so libsql's leaked descriptors could accumulate across the new files and reintroduce flaky "Too many open files" failures under --parallel. Call reclaimLeakedFdsNow in afterAll, matching describeWithEnv. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CuriW8NroWQjt96b644uwC
stefan-burke
enabled auto-merge
July 10, 2026 10:38
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Jul 10, 2026
…e-square-tests-gcs9r3 # Conflicts: # biome.json
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
2 participants
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.
What changed
The two biggest test files,
test/lib/stripe.test.ts(~2,460 lines) andtest/lib/square.test.ts(~2,130 lines), were too large to fit under the project's per-file line limit, so they had been added to an exclusion list that turns that limit off for them. This change splits each one into a small folder of focused test files, all comfortably under the limit, and removes both from the exclusion list so the rule now applies to them like every other file.test/lib/stripe/—core,connection,webhook, andprovidersuites, plus a sharedharness.tsandfixtures.ts.test/lib/square/—client,payment-link,payment-link-validation,retrieve-refund,webhook,rest-transport, andprovidersuites, plus a sharedharness.tsandfixtures.ts.Each
harness.tsholds the setup every suite in that folder shares — the test environment, a fresh payment client before each test, and a clean database around each test (and, for Square, the mock SDK client). Eachfixtures.tsholds the small builders and helpers the tests reuse: sample checkout items and orders, the code that signs a webbook signature header, and the common "set up a client, stub one call, check the result" steps.Why the extra helpers
Splitting a very large file makes the duplicate-code checker able to see inside it for the first time — the same repeated setup and assertion blocks that were invisible in the monolith become visible once the file is smaller. Rather than silence that, the repeated pieces were pulled into the shared
harness.tsandfixtures.tshelpers, keeping the duplication checker at its required 0%.No test was removed or changed in what it verifies; the tests were only regrouped and their shared scaffolding factored out. No production code changed.
Checks
deno task precommitpasses end to end: typecheck, lint, duplicate-code check (0%), and the full test suite with 100% coverage.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit