refactor(browser): centralize browser config validation in @rstest/browser#951
refactor(browser): centralize browser config validation in @rstest/browser#951
Conversation
There was a problem hiding this comment.
Pull request overview
This PR moves browser-specific config validation (provider + viewport) out of @rstest/core and into @rstest/browser, running validation after the browser package is dynamically loaded. This keeps core focused on normalization while delegating runtime validation to the browser runtime package.
Changes:
- Added
validateBrowserConfig()to@rstest/browserand invoked it from core before running/listing browser tests. - Moved viewport preset IDs + preset resolution into
@rstest/browser, and updated core types to keepdefineConfigautocomplete. - Updated core config tests to reflect “normalize only” behavior and added an e2e fixture/test for invalid provider early exit.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/tests/config.test.ts | Updates expectations: core no longer throws on invalid/missing browser fields during normalization. |
| packages/core/src/utils/constants.ts | Removes viewport preset IDs from core constants. |
| packages/core/src/types/config.ts | Converts DevicePreset typing to an explicit union and documents sync requirement with browser runtime presets. |
| packages/core/src/env.d.ts | Adds validateBrowserConfig to the ambient @rstest/browser module declaration. |
| packages/core/src/core/runTests.ts | Calls validateBrowserConfig() after loading @rstest/browser and before running browser tests. |
| packages/core/src/core/listTests.ts | Calls validateBrowserConfig() after loading @rstest/browser and before listing browser tests. |
| packages/core/src/core/browserLoader.ts | Extends BrowserModule type to include validateBrowserConfig. |
| packages/core/src/config.ts | Removes browser provider/viewport validation from withDefaultConfig() (normalization only). |
| packages/browser/src/viewportPresets.ts | Introduces runtime source-of-truth for viewport preset IDs and dimensions + resolver. |
| packages/browser/src/index.ts | Exports validateBrowserConfig and viewport preset utilities from the browser package entrypoint. |
| packages/browser/src/configValidation.ts | Implements provider + viewport validation used by core before starting browser mode. |
| e2e/browser-mode/fixtures/invalid-provider/tests/smoke.test.ts | Adds a minimal test file for the invalid-provider e2e fixture. |
| e2e/browser-mode/fixtures/invalid-provider/rstest.config.ts | Adds an e2e config fixture with an intentionally invalid browser provider. |
| e2e/browser-mode/config.test.ts | Adds an e2e test asserting invalid provider fails before browser mode starts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core/src/core/listTests.ts
Outdated
| validateBrowserConfig(context as any); | ||
| // Cast to any because listBrowserTests expects Rstest but we have RstestContext | ||
| // In practice, context is always a Rstest instance | ||
| return listBrowserTests(context as any, { shardedEntries }); |
There was a problem hiding this comment.
The as any casts aren’t needed here: BrowserModule.validateBrowserConfig/listBrowserTests are typed to accept unknown in core, so context can be passed directly. Keeping the casts (and the nearby comment about type mismatch) makes the code harder to reason about and can hide real typing issues.
| validateBrowserConfig(context as any); | |
| // Cast to any because listBrowserTests expects Rstest but we have RstestContext | |
| // In practice, context is always a Rstest instance | |
| return listBrowserTests(context as any, { shardedEntries }); | |
| validateBrowserConfig(context); | |
| return listBrowserTests(context, { shardedEntries }); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c5602a422
ℹ️ 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".
| const { validateBrowserConfig, runBrowserTests } = await loadBrowserModule({ | ||
| projectRoots, | ||
| }); | ||
| validateBrowserConfig(context); |
There was a problem hiding this comment.
Handle validation failures before parallel browser launch
runBrowserModeTests now calls validateBrowserConfig(context) and can reject immediately on invalid browser config, but in mixed node+browser runs runTests starts this promise and only awaits it much later after node work. In Node 22 this creates an unhandled rejection window (default --unhandled-rejections=throw), so an invalid browser provider/viewport can crash the process before reporters/cleanup run instead of producing a controlled test failure; this regression is introduced by the new synchronous throw point here.
Useful? React with 👍 / 👎.
2c5602a to
8b22d6b
Compare
Summary
Moves browser-specific config validation (provider, viewport) from
@rstest/coreto@rstest/browser, called after dynamic import. This keeps@rstest/corefocused on config normalization while@rstest/browserowns runtime validation logic. Type definitions remain in core to preservedefineConfigautocomplete.Behavior diff:
withDefaultConfig()in core@rstest/browser, slightly laterwithDefaultConfig()in corevalidateBrowserConfig()in browser packageChanges:
validateBrowserConfig()exported from@rstest/browserBROWSER_VIEWPORT_PRESET_IDSandresolveBrowserViewportPreset()to browser packageDevicePresetfrom derived const to explicit literal union in core typeswithDefaultConfig()in coreRelated Links
Checklist