fix(browser): unify error reporting and avoid duplicate error messages#950
fix(browser): unify error reporting and avoid duplicate error messages#950
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dec2f12b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR aims to make browser-mode failures (especially early startup/runtime failures) flow through the same unified reporting path as normal test failures, so errors are neither lost nor printed twice—particularly in mixed node+browser runs.
Changes:
- Extend
BrowserTestRunResultto includeunhandledErrorsfor failures occurring outside test execution. - Merge browser
unhandledErrorsinto the unified reporter flow inrunTests. - Refactor browser controller early-exit error paths to return an error result and optionally call
onTestRunEnd.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/core/src/types/browser.ts | Adds unhandledErrors to represent browser startup/runtime errors outside test execution. |
| packages/core/src/reporter/index.ts | Changes DefaultReporter to eagerly create StatusRenderer in the constructor. |
| packages/core/src/core/runTests.ts | Merges browser unhandledErrors into the unified error list for summary reporting. |
| packages/browser/src/hostController.ts | Introduces buildErrorResult/failWithError to route early browser failures into the reporter summary path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isTTY() || options.logger) { | ||
| this.statusRenderer = new StatusRenderer( | ||
| this.rootPath, | ||
| this.testState, | ||
| this.options.logger, | ||
| rootPath, | ||
| testState, | ||
| options.logger, | ||
| ); | ||
| } |
There was a problem hiding this comment.
DefaultReporter now instantiates StatusRenderer in the constructor, which immediately starts WindowRenderer and intercepts process.stdout/stderr. Because WindowRenderer periodically clears/re-renders the terminal window, this can again buffer or wipe startup-time error output that occurs before the first test file starts (the exact regression the previous lazy-init avoided). Consider restoring lazy initialization (create StatusRenderer on first onTestFileStart) or delaying WindowRenderer.start()/stream interception until tests actually begin.
| const errorResult: BrowserTestRunResult = { | ||
| results: [], | ||
| testResults: [], | ||
| duration: { totalTime: 0, buildTime: 0, testTime: 0 }, |
There was a problem hiding this comment.
buildErrorResult hard-codes duration to 0 for early failures. For errors that happen after spending real time (e.g. runtime creation, temp dir setup), this under-reports time in the summary and also skews unified node+browser duration calculations. Consider populating duration from the elapsed time so far (at least buildTime/totalTime) when building the error result.
| const errorResult: BrowserTestRunResult = { | |
| results: [], | |
| testResults: [], | |
| duration: { totalTime: 0, buildTime: 0, testTime: 0 }, | |
| const elapsed = Math.max(0, Date.now() - buildStart); | |
| const errorResult: BrowserTestRunResult = { | |
| results: [], | |
| testResults: [], | |
| duration: { totalTime: elapsed, buildTime: elapsed, testTime: 0 }, |
| ensureProcessExitCode(1); | ||
| return; | ||
| const message = `Invalid RSTEST_CONTAINER_DEV_SERVER value: ${toError(error).message}`; | ||
| return failWithError(new Error(message)); |
There was a problem hiding this comment.
The invalid RSTEST_CONTAINER_DEV_SERVER path wraps the caught error into a new Error(message), which drops the original stack/context. Prefer preserving the original error via cause (or by augmenting the existing Error’s message) so the summary can still show where the URL parsing failed.
| return failWithError(new Error(message)); | |
| return failWithError(new Error(message, { cause: toError(error) })); |
3dec2f1 to
4c14011
Compare
Summary
credits to #891 (comment). @claneo
This change fixes browser-mode error reporting so early failures are no longer lost or printed twice.
We now route startup/runtime failures through a single
unhandledErrors -> onTestRunEndpath, which keeps reporting consistent.It also merges browser unhandled errors into the unified reporter flow in mixed node+browser runs.
As a result, users get one clear summary error output with correct failure exit behavior.
Related Links
Checklist