Expand e2e testing#151
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds self-managed CDP E2E execution, shared-instance recovery helpers, feature tests, tier-specific CI artifacts, and documentation. It also changes project modal discard guards to include pending edits through a renamed ChangesUnsaved-work modal guarding
E2E test infrastructure and feature coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![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.
alex-rawlings-yyc
force-pushed
the
expand-e2e-testing
branch
from
9e3e8d2 to
66fc3fc
Compare
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 4 discussions.
Reviewable status: 0 of 56 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
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.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
alex-rawlings-yyc
force-pushed
the
expand-e2e-testing
branch
from
ca70ddd to
e9c1bed
Compare
|
When I pull this branch and run When I run |
|
Whoops. I'll have another look. I assume we want |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 0 of 70 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec partially reviewed 53 files and made 1 comment.
Reviewable status: 50 of 73 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
.github/workflows/test.yml line 114 at r9 (raw file):
if ($LASTEXITCODE -eq 0) { exit 0 } Write-Host "npm ci failed (exit $LASTEXITCODE); verifying cache before retry" npm cache verify
Claude suggestion from a Devin nit:
worth a one-liner after npm cache verify (e.g., if ($LASTEXITCODE -ne 0) { Write-Host "npm cache verify failed (exit $LASTEXITCODE)" } for clearer diagnostics)
Exclude the "Open Interlinearizer" project-picker tab from the four broad .dock-tab matches (ensureInterlinearizerOpenOnWeb, isDraftDirty, closeInterlinearizerTab, rescueDraftToNewProject) so they target the real WebView tab. Also anchor the active-project check in ensureE2eProjectActive to match E2E_PROJECT_NAME exactly instead of via unanchored includes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The e2e feature helpers couldn't reliably pass on the shared CDP
instance:
- ensureE2eProjectActive matched the project entry by the button's
accessible
name, but the modal renders name/badge/language spans with no
separating
whitespace, so the anchored regex never matched — every mutating test
fell
into the create branch and re-created an existing project. Match the
name
element by exact text instead (also fixes the "E2E Test Project 2"
overmatch).
- The create branch hung on a dirty draft (Create defers behind the
discard
confirm); dismiss it when dirty.
- closeInterlinearizerTab used hover()+click(), which times out when the
tab
overflows off-viewport on CI; use dispatchEvent('click') like
paranext-core.
- ensureInterlinearizerOpenOnWeb settles the dock before its isVisible()
branch.
- Extract interlinearizerTabLocator/escapeRegExp helpers.
The tab's unsaved marker used dirty||pendingEdits but ProjectModals
gated its
discard confirmation on committed dirty only, so opening/creating a
project
mid-typing silently discarded the uncommitted gloss. Rename the prop to
hasUnsavedWork and feed it the combined state so the guard and marker
agree.
Document the broadened discard-on-swap behavior in user-questions.md.
globalSetup now launches and tears down its own Platform.Bible instance (reusing a warm start:cdp when the CDP port is in use), and .or() tab waits get .first() so a docked Interlinearizer + WEB tab no longer trip Playwright strict mode.
Kill the CDP app's whole process tree (taskkill /T /F on Windows, POSIX process-group SIGKILL elsewhere) so no orphans hold port 8876, and stream the launched app's output to a log instead of discarding it, so a startup crash shows its cause rather than an opaque WebSocket-port timeout.
* Suggested changes on expand-e2e-testing * Fix Linux CI
Self-heal any modal a prior failed test left mounted (dismissLeftoverModals in ensureInterlinearizerOpenOnWeb) so its full-viewport overlay can't intercept every downstream test's clicks, force-click through the wipe modal's settle race, and enable CI retries now that each attempt starts clean. Also race the CDP-port wait against the early-exit sentinel so a late startup crash fails fast instead of timing out.
Disable electron-debug's auto-opened DevTools (ELECTRON_IS_DEV=0) and pin the window to 1280x960 so dock panels and modals are not clipped; gate readiness on dock-tab titles resolving (no "Unknown" tabs) in both waitForAppReady and CDP global setup so a broken instance fails fast with its startup log; upload the dotfile startup log (include-hidden-files) and force X11 on Linux so headless xvfb runs work on Wayland machines.
The e2e job's full core install intermittently fails on windows-latest with EEXIST in the shared npm cache; retry up to 3x with cache verify between attempts. Linux keeps its single-shot install.
The "Open Interlinearizer" picker is a floating dock tab that opening a project never disposes; on the shared CDP instance it accumulated until a bare .select-project-dialog read tripped strict mode and reddened every downstream test. Scope the locator to .first(), close the picker after selecting a project, and self-heal leftover pickers in the open precondition. Also log npm cache verify failures during the Windows ci retry.
navigateToScriptureRef drove the platform toolbar's book-chapter control, which is permanently disabled in simple interface mode with no main-editor project (the CDP instance's state) — so trigger.click() retried for the full test timeout, hanging every navigating feature test for 6 minutes. Wait a bounded time for the control to enable and skip navigation if it never does; the view already shows its default reference and callers assert on tokens.
The dock-readiness gate failed if any tab anywhere said "Unknown", so a single leftover panel poisoned every later feature test on the shared instance. Make it accept a lenient mode (dock mounted + >=1 tab resolved) for CDP feature tests while smoke/cold-start stay strict, report a closed-page teardown distinctly, and capture the smoke app's stdout.
The readiness wait used the 120s cold-start budget on every attempt, so a dead shared instance cost ~6 min per test (120s x 3 retries) before reporting. Add CDP_FEATURE_READY_TIMEOUT (30s), passed by the feature specs, and split waitForAppAndInterlinearizerReady's timeout across both the app-ready and extension-registration waits so the short budget caps the whole check — a dead instance now fails in ~90s. Smoke tests keep the generous cold-start default.
alex-rawlings-yyc
force-pushed
the
expand-e2e-testing
branch
from
63965e3 to
446ddcd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Wait on the settings/menu-data/theme data-provider objects in rpc.discover before the dock-tab-title check, so a slow cold start resolves against its real cause instead of an opaque "Unknown"-tab timeout. Also fast-fail the cold-start wait on the fatal theme-settle page error, turning a 120s dead wait into a fast, labeled failure that retries onto a fresh launch.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 50 of 73 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc and imnasnainaec).
Cut paranext-core internals (serialization shape, magic numbers, symbol names) that go stale silently; keep the observable contract and flag the few comments that still track an upstream detail.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Also fix the smoke launcher's dual stdout/stderr pipe: use { end: false
}
so neither stream ends the shared log early, and flush it before
dumping.
Replaces the manual process-group kill in launchElectronWithExtension and teardownElectronApp with the shared killProcessTree utility, matching both global-teardown files and closing the Windows tree-kill gap.
The dock-tab and service-host waits floored their leftover budget to 1ms (Math.max(1, ...)), which clears waitForDockTabTitlesResolved's timeout<=0 guard but expires during the CDP round-trip before the first predicate evaluation — failing "tabs still Unknown" on a healthy app. Floor to 1000ms to match waitForAppReady, and correct its stale "mirrors Math.max(1, ...)" comment.
2 participants
This change is
Summary by CodeRabbit