fix(test-teardown): harden playwright suite teardown against CI leaks - #412
Merged
Conversation
Closes the first tranche of #389's teardown audit (epic #409): - extension-testkit, embed suite helpers, and probe-suite share one bounded-close/settle-all teardown (browser?.close() raced against a 30s deadline so a wedged CDP close can't eat the hook, remaining steps still run and settle before the first failure is rethrown). The three near-identical browser+kit+host suite lifecycles are now one shared manageBrowserSuite in extension-testkit. - afterAll hooks get an explicit timeout matching their paired beforeAll's, so vitest's default hook timeout can't cut teardown short. - serveStaticDir's static host adopts packages/serve's bounded graceful-close + closeAllConnections force-close pattern. - get-extension-test-api dispose() settles browser/host/engine teardown independently instead of aborting on the first rejection. - apps/conciv/vitest.config.ts restates testTimeout/hookTimeout on the unit project explicitly, matching ui-kit-system's pattern; cross-project inheritance in vitest is not guaranteed. chromium.launch() itself needs no compensation: playwright's _launchProcess already kills any spawned process before rethrowing a launch failure (verified in playwright-core's coreBundle.js). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe repository adds a shared browser fixture package. Integration tests now use file-scoped browser, site, core, and host fixtures. Teardown runs concurrently with bounded cleanup. Unit tests use CI-configured test and hook timeouts. ChangesBrowser test fixture migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TestFile
participant Fixtures
participant Browser
participant WranglerDev
participant StaticServer
TestFile->>Fixtures: request scoped test resources
Fixtures->>Browser: launch Chromium
Fixtures->>WranglerDev: start site when required
Fixtures->>StaticServer: serve host when required
TestFile->>Fixtures: run integration assertions
Fixtures->>Browser: close browser
Fixtures->>WranglerDev: stop site
Fixtures->>StaticServer: close gracefully or force-close
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
packages/embed/test/** is being migrated to @playwright/test as its runner by another agent — page/browser lifecycle there will be fixture-owned, so the vitest-side hook-timeout/settle-all hardening in suite.ts and probe-suite.ts is obsolete and would collide. Revert those two files to main; extension-testkit's shared bounded-close helper (manageBrowserSuite/settleTeardown/boundedClose) stays, used by widget-suite.ts and get-extension-test-api.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s + p-timeout
Reworks the widget-suite teardown hardening onto vitest 4's native
test.extend() fixtures instead of hand-rolled orchestration, per
review feedback that manageBrowserSuite/settleTeardown-as-orchestrator/
suiteTeardown duplicated a primitive vitest already has.
widgetComponentSuite(opts) now declares browser/kit/host as
{scope: 'file'} fixtures (boots once per file, same as the old
beforeAll) instead of manually wiring beforeAll/afterAll + accessor
closures. Fixture dependency resolution gives "teardown runs even
when a test fails" and correct close ordering for free — verified
against @vitest/runner@4.1.10's chunk-artifact.js: file-scoped
fixture cleanup always runs from runSuite's try/finally
(~3168-3174), regardless of test outcome.
That source read also surfaced two DISTINCT teardown-timeout
subsystems in vitest 4.1.10, which is why one bounded call survives:
- aroundAll/aroundEach hooks get a dedicated per-hook teardown timer
(createTimeoutPromise in callAroundHooks, ~2666-2779), sourced from
hookTimeout via getAroundHookTimeout (~904).
- test.extend() fixture cleanup (what browser/kit/host use here) has
no such wrapper: callFixtureCleanup (~251) is a bare reverse loop
over cleanup callbacks, and its call site has no timeout either.
A hung browser.close() inside a file-scoped fixture's teardown would
therefore hang the whole file/worker forever with nothing to stop
it. That's the one place bounded-close-style plumbing is provably
still needed, so it stays — implemented with p-timeout (already
resolved at 7.0.1 in the lockfile via p-queue, pinned exact as a
direct devDependency) instead of a hand-rolled timer/race.
bounded-close.ts is renamed settle-teardown.ts and reduced to just
settleTeardown, still used by get-extension-test-api.ts's dispose()
(not vitest-hook-based, fixtures don't apply there).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 `@packages/extension-testkit/src/settle-teardown.ts`:
- Line 2: Update the teardown execution around the results collection so each
callback in steps starts through a promise boundary before invocation, ensuring
synchronous throws become settled rejections and Promise.allSettled attempts
every teardown step independently. Preserve the existing all-settled result
handling and disposal order.
🪄 Autofix
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 Plus
Run ID: eccc0431-c559-46de-8674-0eeebfbf2814
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/conciv/vitest.config.tspackages/extension-testkit/package.jsonpackages/extension-testkit/src/get-extension-test-api.tspackages/extension-testkit/src/settle-teardown.tspackages/extension-testkit/src/widget-suite.ts
| @@ -0,0 +1,5 @@ | |||
| export async function settleTeardown(steps: Array<() => Promise<void>>): Promise<void> { | |||
| const results = await Promise.allSettled(steps.map((step) => step())) | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Start every teardown callback through a promise boundary.
Line 2 invokes callbacks while map builds its array. If one callback throws before it returns a promise, later callbacks do not start and Promise.allSettled is not reached. This can leave the host or engine running.
Proposed fix
- const results = await Promise.allSettled(steps.map((step) => step()))
+ const results = await Promise.allSettled(steps.map((step) => Promise.resolve().then(step)))Based on PR objectives: disposal must attempt browser, host, and engine cleanup independently.
📝 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 results = await Promise.allSettled(steps.map((step) => step())) | |
| const results = await Promise.allSettled(steps.map((step) => Promise.resolve().then(step))) |
🤖 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 `@packages/extension-testkit/src/settle-teardown.ts` at line 2, Update the
teardown execution around the results collection so each callback in steps
starts through a promise boundary before invocation, ensuring synchronous throws
become settled rejections and Promise.allSettled attempts every teardown step
independently. Preserve the existing all-settled result handling and disposal
order.
omridevk
added a commit
that referenced
this pull request
Aug 10, 2026
…named per call site Replace the hand-rolled deadline() helper with p-timeout at every call site (sessions.resolve, chat.subscribe/permissionDecision, approval pump drain, until()'s hang guard), keeping the same named labels. TESTKIT_DEADLINE_MS stays as a constants-only export. Revert the extension-testkit deadline wiring: #412 owns that package now and would conflict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the shared browser fixture Extends the vitest fixture rework to every other test file in the repo that still hand-launched chromium via beforeAll/afterAll, per coverage-gap inventory on #389 (shard-2). New @conciv/extension-testkit/browser-fixture exports a minimal `test` with just a {scope: 'file'} browser fixture (bounded close via p-timeout, same as widget-suite.ts) so consumers that only need a browser+page get that shape without dragging kit/host along. widget-suite.ts now builds on this shared base instead of duplicating the browser fixture. Migrated: - packages/extensions/tanstack/test/connect-parity.it.test.ts: kit, host and the already-connected page are file-scoped fixtures layered on the shared browser fixture. - apps/conciv/test/transport-standalone.it.test.ts (found via the repo-wide chromium.launch() grep, not itself in the original four-file list, but needed to make the "zero outside extension-testkit" claim true): kit, the proxy, and the static app host are file-scoped fixtures. - apps/site/test/{live-connect,mobile-gating}.it.test.ts: a new local apps/site/test/site-fixture.ts factors the shared startWranglerDev/stop() lifecycle (already SIGTERM->SIGKILL safe, no p-timeout needed there) into an auto file-scoped `site` fixture layered on the shared browser fixture, reused by both files. live-connect's per-test `engine` stays a plain module-level variable with its existing afterAll safety net: it's created and torn down inside a single test body, not a file-scoped resource. apps/site gains @conciv/extension-testkit as a devDependency (verified non-circular: extension-testkit does not depend on site). NOT migrated: packages/ui-kit-system/test/reduced-motion.it.test.ts. extension-testkit depends on @conciv/ui-kit-system, so adding extension-testkit as a devDependency of ui-kit-system would be circular (ui-kit-system -> extension-testkit -> ui-kit-system). Left as a hand-managed chromium.launch() suite; flagging for a follow-up that extracts a browser-only fixture into a package neither side already depends on, or moves it under extension-testkit in the other direction. Repo-wide `grep -rln "chromium.launch()"` after this change: only extension-testkit/src (in-scope), packages/embed/test/** (excluded, migrating to @playwright/test in a parallel PR), packages/ui-kit-system/test/reduced-motion.it.test.ts (excluded, circular dependency, see above), and extensions/recorder/src/server/render.ts (excluded, product code). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s leaf, migrate reduced-motion onto it
The extension-testkit-hosted browser fixture was itself a dead end:
extension-testkit depends on @conciv/harness-testkit, and
harness-testkit depends on @conciv/extension, which depends on
@conciv/ui-kit-system - so hosting it in either extension-testkit or
harness-testkit put ui-kit-system one or two hops from a real cycle
the moment it took a devDependency on the fixture. Confirmed by
turbo's own cycle detector on the harness-testkit attempt:
Cyclic dependency detected:
@conciv/ui-kit-system#build, @conciv/extension#build,
@conciv/harness-testkit#build
New packages/browser-fixture (@conciv/browser-fixture, private,
manifest/build/tsconfig shape mirrors @conciv/vitest-config and
@conciv/harness-testkit: no build step, exports point straight at
src/*.ts, no test script since it has no tests of its own - it's
exercised by every consumer's own suite): playwright ^1.61.1 +
p-timeout ^7.0.1 as dependencies, vitest ^4.1.8 the same way
extension-testkit declares it (no peerDependency-for-vitest
precedent exists anywhere else in the repo to match instead). Zero
@conciv/* dependencies by construction, so nothing downstream of it
can cycle back.
extension-testkit's browser-fixture.ts is deleted; its four
consumers (widget-suite.ts, connect-parity.it.test.ts,
transport-standalone.it.test.ts, site-fixture.ts) now import
@conciv/browser-fixture directly - one canonical import path, not
two. apps/site's now-unused @conciv/extension-testkit devDependency
(only needed for the old import path) is removed.
packages/ui-kit-system/test/reduced-motion.it.test.ts migrates onto
the shared browser fixture (devDep @conciv/browser-fixture): the
hand-rolled beforeAll/afterAll chromium.launch()/browser.close()
lifecycle is gone, each test destructures {browser} and opens its
own page inline (each test needs a different reducedMotion context
option, so a shared page fixture doesn't fit - matches the file's
original per-test page shape).
Repo-wide `grep -rln "chromium.launch()"` after this change: only
packages/browser-fixture/src/browser-fixture.ts (the fixture itself)
and packages/extension-testkit/src/launch.ts (get-extension-test-api's
page launcher, previously verified to need no compensation - playwright
already guarantees process cleanup on launch failure) remain as
in-scope implementation call sites. The only two exclusions left:
packages/embed/test/** (migrating to @playwright/test in a parallel
PR) and extensions/recorder/src/server/render.ts (product code).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces shared Vitest browser fixtures and migrates Playwright integration suites toward fixture-owned teardown.
Changes:
- Adds a reusable file-scoped Chromium fixture with timeout handling.
- Migrates browser suites and site infrastructure to Vitest fixtures.
- Adds teardown utilities, server cleanup, dependencies, and timeout configuration.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Records new workspace dependencies. |
packages/browser-fixture/package.json |
Defines the fixture package. |
packages/browser-fixture/tsconfig.json |
Configures fixture typechecking. |
packages/browser-fixture/src/browser-fixture.ts |
Implements browser lifecycle handling. |
packages/extension-testkit/package.json |
Adds the fixture dependency. |
packages/extension-testkit/src/widget-suite.ts |
Migrates widget suites to fixtures. |
packages/extension-testkit/src/settle-teardown.ts |
Adds multi-resource teardown handling. |
packages/extension-testkit/src/get-extension-test-api.ts |
Uses settled teardown and exports host types. |
packages/extensions/tanstack/package.json |
Adds the fixture dependency. |
packages/extensions/tanstack/test/connect-parity.it.test.ts |
Migrates parity testing to fixtures. |
packages/ui-kit-system/package.json |
Adds the fixture dependency. |
packages/ui-kit-system/test/reduced-motion.it.test.ts |
Migrates reduced-motion tests. |
apps/site/package.json |
Adds the fixture dependency. |
apps/site/test/site-fixture.ts |
Adds a shared site fixture. |
apps/site/test/mobile-gating.it.test.ts |
Uses the shared site fixture. |
apps/site/test/live-connect.it.test.ts |
Uses the shared site fixture. |
apps/conciv/package.json |
Adds the fixture dependency. |
apps/conciv/vitest.config.ts |
Restates unit-project timeouts. |
apps/conciv/test/transport-standalone.it.test.ts |
Migrates transport tests to fixtures. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+13
to
+16
| await pTimeout(browser.close(), { | ||
| milliseconds: BROWSER_CLOSE_TIMEOUT_MS, | ||
| message: `browser.close did not settle within ${BROWSER_CLOSE_TIMEOUT_MS}ms; a wedged CDP connection would otherwise hang fixture cleanup forever (vitest test.extend cleanup is unbounded)`, | ||
| }) |
| await page.getByRole('button', {name: 'Open conciv chat'}).click({timeout: 30_000}) | ||
| await completeConnectHandshake(page, kit.base) | ||
| await use(page) | ||
| await page.close() |
| site: [ | ||
| // oxlint-disable-next-line no-empty-pattern -- vitest's fixture parser requires the literal `{}` destructuring | ||
| async ({}, use) => { | ||
| const site = await startWranglerDev(options) |
| expect(loaderData.server.greeting).toBe('hello') | ||
| expect(loaderData.local.n).toBe(42) | ||
| }, | ||
| CONNECT_SETUP_TIMEOUT_MS, |
| await tab.page.close() | ||
| } | ||
| }, | ||
| SUITE_SETUP_TIMEOUT_MS, |
| @@ -0,0 +1,5 @@ | |||
| export async function settleTeardown(steps: Array<() => Promise<void>>): Promise<void> { | |||
| const results = await Promise.allSettled(steps.map((step) => step())) | |||
This was referenced Aug 12, 2026
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
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
PR 1 of 2 for #389 (epic #409). Closes the audit findings on playwright teardown leaks under CI, following the reference pattern already used in
apps/site/test(optional-chained closes, SIGTERM→SIGKILL escalation).Scope note:
packages/embed/test/**is being migrated to@playwright/testas its runner by another agent (page/browser lifecycle there will be fixture-owned), so this PR is scoped to everything else.Design (final): built on vitest 4's native
test.extend()fixtures instead of hand-rolledbeforeAll/afterAllorchestration — fixtures already give "teardown runs even when a test fails" and correct dependency-based close ordering for free.Reading
@vitest/runner@4.1.10'schunk-artifact.jssurfaced two distinct teardown-timeout subsystems:aroundAll/aroundEachhooks get a dedicated per-hook teardown timer (createTimeoutPromiseinsidecallAroundHooks, sourced fromhookTimeout), buttest.extend()fixture cleanup has no such wrapper —callFixtureCleanupis a bare reverse loop over cleanup callbacks with no timeout, and its call site inrunSuite'sfinallyblock doesn't wrap it either. So a hungbrowser.close()inside a file-scoped fixture's teardown line hangs the whole file/worker forever with nothing in vitest to stop it. That's the one place bounded-close protection is provably needed, implemented withp-timeoutinstead of a hand-rolled timer/race.@conciv/browser-fixture— a zero-@conciv/*-deps leaf packageThe fixture started life inside
extension-testkit, then an attempted move toharness-testkit(to unblockui-kit-system) turned out to be circular too:harness-testkit→@conciv/extension→@conciv/ui-kit-system— confirmed by turbo's own cycle detector:That attempt was reverted (never pushed with the cycle in it). The fixture now lives in a brand new leaf package,
packages/browser-fixture(@conciv/browser-fixture,private: true, manifest/build/tsconfig shape mirrors@conciv/vitest-config/@conciv/harness-testkit— no build step, exports point straight atsrc/browser-fixture.ts, no test script since it has no tests of its own):playwright ^1.61.1+p-timeout ^7.0.1as dependencies,vitest ^4.1.8the same wayextension-testkitalready declares it. Zero@conciv/*dependencies by construction, so nothing downstream can ever cycle back into it.packages/extension-testkit/src/widget-suite.ts:widgetComponentSuite(opts)keeps its existing call signature (solid/preact/react consumers unchanged);browser/kit/hostare{scope: 'file'}fixtures,browserfrom@conciv/browser-fixture.serveStaticDir's static host adopts the bounded graceful-close +closeAllConnections()pattern frompackages/serve'scloseServer.packages/extensions/tanstack/test/connect-parity.it.test.ts:kit,host, and the already-connectedpageare file-scoped fixtures layered on@conciv/browser-fixture.apps/conciv/test/transport-standalone.it.test.ts(found via a repo-widechromium.launch()grep, not in the original file list, needed to make the "zero outside the fixture" claim true):kit, the proxy, and the static app host are file-scoped fixtures.apps/site/test/{live-connect,mobile-gating}.it.test.ts:apps/site/test/site-fixture.tsfactorsstartWranglerDev/stop()(already SIGTERM→SIGKILL-safe, nop-timeoutneeded) into anautofile-scopedsitefixture layered on@conciv/browser-fixture, reused by both files.live-connect's per-testenginestays a plain module-level variable with its existingafterAllsafety net — created/torn down inside a single test body, not file-scoped.packages/ui-kit-system/test/reduced-motion.it.test.ts: migrated onto@conciv/browser-fixture(devDep added). The hand-rolledbeforeAll/afterAllchromium lifecycle is gone; each test destructures{browser}and opens its own page inline (each test needs a differentreducedMotioncontext option, so a shared page fixture doesn't fit).apps/conciv/vitest.config.ts: theunitproject restatestestTimeout/hookTimeoutfromciTest()explicitly, matchingpackages/ui-kit-system/vitest.config.ts.chromium.launch()guarding: verified in playwright-core's bundled source that on any launch failure playwright already awaitscloseOrKill()on the spawned process before rethrowing. No compensation code needed forlaunch.tsor the fixture itself.Proof: no cycle
Foreground
turbo run typecheckacross every touched package, full output grepped for cycle warnings:Proof: zero
chromium.launch()outside the fixture/canonical launcherTest plan
pnpm turbo run typecheck --filter=@conciv/browser-fixture --filter=@conciv/extension-testkit --filter=@conciv/extension-tanstack --filter=@conciv/app --filter=site --filter=@conciv/ui-kit-system --filter=@conciv/embed— 45/45 green, no cycle warningpackages/extension-testkit/test/smoke.it.test.ts— passespackages/solid/test/widget.it.test.ts— passes (3/3)packages/extensions/tanstack/test/connect-parity.it.test.ts— passes (1/1)apps/conciv/test/transport-standalone.it.test.ts— passes (2/2)apps/site/test/mobile-gating.it.test.ts— passes (8/8)apps/site/test/live-connect.it.test.ts— passes (2/2)packages/ui-kit-system/test/reduced-motion.it.test.ts— passes (2/2)oxfmt --check/oxlinton all touched filespnpm install --frozen-lockfile— lockfile fully in sync, no driftfallow audit --changed-since main— verdictwarn, zero introduced findings on any touched file (2 unrelated pre-existing findings on files this PR does not touch)🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores