Harden e2e setup/teardown cleanup and scope the BCV nav helper to the top toolbar - #167
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
d7ccbe3 to
c5d7f45
Compare
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
bccfa24 to
7c6823e
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.
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.
alex-rawlings-yyc
changed the title
7c6823e to
1e1da28
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A failed spawn reports itself by emitting 'error' asynchronously, and an unhandled 'error' on an EventEmitter throws as an uncaughtException — outside the try/catch, so cleanUpFailedLaunch never ran and the seeded settings were left in place. Race a listener-backed rejection against the readiness waits in both setups instead. Creating the isolated user-data dir also sat outside its cleanup try, so a failure there left the just-started renderer dev server running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The marker is only written when a run spawns the server, so one left on disk here belongs to a prior run — killing by it could hit a process the OS has since recycled that PID onto.
Ideas (subset of a Devin-inspired, Claude generated list)1. Give The scoping itself is verified correct: The residual is diagnostics: if that testid is renamed upstream the locator resolves to nothing and the failure reads "the top toolbar's book-chapter control never became enabled … the WEB Scripture Editor is likely closed", pointing at the wrong cause. An 2. Drop the duplicated message inside For a normal 3. Enforce the single-backup-file invariant instead of only documenting it — The branch documents that one shared 4. Reconcile the two post-SIGKILL exit-wait budgets — Both tiers restore settings only after killing the app, and both are correct today because SIGKILL leaves no opportunity for an exit-time settings flush — the ordering is what makes them safe, not the wait length. Worth a shared constant or a one-line note on why the budgets differ, since the two paths guard the same file. |
A truncated backup would be restored over the developer's real settings, and the self-heal restore reads it unconditionally. Also name a duplicated test id as a cause in the toolbar-root failure message.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
(My approval is conditional on the e2e checks passing.)
@imnasnainaec reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
2 participants
Three follow-ups to #169, plus one diagnostics improvement. E2e-harness only; no
production/extension code changes. None of this changes pass/fail behavior on a healthy run.
Scope the BCV nav helper to the top toolbar's control
#169 forces
platform.interfaceMode: 'power'for every e2e launch, and Power mode is exactly whatbrings a second kind of BCV control into existence: paranext-core renders a per-tab
BookChapterControlin each web view's own tab toolbar (web-view.component.tsx, gated onisPowerMode) alongside the top toolbar's, and every instance carries the samearia-label="book-chapter-trigger".navigateToScriptureReflooked the trigger up page-wide andtook
.first(), so which control it drives now depends on the toolbar happening to render beforethe dock layout in document order.
The two are not interchangeable. Only the toolbar's control is passed a
disabledprop (bound towhether BCV navigation has a resolved target web view); the per-tab controls receive no such prop
and are therefore always enabled. If the lookup ever resolved to one of those, the helper's
toBeEnabledwait would become vacuous — passing even with no navigation target resolved.The trigger is now scoped to the toolbar's own
toolbar-reserved-space-wrapperroot, with.first()kept only as a strict-mode guard within that single scope. The helper's JSDoc and itsfailure message are corrected too: both still described simple mode as the default, which #169 made
false.
Make setup and teardown cleanup failure-safe
Playwright skips
globalTeardownwhenglobalSetupthrows. #169 added CDP setup failure-pathcleanup, but its try/catch begins at the readiness waits, so everything acquired before them still
leaked on failure.
cleanup extracted as
cleanUpFailedLaunch()— callable at any point after the seed, since anabsent PID simply skips the kill.
finally, so a failed spawn leaves no leaked descriptor.reference to find it by — and stops the dev server the bootstrap started, since that point is past
bootstrapRendererDevServer's own cleanup but before the main try/catch.cleanup with a usable exit signal instead of silently skipping the post-SIGKILL wait.
bootstrapRendererDevServerkills a dev server it started that never became ready, rather thanleaving it to survive the failed run, hold port 1212, and silently serve a stale bundle to every
later run.
killProcessFromPidFileremoves the PID marker independently of the kill (removePidFile), so anfs error there can no longer report a successful kill as
killed: false— which would skip thecaller's wait for that PID to exit and walk straight into a live
SingletonLock.preConfigureSettingsrolls back a partial seed whenbackupAndSeedSettingsthrows, so a mid-seedfilesystem failure can't strand a developer's real settings in a seeded state.
Describe page errors in full
newPlatformErrorreturns a plain object literal, not anError. When one surfaces as an unhandledrejection, Playwright's
pageerrorhands us anErrorwhose.messagehas collapsed to a uselesshusk — which is why the CI log showed
Page error: Objectwith the actual cause nowhere in it.describePageError()combines an error's message, its stack (which often carries the true text for awrapped throwable), and a JSON dump of own-enumerable properties, skipping the empty cases a real
Errorproduces. The smoke fixture'spageerrorlogging uses it.withFatalStartupTripwirematches against that full description rather than.message. To be clearabout what this is worth: it is defensive, not a fix for a known miss. The theme-settle error the
tripwire targets is rejected by
AsyncVariablewith a plain string, so its text does reach.messagetoday and the tripwire already matches it. The change costs nothing and covers a futurefatal signal that arrives wrapped.
Documentation
withFatalStartupTripwirerecords why the boot-timePlatformErrorrejections (e.g. theplatform.getOSPlatform-32601 race) must stay log-only: promoting them to fatal signals was triedand reverted after CI showed that launches emitting them usually recover, so tripping on a
per-launch-recurring signal exhausted the retry budget and turned flaky-but-green runs into hard
failures.
backupAndSeedSettingsdocuments its failure conditions, including that a partial seed may needrolling back by the caller.
SETTINGS_BACKUP_FILErecords why one backup file shared across both tiers is safe (workers: 1,fullyParallel: false, and the tiers run sequentially) and what would break it.This change is
Summary by CodeRabbit