Revert "chore(clerk-js): Convert all Jest tests to Vitest (#6798)"#6819
Revert "chore(clerk-js): Convert all Jest tests to Vitest (#6798)"#6819nikosdouvlis merged 2 commits intomainfrom
Conversation
This reverts commit e5b7eb7.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMigrates packages/clerk-js tests from Vitest to Jest, adding Jest configs and setups, updating scripts/deps, and introducing shared test utilities (render, runFakeTimers). Adjusts ESLint and tsconfigs. Minor production edits in fapiClient query handling and SignInFactorTwoCodeForm. Removes SubscriptionDetails/Checkout context exports. Broad test updates align with Jest and new utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Jest Test
participant Env as TimezoneAwareJSDOMEnvironment
participant Proc as process.env
Note over Test,Env: Jest custom environment for per-suite timezone
Test->>Env: Initialize environment (constructor)
Env->>Proc: Set TZ from docblock @timezone or existing TZ or 'UTC'
Env-->>Test: Base jsdom ready with TZ
Note right of Test: Tests run under configured timezone
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
|
Why not just revert the specific functionality that is not working properly? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/clerk-js/src/ui/utils/test/mockHelpers.ts (1)
1-92: Migration incomplete — replace remaining Vitest imports and ensure Jest is installed
- Found jest config at packages/clerk-js/jest.config.js.
- Many files still import from "vitest" (e.g. packages/clerk-js/src/vitestUtils.ts, packages/clerk-js/src/ui/utils/vitest/mockHelpers.ts and numerous tests under packages/clerk-js/src/**). Replace those imports with Jest equivalents (e.g. @jest/globals) or use Jest globals.
- packages/clerk-js/package.json devDependencies show only @emotion/jest and @swc/jest — no "jest" or "@jest/globals". Add "jest" (or @jest/globals) to the package or ensure the workspace root provides it.
- Verify jest.config.js supports ESM + TypeScript (transformer/settings) and re-run a grep for remaining "vitest" imports.
packages/clerk-js/src/ui/utils/vitest/mockHelpers.ts (5)
10-16: Update type name to reflect Jest migration.The type name
DeepVitestMockedshould be updated to reflect the Jest migration, and the internalMockedtype reference should be updated to use Jest's equivalent.-type DeepVitestMocked<T> = T extends FunctionLike - ? Mocked<T> +type DeepJestMocked<T> = T extends FunctionLike + ? MockedFunction<T> : T extends object ? { - [k in keyof T]: DeepVitestMocked<T[k]>; + [k in keyof T]: DeepJestMocked<T[k]>; } : T;
23-28: Replace Vitest mock function with Jest equivalent.All references to
vi.fn()should be replaced withjest.fn()to complete the Jest migration.const mockProp = <T>(obj: T, k: keyof T, mocks?: MockMap<T>) => { if (typeof obj[k] === 'function') { - const mockFn = mocks?.[k] ?? vi.fn(); - (obj[k] as unknown as ReturnType<typeof vi.fn>) = mockFn; + const mockFn = mocks?.[k] ?? jest.fn(); + (obj[k] as unknown as ReturnType<typeof jest.fn>) = mockFn; } };
46-82: Update function signature and all mock function calls.The function signature and all internal mock function calls need to be updated to use Jest instead of Vitest.
-export const mockClerkMethods = (clerk: LoadedClerk): DeepVitestMocked<LoadedClerk> => { +export const mockClerkMethods = (clerk: LoadedClerk): DeepJestMocked<LoadedClerk> => { // Cast clerk to any to allow mocking properties const clerkAny = clerk as any; mockMethodsOf(clerkAny); if (clerkAny.client) { mockMethodsOf(clerkAny.client.signIn); mockMethodsOf(clerkAny.client.signUp); clerkAny.client.sessions?.forEach((session: ActiveSessionResource) => { const sessionAny = session as any; mockMethodsOf(sessionAny, { exclude: ['checkAuthorization'], mocks: { // Ensure touch mock matches expected signature if available, otherwise basic mock - touch: vi.fn(() => Promise.resolve(session)), + touch: jest.fn(() => Promise.resolve(session)), }, }); if (sessionAny.user) { mockMethodsOf(sessionAny.user); sessionAny.user.emailAddresses?.forEach((m: any) => mockMethodsOf(m)); sessionAny.user.phoneNumbers?.forEach((m: any) => mockMethodsOf(m)); sessionAny.user.externalAccounts?.forEach((m: any) => mockMethodsOf(m)); sessionAny.user.organizationMemberships?.forEach((m: any) => { mockMethodsOf(m); if (m.organization) { mockMethodsOf(m.organization); } }); sessionAny.user.passkeys?.forEach((m: any) => mockMethodsOf(m)); } }); } mockProp(clerkAny, 'navigate'); mockProp(clerkAny, 'setActive'); mockProp(clerkAny, 'redirectWithAuth'); mockProp(clerkAny, '__internal_navigateWithError'); - return clerkAny as DeepVitestMocked<LoadedClerk>; + return clerkAny as DeepJestMocked<LoadedClerk>; };
85-103: Update mock function calls and type signature.All
vi.fn()calls need to be replaced withjest.fn()and the parameter type updated.-export const mockRouteContextValue = ({ queryString = '' }: Partial<DeepVitestMocked<RouteContextValue>>) => { +export const mockRouteContextValue = ({ queryString = '' }: Partial<DeepJestMocked<RouteContextValue>>) => { return { basePath: '', startPath: '', flowStartPath: '', fullPath: '', indexPath: '', currentPath: '', queryString, queryParams: {}, - getMatchData: vi.fn(), - matches: vi.fn(), - baseNavigate: vi.fn(), - navigate: vi.fn(() => Promise.resolve(true)), - resolve: vi.fn((to: string) => new URL(to, 'https://clerk.com')), - refresh: vi.fn(), + getMatchData: jest.fn(), + matches: jest.fn(), + baseNavigate: jest.fn(), + navigate: jest.fn(() => Promise.resolve(true)), + resolve: jest.fn((to: string) => new URL(to, 'https://clerk.com')), + refresh: jest.fn(), params: {}, - } as RouteContextValue; // Keep original type assertion, DeepVitestMocked applied to input only + } as RouteContextValue; // Keep original type assertion, DeepJestMocked applied to input only };
1-103: Remove leftover vitest mock helper and consolidate under test/Found both packages/clerk-js/src/ui/utils/vitest/mockHelpers.ts and packages/clerk-js/src/ui/utils/test/mockHelpers.ts; repo search shows no other imports. Delete packages/clerk-js/src/ui/utils/vitest/mockHelpers.ts and ensure tests import the Jest helper at packages/clerk-js/src/ui/utils/test/mockHelpers.ts.
packages/clerk-js/src/core/__tests__/fapiClient.spec.ts (1)
2-2: Migration to Jest complete but still importing from VitestThe imports are still from
vitestbut according to the PR objectives, this should be migrated to Jest. The import should be updated to use Jest's testing utilities.-import { afterAll, beforeAll, beforeEach, describe, expect, it, type Mock, vi } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from '@jest/globals'; +import type { Mock } from 'jest';Note: You'll also need to update the mock setup and
vi.fn()calls to use Jest'sjest.fn()throughout the file.
🧹 Nitpick comments (32)
packages/clerk-js/src/core/__tests__/clerk.redirects.spec.ts (1)
14-14: Non-functional comment added for Jest compatibility.The comment "Because Jest, don't ask me why..." appears to be added to work around some Jest-specific quirk with the devBrowser mock setup. While functional, this type of comment doesn't provide helpful context for future maintainers about what specific Jest behavior required this workaround.
Consider replacing the comment with a more descriptive explanation:
-// Because Jest, don't ask me why... +// Jest requires explicit mock definitions in this format to avoid hoisting issuespackages/clerk-js/src/core/auth/__tests__/getCookieDomain.spec.ts (1)
15-15: Test function made async without await usage.The test function signature was changed to
asyncbut there are noawaitstatements or asynchronous operations in the test body. This suggests either incomplete migration or unnecessary async declaration.Since there are no async operations in this test, consider removing the
asynckeyword:- it('returns the eTLD+1 domain based on where the cookie can be set', async () => { + it('returns the eTLD+1 domain based on where the cookie can be set', () => {Alternatively, if this change was made to prepare for async operations, document the rationale.
packages/clerk-js/src/ui/utils/vitest/createFixtures.tsx (1)
1-1: Remove the leftover Jest import from the Vitest fixturepackages/clerk-js/src/ui/utils/vitest/createFixtures.tsx — delete the commented line
// import { jest } from '@jest/globals';(this file is the Vitest variant and usesvi; the Jest variant is under src/ui/utils/test/createFixtures.tsx).packages/clerk-js/src/utils/__tests__/captcha.spec.ts (1)
152-152: Verify the necessity of the async beforeEach.The change from
beforeEach(() => {tobeforeEach(async () => {is unnecessary here. Based on Jest documentation, beforeEach can handle asynchronous code, but this particular beforeEach hook only performs synchronous operations (vi.clearAllMocks() and vi.resetModules()). Making it async without awaiting any promises adds overhead and may confuse other developers about the hook's requirements.- beforeEach(async () => { + beforeEach(() => {packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts (3)
353-354: Use type assertion instead of non-null assertion.The ESLint comment disables the no-non-null-assertion rule, but a safer type assertion could be used instead.
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(new URL(resp!.headers.get('location')!).searchParams.get('redirect_url')).toContain('/protected'); + expect(resp?.status).toBe(307); + const location = resp?.headers.get('location'); + expect(location).toBeTruthy(); + expect(new URL(location as string).searchParams.get('redirect_url')).toContain('/protected');
371-374: Use type assertion instead of non-null assertion.Same issue as above. The non-null assertion can be replaced with safer type checking.
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(new URL(resp!.headers.get('location')!).searchParams.get('redirect_url')).toEqual( - 'https://www.clerk.com/hello', - ); + const location = resp?.headers.get('location'); + expect(location).toBeTruthy(); + expect(new URL(location as string).searchParams.get('redirect_url')).toEqual( + 'https://www.clerk.com/hello', + );
391-392: Use type assertion instead of non-null assertion.Third instance of the same pattern. Apply consistent type checking approach.
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(new URL(resp!.headers.get('location')!).searchParams.get('redirect_url')).toBeNull(); + const location = resp?.headers.get('location'); + expect(location).toBeTruthy(); + expect(new URL(location as string).searchParams.get('redirect_url')).toBeNull();packages/clerk-js/vitest.setup.mts (1)
12-91: Remove or document obsolete DOM mocks in Vitest setup.Since this PR is reverting to Jest (as evidenced by the new Jest config files), this Vitest setup file contains DOM mocks that may no longer be needed. The extensive browser API mocks (HTMLCanvasElement, Element.prototype.animate, requestAnimationFrame, etc.) were specifically for Vitest but Jest typically handles these differently.
Consider either:
- Removing this entire file if it's no longer used with the Jest migration
- Adding a comment indicating this is legacy/deprecated
- Migrating the still-needed mocks to the Jest setup files
+// DEPRECATED: This file is part of the legacy Vitest setup. +// Most functionality has been migrated to Jest setup files. +// Consider removing this file once Jest migration is complete. + import '@testing-library/jest-dom/vitest';packages/clerk-js/vitest.config.mts (1)
8-8: Remove unused parameter prefix.The parameter was renamed from
_codetocodebut it's still not used in the function. Consider either using the parameter or keeping the underscore prefix to indicate it's intentionally unused.- transform(code: string, id: string) { + transform(_code: string, id: string) {packages/clerk-js/src/ui/components/OrganizationProfile/__tests__/OrganizationMembers.test.tsx (1)
2-2: Unnecessary Jest globals import with current setup.The import
{ describe } from '@jest/globals'is unnecessary when using the default Jest configuration that provides globals. Thedescribefunction should be available globally.-import { describe } from '@jest/globals';If you need to suppress ESLint warnings about globals, consider updating the ESLint configuration instead.
packages/clerk-js/src/core/__tests__/clerk.test.ts (1)
25-34: Consider clarifying the Jest-specific comment.The comment "Because Jest, don't ask me why..." appears to reference a workaround. If this is a known Jest limitation, consider providing a more descriptive explanation or linking to relevant documentation for future maintainers.
packages/clerk-js/src/ui/utils/vitest/runFakeTimers.ts (2)
1-1: Remove commented-out import.The commented import
// import { jest } from '@jest/globals';should be removed as this is a Vitest-specific utility file.-// import { jest } from '@jest/globals'; import { act } from '@testing-library/react'; import { vi } from 'vitest';
5-11: Consider improving type safety for withAct.The
withActimplementation usesanytypes which bypasses TypeScript's type checking. Consider a more type-safe implementation.-type WithAct = <T>(fn: T) => T; -const withAct = ((fn: any) => - (...args: any) => { - act(() => { - fn(...args); - }); - }) as WithAct; +type WithAct = <T extends (...args: any[]) => any>(fn: T) => T; +const withAct: WithAct = (fn) => + ((...args) => { + act(() => { + fn(...args); + }); + }) as any;packages/clerk-js/src/ui/utils/test/runFakeTimers.ts (3)
5-11: Consider improving type safety for withAct (same as Vitest version).This implementation has the same type safety issue as the Vitest version.
-type WithAct = <T>(fn: T) => T; -const withAct = ((fn: any) => - (...args: any) => { - act(() => { - fn(...args); - }); - }) as WithAct; +type WithAct = <T extends (...args: any[]) => any>(fn: T) => T; +const withAct: WithAct = (fn) => + ((...args) => { + act(() => { + fn(...args); + }); + }) as any;
29-30: Consider removing @ts-expect-error with proper typing.The
@ts-expect-errorcomment suggests a typing issue that could be addressed.- // @ts-expect-error - return res.finally(() => jest.useRealTimers()); + return (res as Promise<any>).finally(() => jest.useRealTimers()) as any;
33-34: Remove unnecessary @ts-ignore.The
@ts-ignoreon the return statement is unnecessary.jest.useRealTimers(); - // @ts-ignore - return; + return undefined as any;packages/clerk-js/jest.setup.ts (1)
36-55: Consider extracting IntersectionObserver mock to reduce redundancy.The IntersectionObserver mock could be simplified using Jest's mock implementation pattern.
- //@ts-expect-error - JSDOM doesn't provide IntersectionObserver, so we mock it for testing - global.IntersectionObserver = class IntersectionObserver { - constructor() {} - - disconnect() { - return null; - } - - observe() { - return null; - } - - takeRecords() { - return null; - } - - unobserve() { - return null; - } - }; + // JSDOM doesn't provide IntersectionObserver, so we mock it for testing + global.IntersectionObserver = jest.fn().mockImplementation(() => ({ + disconnect: jest.fn(), + observe: jest.fn(), + takeRecords: jest.fn(() => []), + unobserve: jest.fn(), + root: null, + rootMargin: '', + thresholds: [], + })) as any;packages/clerk-js/src/ui/components/SignIn/__tests__/ResetPassword.test.tsx (1)
132-132: Consider documenting why this test needs extended timeout.The timeout has been increased to 10000ms. Consider adding a comment explaining why this test requires more time than the default timeout.
- }, 10000); + }, 10000); // Extended timeout due to multiple async validation cyclespackages/clerk-js/src/ui/customizables/__tests__/makeCustomizable.test.tsx (1)
117-127: Inconsistent style assertions within the same test.There's an inconsistency in how style assertions are made. Some tests use
toHaveStyleRulewhile others usegetComputedStyle. Consider standardizing to one approach for consistency.For example, lines 117 and 126 use
getComputedStyle:- expect(getComputedStyle(screen.getByTestId('test')).backgroundColor).toBe('yellow'); + expect(screen.getByTestId('test')).toHaveStyleRule('background-color', 'yellow');- expect(getComputedStyle(screen.getByTestId('test')).backgroundColor).toBe('red'); + expect(screen.getByTestId('test')).toHaveStyleRule('background-color', 'red');Also applies to: 155-165, 195-205, 238-248, 280-290
packages/clerk-js/src/ui/components/UserProfile/__tests__/PasskeysSection.test.tsx (1)
39-40: Consider using assertion helpers instead of non-null assertions.While the ESLint suppression is acceptable, consider using type guards or assertion helpers to maintain type safety instead of relying on non-null assertions.
- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - fixtures.clerk.user!.getSessions.mockReturnValue(Promise.resolve([])); + if (!fixtures.clerk.user) { + throw new Error('User fixture not initialized'); + } + fixtures.clerk.user.getSessions.mockReturnValue(Promise.resolve([]));packages/clerk-js/src/ui/components/UserProfile/__tests__/PasswordSection.test.tsx (2)
552-568: Duplicate test detected.This test appears to be an exact duplicate of the previous test (lines 534-550) with the same name "results in error if the password is too small". The duplicate should be removed.
- it('results in error if the password is too small', async () => { - const { wrapper } = await createFixtures(initConfig); - - await runFakeTimers(async () => { - const { userEvent, getByRole } = render(<PasswordSection />, { wrapper }); - await userEvent.click(getByRole('button', { name: /set password/i })); - await waitFor(() => getByRole('heading', { name: /set password/i })); - - await userEvent.type(screen.getByLabelText(/new password/i), 'test'); - const confirmField = screen.getByLabelText(/confirm password/i); - await userEvent.type(confirmField, 'test'); - fireEvent.blur(confirmField); - await waitFor(() => { - screen.getByText(/or more/i); - }); - }); - });
324-324: Remove or reduce per-test 10s timeoutsThese unit tests declare a 10_000ms test timeout but they mostly await DOM updates or use fake timers — the explicit 10s timeout is likely unnecessary. Remove the per-test timeout (use the default) or lower it (2–5s); where timers are used, advance timers with runFakeTimers/advanceTimers instead of relying on long wall-clock timeouts.
Locations: packages/clerk-js/src/ui/components/UserProfile/tests/PasswordSection.test.tsx — tests ending with "}, 10_000);" at lines 324, 591, 624.
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInFactorOne.test.tsx (1)
143-145: Consider using more specific assertions.The text assertions outside
waitForcould be more specific. Consider wrapping them inexpectstatements or usinggetByTextwhich will throw if not found.- screen.getByText('Use another method'); - expect(screen.queryByText('Or, sign in with another method')).not.toBeInTheDocument(); - screen.getByText(`Email code to ${email}`); + expect(screen.getByText('Use another method')).toBeInTheDocument(); + expect(screen.queryByText('Or, sign in with another method')).not.toBeInTheDocument(); + expect(screen.getByText(`Email code to ${email}`)).toBeInTheDocument();packages/clerk-js/jest.config.js (1)
39-39: Keep the transformIgnorePatterns exception; add a one-line justification in jest.config.js.Verified: packages/clerk-js/package.json lists "@formkit/auto-animate": "^0.8.2" and packages/clerk-js/src/ui/elements/Animated.tsx imports from '@formkit/auto-animate/react'; jest.config.js contains the exception:
transformIgnorePatterns: ['[/\\\\]node_modules[/\\\\](?!(@formkit/auto-animate/react)).+\\.(js|jsx|mjs|cjs|ts|tsx)$'],Action: retain the exception and add a concise comment above it in packages/clerk-js/jest.config.js explaining the concrete reason (e.g., this package ships untranspiled ESM/JSX requiring Jest/Babel transform).
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInFactorTwo.test.tsx (4)
128-137: Consider using constants for timer durations.The hardcoded timer values (15000ms, 30000ms) are used consistently across tests. Consider extracting these to named constants for better maintainability.
+const RESEND_BUTTON_HALF_DELAY = 15000; +const RESEND_BUTTON_FULL_DELAY = 30000; + runFakeTimers(timers => { fixtures.signIn.prepareSecondFactor.mockReturnValueOnce(Promise.resolve({} as SignInResource)); const { getByText } = render(<SignInFactorTwo />, { wrapper }); expect(getByText(/Resend/, { exact: false }).closest('button')).toHaveAttribute('disabled'); - timers.advanceTimersByTime(15000); + timers.advanceTimersByTime(RESEND_BUTTON_HALF_DELAY); expect(getByText(/Resend/, { exact: false }).closest('button')).toHaveAttribute('disabled'); getByText('(15)', { exact: false }); - timers.advanceTimersByTime(15000); + timers.advanceTimersByTime(RESEND_BUTTON_HALF_DELAY); expect(getByText(/Resend/, { exact: false }).closest('button')).not.toHaveAttribute('disabled'); });
200-205: Timeout parameter might be unnecessary.The test has a 10-second timeout specified, but with fake timers and proper async handling, this shouldn't be needed. Consider removing it to use the default Jest timeout.
-}, 10000); +});
289-294: Duplicate timeout parameter.Same as above - the 10-second timeout is likely unnecessary.
-}, 10000); +});
379-385: Another duplicate timeout parameter.Same issue with the explicit timeout.
-}, 10000); +});packages/clerk-js/src/ui/components/UserVerification/__tests__/UVFactorTwo.test.tsx (4)
159-170: Inconsistent interaction patterns - direct clicks vs userEvent.The test uses direct
.click()calls instead ofuserEvent.click(). While this works, it's inconsistent with line 128 whereuserEvent.type()is used. Consider standardizing the interaction approach.For consistency with other tests and better simulation of user interactions, consider using
userEventthroughout:- const { getByText, getByRole } = render(<UserVerificationFactorTwo />, { wrapper }); + const { userEvent, getByText, getByRole } = render(<UserVerificationFactorTwo />, { wrapper }); await waitFor(() => { getByText('Verification required'); getByText('Use another method'); }); await waitFor(() => { - getByText('Use another method').click(); + await userEvent.click(getByText('Use another method')); expect(getByRole('button')).toHaveTextContent('Send SMS code to +3069XXXXXXX2'); expect(getByRole('button')).not.toHaveTextContent('Send SMS code to +3069XXXXXXX1'); });
203-206: Avoid multiple user interactions within a singlewaitForblock.Having multiple click actions inside a
waitForblock can lead to timing issues and make tests flaky. Each interaction should be properly awaited.Split the interactions for better reliability:
- await waitFor(() => { - getByText('Use another method').click(); - getByText('Send SMS code to +3069XXXXXXX2').click(); - }); + await waitFor(() => getByText('Use another method')); + getByText('Use another method').click(); + + await waitFor(() => getByText('Send SMS code to +3069XXXXXXX2')); + getByText('Send SMS code to +3069XXXXXXX2').click();
214-217: Another instance of click withinwaitFor.Similar to the previous comment, this click action should be outside the
waitForblock.- await waitFor(() => { - getByText('Use another method').click(); + await waitFor(() => getByText('Use another method')); + getByText('Use another method').click(); + await waitFor(() => { expect(container).toHaveTextContent('+3069XXXXXXX1'); });
222-222: Consider implementing or removing TODO test.There's a TODO test for the "Get Help" functionality. This should either be implemented or tracked in an issue.
Would you like me to help implement this test or create an issue to track this TODO?
| import { render } from '../../../../testUtils'; | ||
| import { bindCreateFixtures } from '../../../utils/test/createFixtures'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify updated import paths are correct.
The import paths have been updated from Vitest-specific locations to Jest-specific ones. Ensure these new paths exist and contain the expected exports.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the updated import paths exist and export the expected functions
# Check if testUtils exists and exports render function
if [ -f "packages/clerk-js/src/testUtils/index.ts" ] || [ -f "packages/clerk-js/src/testUtils.ts" ]; then
echo "✓ testUtils file exists"
rg -n "export.*render" packages/clerk-js/src/testUtils* || echo "⚠ render export not found in testUtils"
else
echo "❌ testUtils file not found"
fi
# Check if createFixtures exists and exports bindCreateFixtures
if [ -f "packages/clerk-js/src/ui/utils/test/createFixtures.ts" ] || [ -f "packages/clerk-js/src/ui/utils/test/createFixtures/index.ts" ]; then
echo "✓ createFixtures file exists"
rg -n "export.*bindCreateFixtures" packages/clerk-js/src/ui/utils/test/createFixtures* || echo "⚠ bindCreateFixtures export not found"
else
echo "❌ createFixtures file not found"
fiLength of output: 77
Fix missing createFixtures import path
The path ../../../utils/test/createFixtures in packages/clerk-js/src/ui/components/SignIn/__tests__/SignInAccountSwitcher.test.tsx:4 does not exist; ensure the file exporting bindCreateFixtures is placed correctly or update the import to the correct location.
🤖 Prompt for AI Agents
In
packages/clerk-js/src/ui/components/SignIn/__tests__/SignInAccountSwitcher.test.tsx
around lines 3 to 4, the import path ../../../utils/test/createFixtures is
invalid; update the import to the correct module path or relocate the
createFixtures file. Fix by locating the actual file that exports
bindCreateFixtures (e.g., search in packages/clerk-js/src/utils/test or
packages/shared/utils/test) then change the import to the correct relative path
from this test file, or move the createFixtures file into the expected
utils/test directory and adjust exports so bindCreateFixtures is exported and
resolvable by the test.
| fixtures.clerk.handleEmailLinkVerification.mockImplementationOnce( | ||
| await Promise.resolve(() => { | ||
| throw new EmailLinkError(EmailLinkErrorCodeStatus.Expired); | ||
| }), |
There was a problem hiding this comment.
Fix incorrect mock implementation pattern.
The mockImplementationOnce(await Promise.resolve(() => {...})) pattern is incorrect. You're awaiting a resolved promise that contains a function, rather than providing the function directly.
- fixtures.clerk.handleEmailLinkVerification.mockImplementationOnce(
- await Promise.resolve(() => {
- throw new EmailLinkError(EmailLinkErrorCodeStatus.Expired);
- }),
- );
+ fixtures.clerk.handleEmailLinkVerification.mockImplementationOnce(() => {
+ throw new EmailLinkError(EmailLinkErrorCodeStatus.Expired);
+ });Apply the same fix to the second error test case on Lines 69-72.
Also applies to: 69-72
🤖 Prompt for AI Agents
In
packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpEmailLinkFlowComplete.test.tsx
around lines 51-54 (and likewise for lines 69-72), the test incorrectly uses
mockImplementationOnce(await Promise.resolve(() => { throw new
EmailLinkError(EmailLinkErrorCodeStatus.Expired); })), which awaits a resolved
promise wrapping a function instead of supplying the function directly; replace
those patterns with direct mockImplementationOnce(() => { throw new
EmailLinkError(EmailLinkErrorCodeStatus.Expired); }) (and the analogous
replacement for the second test) so the mock receives the function immediately.
| ); | ||
|
|
||
| expect(getComputedStyle(screen.getByTestId('test')).backgroundColor).toBe(computedColors.blue); | ||
| expect(screen.getByTestId('test')).toHaveStyleRule('background-color', knownColors.blue); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify toHaveStyleRule is configured for Jest.
The test assertions have been changed from getComputedStyle(...).backgroundColor to toHaveStyleRule('background-color', ...). Please ensure the jest-styled-components or similar matcher extension is properly configured in the Jest setup.
Also applies to: 44-44, 63-63
🏁 Script executed:
#!/bin/bash
# Check if jest-styled-components or similar is installed and configured
fd -e json "package.json" -x grep -l "jest-styled-components\|@emotion/jest" {} \;
# Check Jest setup files for style rule matchers
fd -e ts -e js "jest.setup" -x grep -l "toHaveStyleRule" {} \;Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Top-level files ----"
ls -la | sed -n '1,200p' || true
echo
echo "---- Search: toHaveStyleRule ----"
rg -n --hidden -S "toHaveStyleRule" -g '!*node_modules/*' || true
echo
echo "---- Search: jest-styled-components / @emotion/jest ----"
rg -n --hidden -S "jest-styled-components|@emotion/jest" -g '!*node_modules/*' || true
echo
echo "---- Search: common jest setup keys / filenames ----"
rg -n --hidden -S "setupFilesAfterEnv|setupTests|jest.setup|setup-jest" -g '!*node_modules/*' || true
echo
echo "---- Find likely setup files (js/ts) ----"
fd -HI -e js -e ts -e mjs -e cjs -e jsx "setupTests|jest.setup|setup-jest" || true
echo
echo "---- Search package.json fields for jest/config related entries ----"
rg -n --hidden -S '"setupFilesAfterEnv"|"setupTests"|"jest"' package.json packages/**/package.json || true
echo
echo "---- Print relevant parts of package.json files (show matching deps) ----"
for f in package.json packages/**/package.json; do
if [ -f "$f" ]; then
echo
echo "===== $f ====="
python - "$f" <<'PY'
import json,sys
p=sys.argv[1]
with open(p) as fh:
j=json.load(fh)
for section in ("dependencies","devDependencies","peerDependencies","optionalDependencies"):
if section in j and isinstance(j[section],dict):
out=[]
for k,v in j[section].items():
if any(s in k for s in ("jest-styled-components","@emotion/jest","styled-components","@testing-library","jest-dom","@testing-library/jest-dom","enzyme","react-test-renderer")):
out.append((k,v))
if out:
print(section)
for k,v in out:
print(" ",k,":",v)
if "jest" in j and isinstance(j["jest"],dict):
jest=j["jest"]
if "setupFilesAfterEnv" in jest:
print("jest.setupFilesAfterEnv:", jest["setupFilesAfterEnv"])
PY
fi
done
echo
echo "---- List jest config files if present ----"
fd -HI "jest.config*" || true
echo
echo "---- Done ----"Length of output: 5661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== Root jest.setup-after-env.ts ====="
sed -n '1,200p' jest.setup-after-env.ts || true
echo
echo "===== packages/clerk-js/jest.setup-after-env.ts ====="
sed -n '1,200p' packages/clerk-js/jest.setup-after-env.ts || true
echo
echo "===== packages/clerk-js/jest.setup.ts ====="
sed -n '1,200p' packages/clerk-js/jest.setup.ts || true
echo
echo "===== makeCustomizable.test.tsx ====="
sed -n '1,200p' packages/clerk-js/src/ui/customizables/__tests__/makeCustomizable.test.tsx || trueLength of output: 8204
Register the toHaveStyleRule matcher
Tests use toHaveStyleRule, which ships with jest-styled-components (not Jest or Jest DOM by default). Install jest-styled-components and import it in your package’s Jest setup—e.g. in packages/clerk-js/jest.setup-after-env.ts add:
import 'jest-styled-components';🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/customizables/__tests__/makeCustomizable.test.tsx
around line 26 the test uses toHaveStyleRule (from jest-styled-components) but
the matcher isn’t registered; install jest-styled-components as a dev dependency
and import it in the package Jest after-env setup (e.g. add an import
'jest-styled-components' to packages/clerk-js/jest.setup-after-env.ts or
whichever file is configured in jest.config) so the custom matcher is available
to tests.
| // import { jest } from '@jest/globals'; | ||
| import type { LoadedClerk } from '@clerk/types'; | ||
| import type { ActiveSessionResource } from '@clerk/types'; |
There was a problem hiding this comment.
Update file path and imports to reflect Jest migration.
The file is still located in /vitest/ directory and imports from Vitest, but the PR is reverting from Vitest back to Jest. The imports and directory structure should be updated to align with Jest.
-// import { jest } from '@jest/globals';
-import type { LoadedClerk } from '@clerk/types';
-import type { ActiveSessionResource } from '@clerk/types';
+import { jest } from '@jest/globals';
+import type { ActiveSessionResource, LoadedClerk } from '@clerk/types';The file should also be moved from vitest/ to jest/ or a more generic test/ directory to reflect the migration.
📝 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.
| // import { jest } from '@jest/globals'; | |
| import type { LoadedClerk } from '@clerk/types'; | |
| import type { ActiveSessionResource } from '@clerk/types'; | |
| import { jest } from '@jest/globals'; | |
| import type { ActiveSessionResource, LoadedClerk } from '@clerk/types'; |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/utils/vitest/mockHelpers.ts lines 1-3, the file path
and imports still reference Vitest; move the file to
packages/clerk-js/src/ui/utils/jest/mockHelpers.ts (or
packages/clerk-js/src/ui/utils/test/mockHelpers.ts for a generic location),
update any import lines that reference Vitest to use Jest equivalents (uncomment
or restore import from '@jest/globals' if needed), update any
test-runner-specific helpers to their Jest counterparts, and update all project
references/import paths to the new location.
| // import { jest } from '@jest/globals'; | ||
| import type { LoadedClerk } from '@clerk/types'; | ||
| import type { ActiveSessionResource } from '@clerk/types'; | ||
| import { type Mocked, vi } from 'vitest'; |
There was a problem hiding this comment.
Replace Vitest imports with Jest equivalents.
The import is still using Vitest's vi and Mocked types, which should be replaced with Jest equivalents since this PR reverts back to Jest.
-import { type Mocked, vi } from 'vitest';
+import type { MockedFunction } from 'jest';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/utils/vitest/mockHelpers.ts around line 4, the file
currently imports Vitest's vi and Mocked types; since we're reverting to Jest,
replace that import with Jest equivalents (remove vi and import the Mocked type
from Jest's types or jest-mock package) and update any usage of vi in the file
to use the global jest API (e.g., jest.fn(), jest.spyOn(), jest.resetAllMocks(),
etc.). Ensure the import line uses the correct Jest type source (for example
'jest' or 'jest-mock') and that all mocks in the file call jest instead of vi.
| include: ['**/*.spec.?(c|m)[jt]s?(x)'], | ||
| exclude: ['sandbox/**/*.spec.?(c|m)[jt]s?(x)'], |
There was a problem hiding this comment.
🧩 Analysis chain
Verify spec pattern alignment with actual test files.
The test patterns have been updated to target .spec. files instead of .test. files, which aligns with the Jest migration. However, ensure all test files have been renamed accordingly.
🏁 Script executed:
#!/bin/bash
# Description: Verify test file naming patterns align with config
echo "Checking for .test. files that might need renaming to .spec.:"
fd -e ts -e tsx -e js -e jsx "\\.test\\." packages/clerk-js/src/
echo -e "\nChecking for existing .spec. files:"
fd -e ts -e tsx -e js -e jsx "\\.spec\\." packages/clerk-js/src/
echo -e "\nVerifying the sandbox exclusion pattern:"
fd -e ts -e tsx -e js -e jsx "\\.spec\\." packages/clerk-js/src/sandbox/ || echo "No spec files found in sandbox (expected)"Length of output: 11006
Action required — align Vitest include pattern with test filenames
Vitest config currently only includes .spec files, but many .test files remain in packages/clerk-js/src (examples: packages/clerk-js/src/core/tests/clerk.test.ts, packages/clerk-js/src/ui/components/Checkout/tests/Checkout.test.tsx, packages/clerk-js/src/ui/components/SignIn/tests/SignInFactorOne.test.tsx). packages/clerk-js/src/sandbox/ does not exist, so the exclude is a no-op.
- Either rename remaining .test. → .spec. under packages/clerk-js/src,
- Or update packages/clerk-js/vitest.config.mts (lines ~55–56) to also match .test (e.g. include: ['**/*.{spec,test}.?(c|m)[jt]s?(x)']).
🤖 Prompt for AI Agents
In packages/clerk-js/vitest.config.mts around lines 55–56, the include pattern
only matches *.spec.* files while the repo contains many *.test.* files (and the
sandbox exclude path does not exist); update the include to also match test
files (for example change to include a pattern that covers both spec and test
files like **/*.{spec,test}.?(c|m)[jt]s?(x)) and either remove or correct the
sandbox exclude entry if it's a no-op, so Vitest discovers and runs the existing
tests.
nikosdouvlis
left a comment
nikosdouvlis
left a comment
There was a problem hiding this comment.
Lets hold off reverting this for the time being
|
Merging to unblock #6755 |
6 participants
This reverts commit e5b7eb7.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores