chore(test): pin e2e fixture dependencies and migrate to npm#280
Open
wyattjoh wants to merge 13 commits into
Open
chore(test): pin e2e fixture dependencies and migrate to npm#280wyattjoh wants to merge 13 commits into
wyattjoh wants to merge 13 commits into
Conversation
|
Contributor
Author
|
Stack: wyattjoh/nextjs-pinned-range Part of a stacked PR chain. Do not merge manually. |
wyattjoh
changed the title
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds fixture dependency utilities (scripts/lib/fixture-deps.ts) with tests and integrates them into the fixture refresh flow to validate pinned ranges and resolve specs to exact versions. It migrates test log capture from an AsyncLocalStorage-based API to a hook-style Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes |
…st user lifecycle Introduce createGetFixture() returning a getter bundled with a users API; users created during a test are tracked and auto-cleaned in afterEach. Add runFileExistsTest for verifying clerk init artifacts. Export the Fixture type from fixture-setup. Type-check the e2e source tree via a dedicated test/e2e/tsconfig.json so bun run typecheck covers it. Silence log() unless CLERK_E2E_DEBUG=1 to keep test output clean. Extend the no-console oxlint override to test/e2e/** because the logger is a deliberate console wrapper. Update all fixture test files to the new API, and fold in stray formatter fixes for the vue and astro fixtures.
Move the fixture fields behind a `fixture` property and derive both public-facing types (`FixtureUsers`, `Omit<Fixture, "cleanup">`) from their cleanup-bearing sources via `Omit`. Cleanup remains managed internally by `createGetFixture`'s beforeAll/afterAll/afterEach hooks.
Update the createGetFixture JSDoc and .claude/rules/e2e.md to reflect the createGetFixture rename and add runFileExistsTest to the helper list so future readers see the actual API.
Replace AsyncLocalStorage-backed log capture with a module-level activeCapture slot and a useCaptureLog() lifecycle helper that registers beforeEach/afterEach hooks to install and clear a fresh buffer per test. Tests no longer wrap emitting calls with captured.run(); the entire test body is captured. - lib/log.ts: drop node:async_hooks dependency; expose setActiveCapture and getActiveCapture for harness setup. log.ui() no longer appends a trailing newline so spinner cursor-control writes survive. - test/lib/stubs.ts: replace captureLog() with useCaptureLog(), adding a clear() method for mid-test resets when ignoring setup noise. - test/integration/lib/harness.ts: drop the per-call withCapturedLogs wrapper; setupTest/teardownTest manage the slot directly. - spinner.ts: each line-output log.ui() callsite now includes its own trailing \n; in-place cursor writes (\r, \x1b[?25l, \x1b[?25h) stay raw. Removes the latent double-newline bug after the dirty switch from raw stream.write to log.ui. - cli-program.ts: drop the writeErr newline-stripping workaround now that log.ui passes msg through verbatim. - 33 unit test files: migrate from captureLog().run(() => fn()) to describe-scope useCaptureLog() with plain fn() calls. - .claude/rules/logging.md: document the new test pattern.
Drop the custom scripts/run-tests.ts wrapper in favor of bun test's built-in --parallel mode, which implies --isolate and gives module-state isolation between test files. Expand lint and format coverage to test/e2e/ and add an .oxfmtrc.json that excludes fixture lockfiles. Clean up the e2e fixture helpers: drop the unused description parameter from createTestUser/deleteTestUser, pass dotenv: false to clerkSetup so the CLI's own env-loading is what gets tested, and tidy the per-test user lifecycle and logger surfaces.
wyattjoh
force-pushed
the
wyattjoh/nextjs-pinned-range
branch
from
3e50dc0 to
1514c5e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e/users-list.test.ts`:
- Around line 48-52: The test switched to awaiting Promise.all for
createTestUser but never populates createdIds, so afterAll cleanup doesn't
delete the created users; modify the code that calls createTestUser (the
Promise.all(...) result assigned to users) to extract and push each returned
user id into the existing createdIds array (or replace createdIds with
users.map(u => u.id)), ensuring the afterAll teardown logic that iterates
createdIds will actually delete the created users; reference the createTestUser
call site and the createdIds variable and the afterAll cleanup to make the
change.
🪄 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: CHILL
Plan: Pro
Run ID: d63bd112-853f-4241-b00b-b8d1bcbd7f7a
⛔ Files ignored due to path filters (9)
test/e2e/fixtures/astro/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/nextjs-app-router-next14/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/nextjs-app-router/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/nextjs-pages-router/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/nuxt/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/react-router/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/react/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/tanstack-start/package-lock.jsonis excluded by!**/package-lock.jsontest/e2e/fixtures/vue/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (86)
.claude/rules/e2e.md.claude/rules/logging.md.oxfmtrc.json.oxlintrc.jsonCONTRIBUTING.mdpackage.jsonpackages/cli-core/package.jsonpackages/cli-core/src/cli-program.tspackages/cli-core/src/commands/api/catalog.test.tspackages/cli-core/src/commands/api/index.test.tspackages/cli-core/src/commands/api/interactive.test.tspackages/cli-core/src/commands/api/ls.test.tspackages/cli-core/src/commands/apps/create.test.tspackages/cli-core/src/commands/apps/list.test.tspackages/cli-core/src/commands/auth/login.test.tspackages/cli-core/src/commands/auth/logout.test.tspackages/cli-core/src/commands/billing/index.test.tspackages/cli-core/src/commands/config/pull.test.tspackages/cli-core/src/commands/config/push.test.tspackages/cli-core/src/commands/config/schema.test.tspackages/cli-core/src/commands/deploy/index.test.tspackages/cli-core/src/commands/doctor/context.test.tspackages/cli-core/src/commands/env/pull.test.tspackages/cli-core/src/commands/init/index.test.tspackages/cli-core/src/commands/init/scan.test.tspackages/cli-core/src/commands/link/index.test.tspackages/cli-core/src/commands/open/index.test.tspackages/cli-core/src/commands/orgs/index.test.tspackages/cli-core/src/commands/switch-env/index.test.tspackages/cli-core/src/commands/unlink/index.test.tspackages/cli-core/src/commands/users/create.test.tspackages/cli-core/src/commands/users/list.test.tspackages/cli-core/src/commands/users/menu.test.tspackages/cli-core/src/commands/users/open.test.tspackages/cli-core/src/commands/whoami/index.test.tspackages/cli-core/src/lib/auth-server.test.tspackages/cli-core/src/lib/autoclaim.test.tspackages/cli-core/src/lib/autolink.test.tspackages/cli-core/src/lib/bapi-command.test.tspackages/cli-core/src/lib/first-application.test.tspackages/cli-core/src/lib/keyless.test.tspackages/cli-core/src/lib/log.test.tspackages/cli-core/src/lib/log.tspackages/cli-core/src/lib/spinner.tspackages/cli-core/src/test/integration/lib/harness.tspackages/cli-core/src/test/lib/stubs.tsscripts/lib/fixture-deps.test.tsscripts/lib/fixture-deps.tsscripts/refresh-e2e-fixtures.tsscripts/run-tests.tstest/e2e/astro.test.tstest/e2e/fixtures/astro/README.mdtest/e2e/fixtures/astro/package.jsontest/e2e/fixtures/nextjs-app-router-next14/package.jsontest/e2e/fixtures/nextjs-app-router/package.jsontest/e2e/fixtures/nextjs-pages-router/package.jsontest/e2e/fixtures/nuxt/package.jsontest/e2e/fixtures/react-router/app/routes/home.tsxtest/e2e/fixtures/react-router/package.jsontest/e2e/fixtures/react-router/tsconfig.jsontest/e2e/fixtures/react-router/vite.config.tstest/e2e/fixtures/react/index.htmltest/e2e/fixtures/react/package.jsontest/e2e/fixtures/tanstack-start/AGENTS.mdtest/e2e/fixtures/tanstack-start/README.mdtest/e2e/fixtures/tanstack-start/package.jsontest/e2e/fixtures/vue/index.htmltest/e2e/fixtures/vue/package.jsontest/e2e/fixtures/vue/src/App.vuetest/e2e/fixtures/vue/src/components/HelloWorld.vuetest/e2e/lib/dev-server.tstest/e2e/lib/fixture-setup.tstest/e2e/lib/fixture-test.tstest/e2e/lib/logger.tstest/e2e/lib/test-user.tstest/e2e/lib/types.tstest/e2e/nextjs-app-router-next14.test.tstest/e2e/nextjs-app-router.test.tstest/e2e/nextjs-pages-router.test.tstest/e2e/nuxt.test.tstest/e2e/react-router.test.tstest/e2e/react.test.tstest/e2e/tanstack-start.test.tstest/e2e/tsconfig.jsontest/e2e/users-list.test.tstest/e2e/vue.test.ts
💤 Files with no reviewable changes (4)
- test/e2e/fixtures/react-router/app/routes/home.tsx
- scripts/run-tests.ts
- test/e2e/fixtures/react-router/tsconfig.json
- test/e2e/fixtures/tanstack-start/AGENTS.md
✅ Files skipped from review due to trivial changes (18)
- .oxfmtrc.json
- test/e2e/fixtures/nuxt/package.json
- test/e2e/fixtures/nextjs-pages-router/package.json
- test/e2e/fixtures/astro/package.json
- CONTRIBUTING.md
- test/e2e/fixtures/vue/src/components/HelloWorld.vue
- packages/cli-core/src/commands/users/menu.test.ts
- packages/cli-core/src/commands/doctor/context.test.ts
- test/e2e/fixtures/vue/src/App.vue
- test/e2e/fixtures/vue/index.html
- test/e2e/fixtures/tanstack-start/README.md
- packages/cli-core/src/cli-program.ts
- packages/cli-core/package.json
- test/e2e/fixtures/react/index.html
- test/e2e/fixtures/nextjs-app-router-next14/package.json
- .oxlintrc.json
- packages/cli-core/src/lib/auth-server.test.ts
- test/e2e/fixtures/astro/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- test/e2e/fixtures/nextjs-app-router/package.json
- test/e2e/fixtures/react-router/package.json
- scripts/lib/fixture-deps.test.ts
- test/e2e/lib/types.ts
- scripts/lib/fixture-deps.ts
- scripts/refresh-e2e-fixtures.ts
- test/e2e/fixtures/vue/package.json
Comment on lines
+48
to
+52
| const users = await Promise.all([ | ||
| createTestUser(configDir, { appId: APP_ID }), | ||
| createTestUser(configDir, { appId: APP_ID }), | ||
| createTestUser(configDir, { appId: APP_ID }), | ||
| ]); |
There was a problem hiding this comment.
Track created user IDs or afterAll cleanup is effectively disabled.
createdIds is never populated after switching to explicit Promise.all(...), so teardown does not delete any created users. This leaks test users and can cause cross-run instability.
Suggested fix
test("users list --json returns a { data, hasMore } envelope around the BAPI users", async () => {
- const users = await Promise.all([
- createTestUser(configDir, { appId: APP_ID }),
- createTestUser(configDir, { appId: APP_ID }),
- createTestUser(configDir, { appId: APP_ID }),
- ]);
+ const createTrackedUser = async () => {
+ const user = await createTestUser(configDir, { appId: APP_ID });
+ createdIds.push(user.id);
+ return user;
+ };
+ const users = await Promise.all([createTrackedUser(), createTrackedUser(), createTrackedUser()]);📝 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.
Suggested change
| const users = await Promise.all([ | |
| createTestUser(configDir, { appId: APP_ID }), | |
| createTestUser(configDir, { appId: APP_ID }), | |
| createTestUser(configDir, { appId: APP_ID }), | |
| ]); | |
| const createTrackedUser = async () => { | |
| const user = await createTestUser(configDir, { appId: APP_ID }); | |
| createdIds.push(user.id); | |
| return user; | |
| }; | |
| const users = await Promise.all([createTrackedUser(), createTrackedUser(), createTrackedUser()]); |
🤖 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/e2e/users-list.test.ts` around lines 48 - 52, The test switched to
awaiting Promise.all for createTestUser but never populates createdIds, so
afterAll cleanup doesn't delete the created users; modify the code that calls
createTestUser (the Promise.all(...) result assigned to users) to extract and
push each returned user id into the existing createdIds array (or replace
createdIds with users.map(u => u.id)), ensuring the afterAll teardown logic that
iterates createdIds will actually delete the created users; reference the
createTestUser call site and the createdIds variable and the afterAll cleanup to
make the change.
Remove the now-unused `beforeEach` import from seven cli-core test
files where the empty `beforeEach(() => {})` block was previously
stripped, and the unused `describe` import from `test/e2e/lib/fixture-test.ts`.
These were dormant warnings that surfaced once lint started covering
the full source tree (including `test/e2e/`).
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.
Refreshes the e2e fixture tooling so each scaffolded project is reproducible across runs. Fixtures now pin every non-Clerk dependency to an exact version resolved via
npm view, optionalpinnedDependencyRangesguard scaffolders that drift across majors, and a committedpackage-lock.jsonlets tests install withnpm ci. The refresh and runtime paths swapbunx/bun installfornpx/npm ci, and a newscripts/lib/fixture-deps.tsmodule (with unit tests) houses the resolution, override, and validation logic. The oldpinned: boolean+--forceflag is replaced bypinnedDependencyRanges, and TanStack's@rsbuild/corepeer is now expressed as apackageJsonOverridesentry.