test: remove jsdom by running all DOM tests in a real browser - #1002
Conversation
WalkthroughThe repository now uses explicit Vitest projects. Core tests run in separate Playwright browser and Node projects. Testing documentation and ChangesVitest testing setup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ocavue
changed the title
jsdom by running meta-scrape tests in a real browserjsdom by running all DOM tests in a real browser
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/package.json`:
- Line 13: Ensure the targeted test command defined by the package.json test
script passes with a zero exit status by installing the missing Playwright
Chromium executable via pnpm exec playwright install, then rerunning the core
Vitest command. Do not treat the existing pnpm check warnings as failures.
In `@packages/core/vitest.browser.config.ts`:
- Around line 10-17: Add fileParallelism: false to the top-level Vitest
configuration in both the browser and node core project configs, alongside the
existing sequence/retry settings, so core test files continue executing
serially. Do not alter the existing browser or test execution settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd3ff844-25b2-4304-8a43-463a87eaecd8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
docs/contributing/testing.mdpackages/core/package.jsonpackages/core/src/actions/meta-scrape.test.tsxpackages/core/src/actions/meta-scrape.tspackages/core/vitest.browser.config.tspackages/core/vitest.node.config.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- packages/core/src/actions/meta-scrape.test.tsx
| "scripts": { | ||
| "typecheck": "tsc --noEmit", | ||
| "test": "vitest run" | ||
| "test": "vitest run --config ../../vitest.config.ts --project core-browser --project core-node" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
pnpm check
(
cd packages/core
pnpm test
)Repository: team-reflect/reflect-open
Length of output: 34236
Make the targeted core test command pass before merge.
pnpm check completes with warnings, but cd packages/core && pnpm test fails because the Playwright Chromium executable is missing. Run pnpm exec playwright install, then rerun the test command and require a zero exit status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/package.json` at line 13, Ensure the targeted test command
defined by the package.json test script passes with a zero exit status by
installing the missing Playwright Chromium executable via pnpm exec playwright
install, then rerunning the core Vitest command. Do not treat the existing pnpm
check warnings as failures.
Source: Coding guidelines
| sequence: { groupOrder: 100 }, | ||
| retry: process.env.CI ? 3 : 0, | ||
| slowTestThreshold: 10_000, | ||
| browser: { | ||
| enabled: true, | ||
| provider: playwright(), | ||
| headless: !process.env.DEBUG, | ||
| instances: [{ browser: browserName }], |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -t f 'vitest.*\.config\.(ts|js|mjs|cjs)$' . || true
printf '%s\n' '--- core config files ---'
for file in packages/core/vitest.browser.config.ts packages/core/vitest.node.config.ts vitest.config.ts; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
cat -n "$file"
fi
done
printf '%s\n' '--- Vitest configuration references ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' \
'fileParallelism|sequence\s*:|vitest\.browser|vitest\.node|defineConfig|projects' \
packages/core vitest.config.ts package.json pnpm-workspace.yaml 2>/dev/null || trueRepository: team-reflect/reflect-open
Length of output: 5233
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Vitest versions and test scripts ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' \
'"vitest"|"test"|"check"' \
package.json pnpm-lock.yaml packages/core/package.json apps/desktop/package.json 2>/dev/null | head -160
printf '%s\n' '--- core test files and likely shared-state APIs ---'
files=$(fd -t f -e ts -e tsx '.*\.test\.(ts|tsx)$' packages/core/src | sort)
printf '%s\n' "$files" | wc -l
rg -n --hidden \
'describe\.parallel|test\.parallel|it\.parallel|beforeAll|afterAll|beforeEach|afterEach|open\(|Database|sqlite|fs\.|writeFile|mkdir|rm\(|process\.env|globalThis|shared|singleton' \
packages/core/src --glob '*.test.ts' --glob '*.test.tsx' 2>/dev/null | head -240
printf '%s\n' '--- lockfile Vitest package entries ---'
rg -n -A4 -B2 \
'(^|/)vitest@|vitest:' \
pnpm-lock.yaml 2>/dev/null | head -120 || trueRepository: team-reflect/reflect-open
Length of output: 17965
🌐 Web query:
Vitest official documentation projects defineProject root config fileParallelism project config inheritance
💡 Result:
Vitest projects (formerly known as workspaces) allow you to run multiple configurations within a single Vitest process [1]. DefineProject The defineProject method is recommended for use within project configuration files (e.g., vitest.config.ts for a specific sub-project) to provide better type safety compared to defineConfig [2][1]. Root Config File Projects can be defined directly in your root configuration file using the test.projects array [2][1]. This array can contain objects (inline configuration), strings (file paths or glob patterns to other config files), or directories [2][3]. The root configuration file itself does not act as a project unless explicitly included in the projects array; it serves primarily to manage global options like reporters and coverage [1][4]. Parallelism Vitest supports file-level parallelism by default, running test files across multiple workers [5]. You can control this behavior per project by setting the fileParallelism property within a project's configuration [6]. For example, you can define a project with fileParallelism: false to force sequential execution for specific test suites, while other projects run in parallel [6]. Project Config Inheritance Inheritance behavior depends on how the project is defined: 1. Inline Projects: As of recent versions (starting with Vitest 5.0), inline projects inherit all options from the root-level configuration by default via the extends: true option [1][7]. You can disable this inheritance by setting extends: false [1][7]. 2. Config Files or Directories: Projects referenced as external config files or directories do not inherit options from the root configuration [1]. To share configuration, you can create a common shared config file and merge it explicitly using the mergeConfig utility [2][1]. 3. Special Cases: Certain options are never inherited (e.g., name and projects) [1][7]. globalSetup is not inherited from the root config because it is intended to run once per test process, but it is inherited when extending a non-root config file [1][7]. Tags replace the inherited array rather than merging with it [1]. [2][1][7][5][6]
Citations:
- 1: https://main.vitest.dev/guide/projects
- 2: https://vitest.dev/guide/projects
- 3: https://github.com/vitest-dev/vitest/blob/v4.1.10/docs/api/advanced/test-project.md
- 4: https://v2.vitest.dev/guide/workspace
- 5: https://vitest.dev/guide/parallelism
- 6: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/recipes.md
- 7: feat!: inline projects extend the root config by default vitest-dev/vitest#10750
Preserve serial file execution in both core projects.
The root fileParallelism: false does not apply to the external core project configs. Add it to packages/core/vitest.browser.config.ts and packages/core/vitest.node.config.ts, or prove that all core test files are safe to run in parallel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/vitest.browser.config.ts` around lines 10 - 17, Add
fileParallelism: false to the top-level Vitest configuration in both the browser
and node core project configs, alongside the existing sequence/retry settings,
so core test files continue executing serially. Do not alter the existing
browser or test execution settings.
1 participant
The only jsdom test was
parsePageMeta, whoseDOMParseralways runs in a browser in production, sopackages/coregets a browser project likeapps/desktopand jsdom leaves the dependency tree entirely.Summary by CodeRabbit
Testing
Documentation