tests: shared browser-regression harness — six drifting copies become one (#515) - #518
Conversation
… one (#515) Every browser suite carried its own ~90-line copy of the same plumbing (startServer, browser lifecycle, browserTest wrapper, gotoDashboard), and the copies had already drifted: the spawn 'error' handler existed only in the newest file (Qodo, PR #514), named timeout constants in three of six, page-error assertions in four of six. tests/browser/helpers/harness.js now owns all of it — spawn error handler and named constants everywhere, uniform page-error gating (dashboard-96's journeys previously lacked the wrapper-level assert; the shared wrapper applies the contract uniformly), consistent teardown. Each suite keeps only its fixtures, selectors, and assertions. Behavior-preserving: the 14-journey suite is the parity net, same count, all green. Closes #515. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
PR Summary by Qodotests: consolidate shared browser regression harness
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
|
Warning Review limit reached
Next review available in: 11 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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 |
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // A spawn failure must reject the promise, never raise an unhandled | ||
| // 'error' event (Qodo, PR #514). | ||
| child.on('error', (err) => { if (!settled) { settled = true; clearTimeout(timer); rejectPromise(err); } }); | ||
| child.on('exit', () => { if (!settled) { settled = true; clearTimeout(timer); rejectPromise(new Error(`server exited\n${out}`)); } }); |
There was a problem hiding this comment.
1. startserver spawn error untested 📜 Skill insight ▣ Testability
The new child.on('error', ...) rejection path in startServer() has no corresponding test that
triggers a spawn failure and asserts the rejection behavior. This violates the requirement that
newly added error-handling branches must be covered by tests.
Agent Prompt
## Issue description
`tests/browser/helpers/harness.js` adds a new error-handling branch for spawn failures (`child.on('error', ...)`), but there is no test that forces this branch and asserts that `startServer()` rejects as intended.
## Issue Context
PR Compliance ID 1400010 requires that when error-handling code is added, a test must trigger the error path and assert the behavior.
## Fix Focus Areas
- tests/browser/helpers/harness.js[52-88]
- tests/browser/helpers/harness.test.js[1-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo (testability, compliance 1400010): the harness's child 'error' handler had no test triggering a real spawn failure. startServer gains an injectable execPath seam; a browser-free core-suite test spawns a nonexistent binary and asserts the clean ENOENT rejection. Mutation- checked: with the handler removed, the test fails on the unhandled event. Refs #515 (PR #518 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Qodo's testability finding addressed: |
2 participants
Closes #515 · #502 hygiene lane · follow-up from PR #514's Qodo review.
What
Every browser suite carried its own ~90-line copy of the same plumbing — and the copies had already drifted: the spawn
errorhandler existed only in the newest file (Qodo's #514 finding), named timeout constants in three of six, wrapper-level page-error assertions in four of six.tests/browser/helpers/harness.jsnow owns it all:startServer(with the error handler + named constants), the one-browser-per-file lifecycle, thebrowserTestwrapper (skip-without-Chromium, temp fixture root, page-error collection + uniform assertion, teardown),gotoDashboard(optional STATE wait) andnavigate(visible-nav-button click with strict/lenient visibility modes). Each suite keeps only its fixtures, selectors, and assertions — net −346 lines.One deliberate tightening: dashboard-96's journeys previously lacked the wrapper-level no-page-errors gate (only journey 1 asserted explicitly); the shared wrapper applies the contract uniformly — all journeys pass under it.
Verification
Behavior-preserving refactor: browser suite 14/14 (same journey count), core 1817/1818 with the single failure being the known temp-dir teardown flake (
dashboard-env-write, ENOTEMPTY, 8/8 in isolation — second sighting of the class, now filed as #517), lint 0 errors, typecheck 0, whitespace clean.Merging after green checks + reviewer bodies read, per the current working protocol.
🤖 Generated with Claude Code