feat(e2e): lazy app init + playwright sharding for integration tests#8019
Open
jacekradko wants to merge 10 commits intomainfrom
Open
feat(e2e): lazy app init + playwright sharding for integration tests#8019jacekradko wants to merge 10 commits intomainfrom
jacekradko wants to merge 10 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 7170adb The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Contributor
📝 WalkthroughWalkthroughAdds per-shard test support by propagating PLAYWRIGHT_SHARD through CI matrices, Playwright config, and artifact names. Converts integration apps to lazy initialization with per-app test.beforeAll (300s timeout), server health checks, adopt-from-state-file logic, and state-file writes guarded by a new file-based acquireProcessLock (exported). Extends state file AppParams with optional clerkFapi and clerkTestingToken and adds a .changeset metadata file. 🚥 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. Comment |
…rkers Workers that adopt a running app from the state file were missing CLERK_FAPI and CLERK_TESTING_TOKEN env vars because clerkSetup() only runs in the worker that acquires the lock. Store the tokens in the state file and set them in process.env when adopting.
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/models/longRunningApplication.ts`:
- Around line 200-205: The destroy() method must be made safe when init() never
ran: check whether the instance fields pid and appDir are set (or truthy) before
calling awaitableTreekill(pid) or fs.rm(appDir, { recursive: true, force: true
}); if neither is present, simply return immediately; if only pid is present
kill the process, if only appDir is present remove the directory, and handle
errors the same way as current implementation. Update the destroy() function in
the LongRunningApplication class to guard uses of pid and appDir and avoid
passing undefined/null into awaitableTreekill or fs.rm.
- Around line 182-191: acquireProcessLock(id) only prevents concurrent
initializers for the same app but multiple different apps can concurrently
perform read-modify-write on the shared JSON via stateFile.addLongRunningApp, so
wrap all stateFile mutations (e.g., stateFile.addLongRunningApp calls in this
file and the similar calls around lines 220-223) with a process-global
state-file lock to serialize access; add or reuse a helper like
acquireStateFileLock/releaseStateFileLock (or a simple in-process mutex/queue or
OS-level file lock) and call it around the read-modify-write sequence so each
addLongRunningApp runs exclusively, then release the lock after the write
completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 741e2985-4098-437b-98d8-b29d485ce7de
📒 Files selected for processing (2)
integration/models/longRunningApplication.tsintegration/models/stateFile.ts
- destroy() now checks pid/appDir before calling awaitableTreekill/fs.rm, safely handling cases where init() never ran or failed - Wrap addLongRunningApp with a file lock to prevent concurrent read-modify-write races when multiple apps initialize in parallel
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/models/longRunningApplication.ts`:
- Line 116: parsePublishableKey can return null, so avoid directly destructuring
its result; in the code around the call to parsePublishableKey(publishableKey)
(where you currently extract instanceType and frontendApi), first capture the
return value into a variable (e.g., parsedKey =
parsePublishableKey(publishableKey)), check if parsedKey is null, and handle
that case (throw a clear error or return early) before using
parsedKey.instanceType and parsedKey.frontendApi; update any callers/logic in
longRunningApplication.ts that rely on instanceType/frontendApi accordingly to
ensure a safe path when parsePublishableKey returns null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f8bc3ceb-3d83-40e0-b52b-5565a1213bc2
📒 Files selected for processing (1)
integration/models/longRunningApplication.ts
| const publishableKey = params.env.publicVariables.get('CLERK_PUBLISHABLE_KEY'); | ||
| const secretKey = params.env.privateVariables.get('CLERK_SECRET_KEY'); | ||
| const apiUrl = params.env.privateVariables.get('CLERK_API_URL'); | ||
| const { instanceType, frontendApi: frontendApiUrl } = parsePublishableKey(publishableKey); |
Contributor
There was a problem hiding this comment.
Handle null return from parsePublishableKey to prevent runtime crash.
parsePublishableKey returns PublishableKey | null. If the publishable key is missing or malformed, this line will throw TypeError: Cannot destructure property 'instanceType' of '...' as it is null.
🐛 Proposed fix
- const { instanceType, frontendApi: frontendApiUrl } = parsePublishableKey(publishableKey);
+ const parsed = parsePublishableKey(publishableKey);
+ if (!parsed) {
+ throw new Error(`Invalid or missing CLERK_PUBLISHABLE_KEY for ${name}`);
+ }
+ const { instanceType, frontendApi: frontendApiUrl } = parsed;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/models/longRunningApplication.ts` at line 116,
parsePublishableKey can return null, so avoid directly destructuring its result;
in the code around the call to parsePublishableKey(publishableKey) (where you
currently extract instanceType and frontendApi), first capture the return value
into a variable (e.g., parsedKey = parsePublishableKey(publishableKey)), check
if parsedKey is null, and handle that case (throw a clear error or return early)
before using parsedKey.instanceType and parsedKey.frontendApi; update any
callers/logic in longRunningApplication.ts that rely on instanceType/frontendApi
accordingly to ensure a safe path when parsePublishableKey returns null.
Use optional chaining on app?.teardown() and thisApp?.teardown() to prevent cascading TypeError when beforeAll fails before assigning app.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/tests/handshake.test.ts`:
- Around line 1078-1080: The first "Client handshake `@generic`" test suite still
calls await app.teardown() unguarded in its afterAll; update that afterAll to
check for app before calling teardown (e.g., if (app) await app.teardown()) just
like the other suites, and ensure any associated jwksServer.close() callback is
preserved; locate the suite by the beforeAll that sets up app and the afterAll
that currently calls app.teardown() and add the same null/undefined guard used
elsewhere in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4bd67bef-63ec-4a58-84b8-d35dc775a20b
📒 Files selected for processing (1)
integration/tests/handshake.test.ts
Comment on lines
1078
to
1080
| test.afterAll('setup local Clerk API mock', async () => { | ||
| await app.teardown(); | ||
| await app?.teardown(); | ||
| return new Promise<void>(resolve => jwksServer.close(() => resolve())); |
Contributor
There was a problem hiding this comment.
The teardown guard is still incomplete in this file.
Lines 1079 and 1463 fix two suites, but the first Client handshake @generic`` suite still does await app.teardown() on Line 70. If its `beforeAll` fails before assigning `app`, this file still throws during `afterAll`, so the PR objective is only partially met. Please apply the same guard there as well.
Also applies to: 1462-1464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/tests/handshake.test.ts` around lines 1078 - 1080, The first
"Client handshake `@generic`" test suite still calls await app.teardown()
unguarded in its afterAll; update that afterAll to check for app before calling
teardown (e.g., if (app) await app.teardown()) just like the other suites, and
ensure any associated jwksServer.close() callback is preserved; locate the suite
by the beforeAll that sets up app and the afterAll that currently calls
app.teardown() and add the same null/undefined guard used elsewhere in this
file.
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.beforeAllintestAgainstRunningAppsPLAYWRIGHT_SHARDenv varHow it works
Before: Global setup starts ALL 13 next.appRouter.* apps in parallel → every test waits for all apps → 8 min total
After: Each shard only initializes the ~4-5 apps its tests actually need → 3 shards run in parallel → ~3-4 min critical path
Lazy init flow
test.beforeAllcallsapp.init()init()checks if app is already running (state file + health check)wxflag)Sharding
PLAYWRIGHT_SHARDenv var (e.g.,"1/3")Test plan
PLAYWRIGHT_SHARDunset returnsundefined)Starting full initvsAdopted from state file)Summary by CodeRabbit