feat: unify browser mode and node mode summary report#883
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f30a640758
ℹ️ 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".
| if (hasBrowserTests) { | ||
| // Pass project roots to resolve @rstest/browser from project-specific node_modules | ||
| const projectRoots = browserProjects.map((p) => p.rootPath); | ||
| const { runBrowserTests } = await loadBrowserModule({ projectRoots }); | ||
| await runBrowserTests(context); | ||
| // Start browser tests in parallel (don't await yet) | ||
| browserResultPromise = runBrowserModeTests(context, browserProjects, { | ||
| skipOnTestRunEnd: shouldUnifyReporter, | ||
| }); |
There was a problem hiding this comment.
Attach handler for parallel browser run promise
Starting the browser run without an attached rejection handler means any early failure (e.g., failing to resolve @rstest/browser, Playwright launch errors, or config import errors) can reject before the later await in run() attaches, which triggers an unhandledRejection event; in Node with --unhandled-rejections=strict or stricter test runners this can terminate the process and skip node-mode cleanup/reporting. Consider attaching .catch immediately or awaiting both via Promise.all so rejections are handled as part of the main control flow.
Useful? React with 👍 / 👎.
Deploying rstest with
|
| Latest commit: |
5dd323e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7e0c9a31.rstest.pages.dev |
| Branch Preview URL: | https://browser-report.rstest.pages.dev |
There was a problem hiding this comment.
Pull request overview
Unifies the summary report output between browser-mode and node-mode test runs, and enables running browser and node tests in parallel.
Changes:
- Runs browser tests and node tests concurrently and (in non-watch mode) emits a single unified reporter summary.
- Adds a structured return value from
@rstest/browsertest runs to support unified duration/failure reporting. - Centralizes Rsbuild logger level configuration based on debug mode.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/core/runTests.ts | Runs browser + node tests in parallel and unifies reporter output/duration/failure handling. |
| packages/core/src/core/rsbuild.ts | Moves debug logger configuration to module scope for Rsbuild. |
| packages/core/src/core/browserLoader.ts | Extends browser module typing to support options and returning a run result. |
| packages/browser/src/index.ts | Exposes new browser run options/result types and forwards options to controller. |
| packages/browser/src/hostController.ts | Implements skipOnTestRunEnd and returns structured run results for unified reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface BrowserModule { | ||
| runBrowserTests: (context: unknown) => Promise<void>; | ||
| runBrowserTests: ( | ||
| context: unknown, | ||
| options?: BrowserTestRunOptions, | ||
| ) => Promise<BrowserTestRunResult | void>; |
There was a problem hiding this comment.
This change makes runBrowserTests accept an options parameter and (optionally) return a structured result. The ambient module declaration in packages/core/src/env.d.ts still declares runBrowserTests(context): Promise<void> without options/return value; it should be updated to match the new signature so TypeScript consumers don't get incorrect types.
Summary
before:
after:
Related Links
Checklist