fix: update stripe unit tests for stripe v19+#1695
fix: update stripe unit tests for stripe v19+#1695ToriChanIntegration wants to merge 2 commits intoCap-go:mainfrom
Conversation
The stripe library v19+ uses createFetchHttpClient() which wasn't being mocked. This caused tests to fail with: 'Cannot read properties of undefined (reading 'createFetchHttpClient')' This fix: - Mocks the stripe module with createFetchHttpClient as a static method - Mocks hono/adapter with getRuntimeKey function - Uses shared mock functions that can be configured per-test - Adds beforeEach to reset mocks between tests
📝 WalkthroughWalkthroughThe test file for Stripe redirects was refactored to replace ad-hoc Stripe client construction and spy-based interactions with centralized mock implementations. Shared mock handlers and a beforeEach reset were added; tests now use pre-configured mocks for billingPortal.sessions.create, checkout.sessions.create, and prices.search. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/stripe-redirects.unit.test.ts`:
- Around line 13-29: The MockStripe class violates the
style/lines-between-class-members rule—add blank lines between the class members
to satisfy ESLint: insert a blank line after the static createFetchHttpClient
declaration, between the billingPortal and checkout property initialisers, and
between checkout and prices property initialisers in the MockStripe class so
each member is separated by a single empty line.
- Around line 4-6: The shared module-level mocks
(mockBillingPortalSessionsCreate, mockCheckoutSessionsCreate, mockPricesSearch)
cause race conditions with concurrent tests; move each vi.fn() declaration and
any mockResolvedValue/mockImplementation calls into the body of each
it.concurrent() test so every test gets its own isolated mocks, remove reliance
on the shared beforeEach reset, and ensure any test-specific mock behavior is
set inline in the test before exercising the code.
- Around line 8-41: Move the import of stripe (import * as stripe from
'../supabase/functions/_backend/utils/stripe.ts') to the top of the test file
immediately after the vitest imports and remove or replace the misleading
comment that says mocks must be before imports; keep the existing
vi.mock('stripe', ...) and vi.mock('hono/adapter', ...) calls (they are hoisted
by Vitest) and update the comment to state that vi.mock() calls are hoisted
automatically by Vitest so explicit ordering is unnecessary; ensure the mocked
symbols (MockStripe, mockBillingPortalSessionsCreate,
mockCheckoutSessionsCreate, mockPricesSearch) remain unchanged.
- Move import to top of file - Add blank lines between class members - Update comment about vi.mock hoisting
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/stripe-redirects.unit.test.ts (1)
63-68:⚠️ Potential issue | 🟠 MajorShared
beforeEachreset +it()instead ofit.concurrent()— unresolved from previous reviewAll five tests (lines 75, 87, 100, 120, 140) still use
it()instead ofit.concurrent(), violating the project guideline. Additionally, the sharedbeforeEachmock-reset pattern is fundamentally incompatible with concurrent execution: one test'sbeforeEachor mid-testmockResolvedValuecall can corrupt another test's state.The fix is either to keep
it.concurrent()and move mock setup inline per test (removing the sharedbeforeEach), or to scope mock declarations inside each test body entirely, as suggested in the prior review.As per coding guidelines: "ALL test files run in parallel; use
it.concurrent()instead ofit()to run tests within the same file in parallel for faster CI/CD."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stripe-redirects.unit.test.ts` around lines 63 - 68, Change tests to run concurrently and avoid shared beforeEach mock resets: replace each it() with it.concurrent() and remove the shared beforeEach() that calls mockBillingPortalSessionsCreate.mockReset(), mockCheckoutSessionsCreate.mockReset(), and mockPricesSearch.mockReset(); instead, initialize and reset those mocks inside each test body (or declare/initialize fresh mocks scoped to each it.concurrent() block) so no test-mutating setup is shared across tests (locate usage by the symbols beforeEach, mockBillingPortalSessionsCreate, mockCheckoutSessionsCreate, mockPricesSearch, and the test cases currently using it()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/stripe-redirects.unit.test.ts`:
- Around line 5-7: The three mock variables (mockBillingPortalSessionsCreate,
mockCheckoutSessionsCreate, mockPricesSearch) are declared with const and cause
a temporal dead zone when used inside the vi.mock factory; replace their plain
const declarations with hoisted declarations by assigning them via vi.hoisted()
so they are available to the hoisted vi.mock factory and test scope, and update
any references inside the vi.mock factory and tests to use these hoisted symbols
(ensure the names match exactly).
---
Duplicate comments:
In `@tests/stripe-redirects.unit.test.ts`:
- Around line 63-68: Change tests to run concurrently and avoid shared
beforeEach mock resets: replace each it() with it.concurrent() and remove the
shared beforeEach() that calls mockBillingPortalSessionsCreate.mockReset(),
mockCheckoutSessionsCreate.mockReset(), and mockPricesSearch.mockReset();
instead, initialize and reset those mocks inside each test body (or
declare/initialize fresh mocks scoped to each it.concurrent() block) so no
test-mutating setup is shared across tests (locate usage by the symbols
beforeEach, mockBillingPortalSessionsCreate, mockCheckoutSessionsCreate,
mockPricesSearch, and the test cases currently using it()).
| const mockBillingPortalSessionsCreate = vi.fn() | ||
| const mockCheckoutSessionsCreate = vi.fn() | ||
| const mockPricesSearch = vi.fn() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest vi.hoisted() vi.mock factory temporal dead zone
💡 Result:
In Vitest, vi.mock() is hoisted to the top of the file and runs before all static imports (Vitest rewrites imports to make this possible). That also means the mock factory is evaluated before your file’s top-level const/let initializers run, so referencing a later-declared binding from inside the factory can throw a TDZ error like:
ReferenceError: Cannot access 'x' before initialization(temporal dead zone) [1].
Why it happens
This fails because mySpy is still in TDZ when the hoisted mock factory runs:
const mySpy = vi.fn()
vi.mock('./dep', () => ({ fn: mySpy })) // TDZ -> ReferenceErrorVitest explicitly warns that you “cannot use any variables inside the factory that are defined outside the factory” because of hoisting [1].
Fix option A (recommended): vi.hoisted()
Define the values you need for the factory inside vi.hoisted() (declared before vi.mock) and then reference them:
import { vi } from 'vitest'
const mocks = vi.hoisted(() => ({
mySpy: vi.fn(),
}))
vi.mock('./dep', () => ({
fn: mocks.mySpy,
}))This is the documented way to “reference some variables outside of its scope” for a hoisted vi.mock() [1] and is a common solution in practice [2].
Fix option B: vi.doMock() (not hoisted)
If you need to reference normal top-level variables, use vi.doMock() (it is not hoisted), with the tradeoff that it only affects subsequent imports [1]:
const mySpy = vi.fn()
vi.doMock('./dep', () => ({ fn: mySpy }))
const { fn } = await import('./dep') // import after doMockSources: Vitest vi.mock / vi.hoisted API docs [1], example solution pattern [2].
[1] (vitest.dev)
[2] (stackoverflow.com)
Citations:
- 1: https://vitest.dev/api/vi?utm_source=openai
- 2: https://stackoverflow.com/questions/76836909/referenceerror-cannot-access-mock-before-initialization-when-using-vitest?utm_source=openai
🏁 Script executed:
cd /tmp && find . -name "stripe-redirects.unit.test.ts" 2>/dev/null | head -5Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
find . -type f -name "stripe-redirects.unit.test.ts" 2>/dev/nullRepository: Cap-go/capgo
Length of output: 93
🏁 Script executed:
cat -n ./tests/stripe-redirects.unit.test.ts | head -80Repository: Cap-go/capgo
Length of output: 2613
Fix temporal dead zone error by wrapping mock variables in vi.hoisted()
vi.mock() is hoisted by Vitest before module-level code executes, causing the const declarations at lines 5–7 to be in their temporal dead zone when the factory closure runs. This triggers a ReferenceError: Cannot access 'x' before initialization. Vitest's documentation explicitly prescribes vi.hoisted() as the solution for variables shared between a vi.mock factory and test scope.
Fix using `vi.hoisted()`
+const { mockBillingPortalSessionsCreate, mockCheckoutSessionsCreate, mockPricesSearch } = vi.hoisted(() => ({
+ mockBillingPortalSessionsCreate: vi.fn(),
+ mockCheckoutSessionsCreate: vi.fn(),
+ mockPricesSearch: vi.fn(),
+}))
+
-// Shared mock functions that can be accessed by tests
-const mockBillingPortalSessionsCreate = vi.fn()
-const mockCheckoutSessionsCreate = vi.fn()
-const mockPricesSearch = vi.fn()📝 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.
| const mockBillingPortalSessionsCreate = vi.fn() | |
| const mockCheckoutSessionsCreate = vi.fn() | |
| const mockPricesSearch = vi.fn() | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| import * as stripe from '../supabase/functions/_backend/utils/stripe.ts' | |
| const { mockBillingPortalSessionsCreate, mockCheckoutSessionsCreate, mockPricesSearch } = vi.hoisted(() => ({ | |
| mockBillingPortalSessionsCreate: vi.fn(), | |
| mockCheckoutSessionsCreate: vi.fn(), | |
| mockPricesSearch: vi.fn(), | |
| })) | |
| // vi.mock is hoisted by Vitest before all imports, so MockStripe is in place | |
| // when stripe.ts is evaluated — no manual ordering needed. | |
| vi.mock('stripe', () => { | |
| return { | |
| __esModule: true, | |
| default: class MockStripe { | |
| static createFetchHttpClient = vi.fn().mockReturnValue({}) | |
| billingPortal = { | |
| sessions: { | |
| create: mockBillingPortalSessionsCreate, | |
| }, | |
| } | |
| checkout = { | |
| sessions: { | |
| create: mockCheckoutSessionsCreate, | |
| }, | |
| } | |
| prices = { | |
| search: mockPricesSearch, | |
| } | |
| }, | |
| } | |
| }) | |
| vi.mock('hono/adapter', () => ({ | |
| env: () => ({ | |
| WEBAPP_URL: 'https://capgo.test', | |
| STRIPE_SECRET_KEY: 'sk_test_123', | |
| }), | |
| getRuntimeKey: () => 'node', | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/stripe-redirects.unit.test.ts` around lines 5 - 7, The three mock
variables (mockBillingPortalSessionsCreate, mockCheckoutSessionsCreate,
mockPricesSearch) are declared with const and cause a temporal dead zone when
used inside the vi.mock factory; replace their plain const declarations with
hoisted declarations by assigning them via vi.hoisted() so they are available to
the hoisted vi.mock factory and test scope, and update any references inside the
vi.mock factory and tests to use these hoisted symbols (ensure the names match
exactly).
|
2 participants
The stripe library v19+ uses createFetchHttpClient() which wasn't being mocked. This caused tests to fail with: 'Cannot read properties of undefined (reading 'createFetchHttpClient')'
This fix:
Summary by CodeRabbit