Match the e2e startup tripwire against the full page-error description - #167
Match the e2e startup tripwire against the full page-error description#167alex-rawlings-yyc wants to merge 7 commits into
Conversation
alex-rawlings-yyc
added
🟩Low
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe E2E harness now isolates settings around Electron launches, improves page-error diagnostics and readiness checks, and coordinates renderer, Electron, and user-data cleanup through shared process utilities. ChangesE2E harness lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestHarness
participant Electron
participant ProjectLookupService
participant Renderer
participant Cleanup
TestHarness->>Electron: seed settings and launch
Electron->>Renderer: activate extension and settle layout
TestHarness->>ProjectLookupService: poll project metadata
ProjectLookupService-->>TestHarness: return installed projects
TestHarness->>Cleanup: restore settings and remove artifacts
Cleanup->>Renderer: wait for process exit
Cleanup->>Electron: remove isolated user-data directory
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
alex-rawlings-yyc
changed the title
7f22b73 to
7ed5e0b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 2 discussions.
Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
This comment was marked as outdated.
This comment was marked as outdated.
48d9e06 to
c917ddb
Compare
Playwright wraps a non-Error value thrown in the renderer into a bare `Error` whose `.message` collapses to something useless like "Object", keeping the real text only in the stack. `withFatalStartupTripwire` matched FATAL_STARTUP_PAGE_ERROR against `.message` alone, so a theme-settle failure arriving that way slipped past the tripwire and cost the launch its whole readiness budget instead of fast-failing. Add `describePageError()`, which combines the message, the stack, and a JSON dump of own-enumerable properties, and match the tripwire against that. The smoke fixture's `pageerror` logging uses it too, so the CI log shows the real error rather than "Page error: Object". Also record why the boot-time PlatformError rejections must stay log-only: promoting them to fatal signals was tried and reverted after CI showed launches that emit them usually recover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seeding and launch now run inside the CDP setup's guarded path so a failure can't leak settings or the user-data dir, PID-marker removal no longer masks a successful kill, and the CDP teardown's tail always runs. Also drop stale simple-mode wording now that the suite forces Power mode.
Playwright skips globalTeardown when globalSetup throws, so a dev server spawned here but stuck before readiness survived the failed run, holding port 1212 and serving a stale bundle to later runs.
A throw from the CDP_PID_FILE write left `exited` undefined, so cleanUpFailedLaunch skipped its post-SIGKILL wait and removed the user-data dir while the app still held it.
d7ccbe3 to
c5d7f45
Compare
Power mode renders a per-tab BookChapterControl with the same aria-label and no disabled prop, so a page-wide .first() relied on render order and would have made the enabled-wait vacuous.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e-tests/global-setup.ts (1)
219-266: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPID-file write happens before the failure-cleanup
try, leaking the dev server on write failure.
fs.writeFileSync(DEV_SERVER_PID_FILE, ...)(Lines 231-233) runs before thetryblock that callskillSpawnedDevServeron failure.writeFileSyncthrows synchronously on error (e.g.EACCES/ENOENT), so if it fails, the spawn already succeeded but the function exits without ever reaching the cleanup path — leaking exactly the orphaned dev-server process this PR is meant to prevent.The sibling
globalSetupCdphandles this same class of failure explicitly by wrapping its analogous write in its own try/catch with cleanup, so there's established precedent for guarding it here too.🐛 Proposed fix: move the PID-file write inside the try block
- if (devServer.pid) { - fs.writeFileSync(DEV_SERVER_PID_FILE, String(devServer.pid)); - } - // From here on this run owns a spawned, detached dev server. Playwright skips globalTeardown // when globalSetup throws, so a readiness failure below must kill it on the spot or it survives // the failed run, holds port 1212, and silently serves a stale bundle to every later run. // Scoped to this branch only: the already-running branch above didn't start the server, so this // run must leave it alone. try { + if (devServer.pid) { + fs.writeFileSync(DEV_SERVER_PID_FILE, String(devServer.pid)); + } console.log(`Waiting for renderer dev server on port ${RENDERER_PORT}...`); await waitForPort(RENDERER_PORT, 60_000);🤖 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 `@e2e-tests/global-setup.ts` around lines 219 - 266, Move the DEV_SERVER_PID_FILE write and related renderer readiness operations into the try block that owns cleanup, so synchronous fs.writeFileSync failures also invoke killSpawnedDevServer(devServer.pid) before rethrowing. Keep the existing detached-server setup and readiness behavior unchanged.e2e-tests/global-setup-cdp.ts (1)
358-377: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUnconditional settings restore + marker cleanup on port reuse risks clobbering a concurrently-running owner.
clearStaleOwnershipMarkers()now also callsrestoreSeededSettings()(new in this PR) wheneverglobalSetupCdpdecides to reuse an already-running instance onCDP_PORT(Line 91-102). The code assumes that instance is always a stale leftover from a crashed prior launched run, but the same code path fires just as readily when the port is held by a developer's activenpm run start:cdpsession or by another concurrently-executing test run. In either case, restoringsettings.jsonout from under a live owner (or deletingCDP_PID_FILE/CDP_USER_DATA_FILEit still needs) can corrupt its settings state or break its own teardown.Consider gating this cleanup on actual ownership evidence — e.g. a per-run token recorded alongside the PID marker, or at minimum verifying the PID in
CDP_PID_FILEis no longer alive — before restoring settings or removing markers, rather than acting unconditionally whenever the port happens to be occupied.Based on learnings, "assume cleanup can be triggered by local concurrent runs or stale-marker recovery... implement a setup-established per-run ownership token and require it in teardown to guard: process termination, user-data cleanup, marker deletion, and settings backup restoration—so teardown only performs cleanup when the current run owns the resources/markers."
🤖 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 `@e2e-tests/global-setup-cdp.ts` around lines 358 - 377, Make ownership cleanup conditional in clearStaleOwnershipMarkers and globalTeardownCdp by recording a setup-established per-run ownership token and validating it before restoring seeded settings, terminating processes, deleting user data, or removing marker files. Preserve warm-instance reuse without modifying resources belonging to a developer session or another concurrent test run, and only perform teardown cleanup when the current run’s token matches the recorded ownership metadata.Source: Learnings
🤖 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.
Outside diff comments:
In `@e2e-tests/global-setup-cdp.ts`:
- Around line 358-377: Make ownership cleanup conditional in
clearStaleOwnershipMarkers and globalTeardownCdp by recording a
setup-established per-run ownership token and validating it before restoring
seeded settings, terminating processes, deleting user data, or removing marker
files. Preserve warm-instance reuse without modifying resources belonging to a
developer session or another concurrent test run, and only perform teardown
cleanup when the current run’s token matches the recorded ownership metadata.
In `@e2e-tests/global-setup.ts`:
- Around line 219-266: Move the DEV_SERVER_PID_FILE write and related renderer
readiness operations into the try block that owns cleanup, so synchronous
fs.writeFileSync failures also invoke killSpawnedDevServer(devServer.pid) before
rethrowing. Keep the existing detached-server setup and readiness behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e973eff-d82d-45d6-bd5d-06ba69e48d6c
📒 Files selected for processing (5)
e2e-tests/fixtures/app.fixture.tse2e-tests/fixtures/helpers.tse2e-tests/global-setup-cdp.tse2e-tests/global-setup.tse2e-tests/global-teardown.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/fixtures/app.fixture.ts
1 participant
Problem
withFatalStartupTripwireexists to fast-fail a doomed cold start: when therenderer emits the theme-settle timeout that leaves dock tabs stuck at
"Unknown", waiting out the full readiness budget is pointless because only a
fresh launch recovers it.
But the tripwire matched
FATAL_STARTUP_PAGE_ERRORagainsterr.messagealone, and that is not reliably where the text is. When the page throws a
non-Error value, Playwright's
pageerrorevent wraps it in a bareErrorwhose
.messagecollapses to something useless like"Object"— the realtext survives only in the stack. A fatal error arriving that way slipped
straight past the tripwire, and the launch burned its entire readiness budget
before failing with a misleading timeout instead of the actual cause.
The same flattening made the CI log unhelpful:
Page error: Object.Changes
describePageError()— combines an error's message, its stack (whichoften carries the true text for a wrapped throwable), and a JSON dump of
own-enumerable properties, skipping the empty cases a real
Errorproduces..message, so a fatalerror whose text survives only in the stack now trips it.
pageerrorlogging uses it too, so the CI log showsthe real error instead of the
"Object"husk.withFatalStartupTripwirerecording why the boot-timePlatformErrorrejections (e.g. theplatform.getOSPlatform-32601 race)must stay log-only: promoting them to fatal signals was tried and reverted
after CI showed launches that emit them usually recover, so tripping on a
per-launch-recurring signal exhausted the retry budget and turned
flaky-but-green runs into hard failures.
Notes
Stacked on #169 — please merge that first; this rebases onto
mainas asingle commit afterward.
An earlier revision of this branch also added an in-page
installPageErrorCapture()hook (~100 lines) to log the pre-flattening constructor name and fields. It has
been dropped: it was only ever armed on the smoke path, while the startup
failure that motivated it lives on the CDP path — which #169 now addresses at
the root with real readiness gates (extension activation, project
registration), the first-run overlay bypass, and forced Power interface mode.
This does not change pass/fail behavior on a healthy run. It makes a doomed
start fail fast and legibly instead of slowly and misleadingly.
This change is
Summary by CodeRabbit