Repository navigation
test: Add Playwright browser config to Vitest; Add GitHub test workflow. - #125
Merged
Merged
Conversation
WalkthroughThis pull request establishes a comprehensive Vitest-based testing infrastructure with Playwright browser testing support. Changes include updating test scripts and configurations, adding GitHub Actions CI/CD workflow, restructuring task runner setup, and introducing test initialization and setup files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Bill-hbrhbr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 20-21: The package.json scripts "test:init" and "test" use
POSIX-only inline env assignment (PLAYWRIGHT_BROWSERS_PATH=0 ...) which breaks
on Windows; update both scripts to use cross-env (e.g., prefix with cross-env
PLAYWRIGHT_BROWSERS_PATH=0) and add cross-env to devDependencies so the commands
(the "test:init" script that runs npm exec playwright -- install --with-deps and
the "test" script that runs vitest run) work cross-platform.
In `@taskfiles/test.yaml`:
- Around line 7-9: The init task currently always runs "npm run test:init" which
triggers "playwright install --with-deps" on every invocation; add a Taskfile
status guard to short-circuit the task when browsers are already installed by
setting a status check for the init task (e.g., a lightweight shell command that
verifies Playwright/browsers exist such as checking for the .playwright
directory or running a quick npx/playwright version check) so the cmd only runs
when the status check fails; update the task named "init" to include this status
key and keep the existing cmd "npm run test:init".
In `@vitest.config.ts`:
- Around line 9-34: The node and browser Vitest projects both use the same
include glob causing tests to run in all environments; update the project
configs for the "node" and "browser" objects in vitest.config.ts so each uses a
distinct include pattern (e.g., node tests match a *.node.test.ts suffix and
browser tests match a *.browser.test.ts suffix) by changing the test.include
values for the "node" and "browser" project entries accordingly and update any
test filenames to follow the chosen naming convention.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yaml:
- Around line 35-37: Update the pinned actions/checkout reference in the
workflow so it uses the latest v6.0.2 release instead of the old SHA for v6.0.1;
locate the uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8"
line in .github/workflows/tests.yaml (the actions/checkout usage) and replace
the pin with the v6.0.2 identifier (de0fac2) or the tag v6.0.2 to keep the
workflow on the current release.
- Line 50: The GitHub Actions step that runs the JS tests is missing a name,
causing it to show only the raw command in the UI; add a human-readable name
field (for example "Run JS tests" or "Run JavaScript tests") above the run:
"task tests:js" entry in the workflow so the step renders with a proper label in
the Actions UI while leaving the run command unchanged.
- Around line 39-50: The workflow is missing project dependency and Playwright
browser setup before running "task tests:js"; update the CI to install
node_modules (run "npm ci" or "npm install" after the "Log tool versions" step)
and then run the browser init (either "npm run test:init" or "npx playwright
install --with-deps") before invoking the existing "task tests:js" step
referenced in taskfiles/tests.yaml so that vitest and Playwright browsers are
available.
In `@taskfiles/tests.yaml`:
- Around line 7-13: Split the current heavy init into two tasks: create a
lightweight task (e.g., init-lite or init-light) that depends only on
":node-modules" and runs "npm run test:init", and keep a full init (e.g.,
init-full or init) that also depends on ":package" plus ":node-modules" for full
WASM builds; then change the js/tests:js task to depend on the lightweight task
(deps: ["init-lite"]) so running task tests:js will install node_modules and
Playwright browsers without triggering the full :package build. Ensure you
update the task names referenced in the cmd/dep blocks accordingly (init,
init-lite/init-full, js/tests:js).
In `@tests/globalSetup.ts`:
- Around line 1-3: The globalSetup function is unnecessarily declared async even
though it contains no await; change the declaration of globalSetup from an async
arrow to a synchronous one (i.e., remove the async keyword on the const
globalSetup = async () => { ... } so it becomes const globalSetup = () => { ...
}) to match Vitest's synchronous globalSetup signature and keep behavior
identical.
---
Duplicate comments:
In `@taskfiles/tests.yaml`:
- Around line 7-9: The tests:init task definition (the init entry with deps
[":package", ":node-modules"] and cmd "npm run test:init") currently triggers
Playwright install unconditionally; split the Playwright install out of the
tests:init flow by creating a dedicated task (e.g., playwright-install) that
runs the install and remove it from the init deps or make tests:init only run
the install conditionally; update the init task's deps/cmd so it no longer
always invokes Playwright installation and ensure callers can opt-in to run the
new playwright-install task when needed.
In `@vitest.config.ts`:
- Line 12: The shared include glob ("include" in vitest.config.ts) is too broad
and causes every test to run in all environments; narrow the test discovery by
making the include globs environment- or project-specific (e.g., per project/env
change the "include" value from "tests/**/*.test.ts" to a unique pattern like
"tests/node/**/*.test.ts" or "packages/<project>/tests/**/*.test.ts"), or move
include into each project config if using a multi-project setup; update the
"include" entry referenced in vitest.config.ts so each environment only matches
its intended test files.
Bill-hbrhbr
commented
Feb 22, 2026
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/tests.yaml:
- Around line 35-37: Update the pinned actions/checkout commit and its inline
comment to the latest patch release: locate the uses:
"actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8" entry
(actions/checkout) and replace the SHA with the current release commit SHA for
the latest v6.x patch, and update the trailing comment from "v6.0.1" to the
actual latest tag (e.g., v6.0.2 or newer) so the workflow points to the
up-to-date release; verify the latest actions/checkout release and commit SHA on
GitHub before making the change.
In `@taskfiles/tests.yaml`:
- Around line 11-13: The js task currently depends on init which triggers the
heavy :package WASM build on every test run; split init into two tasks (e.g.,
init and init:light or node-modules/setup-browsers) and update the js task to
depend only on the lightweight variant so it installs deps and prepares browsers
without invoking :package; modify the task definitions referenced (js, init,
:package, :node-modules, npm run test:init) so the full WASM build stays out of
the test hot-path and only runs when explicitly requested.
junhaoliao
reviewed
Feb 23, 2026
junhaoliao
left a comment
junhaoliao
left a comment
Member
There was a problem hiding this comment.
thanks for getting this started
this PR renames those to tests/ and rewrites vitest.config.ts — it might be worth keeping consistency with what was just established unless there's a strong reason for the rename. The test/ (singular) name follows the convention used by Vitest itself, as well as Mocha and Node.js - it's the standard test directory name in the JS/TS ecosystem
junhaoliao
approved these changes
Feb 23, 2026
junhaoliao
left a comment
junhaoliao
left a comment
Member
There was a problem hiding this comment.
all including the pr title lgtm except the test/ comment
junhaoliao
requested changes
Feb 23, 2026
junhaoliao
left a comment
junhaoliao
left a comment
Member
There was a problem hiding this comment.
waiting for the test/ rename before a final approval
Bill-hbrhbr
commented
Feb 23, 2026
junhaoliao
reviewed
Feb 23, 2026
Co-authored-by: Junhao Liao <junhao@junhao.ca>
junhaoliao
self-requested a review
February 23, 2026 09:56
junhaoliao
approved these changes
Feb 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/test.yaml (1)
35-37:actions/checkoutversion already discussed in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yaml around lines 35 - 37, This is a duplicate review note about the actions/checkout pin; remove the redundant comment and ensure the workflow uses a single, intentional checkout reference (update the actions/checkout invocation in the workflow where it appears — the line containing uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8") and, if you intend to change the version, replace that hash with the chosen canonical tag (for example a maintained semver tag like actions/checkout@v3) in one place only so there are no repeated comments or conflicting pins.taskfiles/test.yaml (1)
7-13: The dependency on:package(full WASM build) in thejstask path was already raised in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/test.yaml` around lines 7 - 13, The js task currently (or previously) includes a direct dependency on :package which duplicates the heavy full WASM build already pulled in by the init task; remove the direct ':package' entry from the js task's deps so js only depends on "init" (leave init's deps as deps: [":package", ":node-modules"]) and rely on init to provide the package artifact—update the js task's deps to ["init"] and ensure there are no remaining direct references to :package in the js task.vitest.config.ts (1)
9-34: Identicalincludeglob for both projects — already discussed in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 9 - 34, Both project entries use the same include glob, causing duplicate test runs; update the include patterns for the test projects so they target distinct files (e.g., separate directories or filename suffixes) to prevent overlap. Locate the two project objects with test.name "node" and test.name "browser" in the vitest config and change their include arrays to non-overlapping globs (for example, "test/node/**/*.test.ts" vs "test/browser/**/*.test.ts" or use different filename suffixes like "*.node.test.ts" and "*.browser.test.ts"), and update any test files or runners accordingly so each project only picks up its intended tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 32-51: Add a pinned Node.js setup step to the tests job using
actions/setup-node before running tools or tests: insert a step that uses
"actions/setup-node@v4" (or latest stable) with the node-version input set to
the desired semver (e.g., "18.x" or a specific patch) so the environment is
deterministic; place this new step before the "Log tool versions" and "Run JS
tests" steps to ensure node/npm are the expected versions when task test:js and
`@go-task/cli` run.
---
Duplicate comments:
In @.github/workflows/test.yaml:
- Around line 35-37: This is a duplicate review note about the actions/checkout
pin; remove the redundant comment and ensure the workflow uses a single,
intentional checkout reference (update the actions/checkout invocation in the
workflow where it appears — the line containing uses:
"actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8") and, if you intend
to change the version, replace that hash with the chosen canonical tag (for
example a maintained semver tag like actions/checkout@v3) in one place only so
there are no repeated comments or conflicting pins.
In `@taskfiles/test.yaml`:
- Around line 7-13: The js task currently (or previously) includes a direct
dependency on :package which duplicates the heavy full WASM build already pulled
in by the init task; remove the direct ':package' entry from the js task's deps
so js only depends on "init" (leave init's deps as deps: [":package",
":node-modules"]) and rely on init to provide the package artifact—update the js
task's deps to ["init"] and ensure there are no remaining direct references to
:package in the js task.
In `@vitest.config.ts`:
- Around line 9-34: Both project entries use the same include glob, causing
duplicate test runs; update the include patterns for the test projects so they
target distinct files (e.g., separate directories or filename suffixes) to
prevent overlap. Locate the two project objects with test.name "node" and
test.name "browser" in the vitest config and change their include arrays to
non-overlapping globs (for example, "test/node/**/*.test.ts" vs
"test/browser/**/*.test.ts" or use different filename suffixes like
"*.node.test.ts" and "*.browser.test.ts"), and update any test files or runners
accordingly so each project only picks up its intended tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/test.yaml.gitignoretaskfiles/test.yamltest/ClpStreamReader.test.tstest/globalSetup.tsvitest.config.ts
Comment on lines
+32
to
+51
| tests: | ||
| runs-on: "ubuntu-24.04" | ||
| steps: | ||
| - uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8" # v6.0.1 | ||
| with: | ||
| submodules: "recursive" | ||
|
|
||
| - name: "Install task" | ||
| run: "npm install -g @go-task/cli@3.48.0" | ||
|
|
||
| - name: "Log tool versions" | ||
| run: |- | ||
| cmake --version | ||
| md5sum --version | ||
| node --version | ||
| npm --version | ||
| task --version | ||
|
|
||
| - name: "Run JS tests" | ||
| run: "task test:js" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin the Node.js version with actions/setup-node.
The workflow does not pin Node.js — it relies on whatever version is pre-installed on ubuntu-24.04. Runner image updates can silently change the Node.js version, making failures hard to diagnose. Adding actions/setup-node makes the environment fully reproducible and consistent with how other tools (@go-task/cli) are explicitly versioned.
♻️ Proposed fix
- uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8" # v6.0.1
with:
submodules: "recursive"
+ - uses: "actions/setup-node@<pinned-sha>" # vX.Y.Z
+ with:
+ node-version: "<version-used-in-project>"
- name: "Install task"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yaml around lines 32 - 51, Add a pinned Node.js setup
step to the tests job using actions/setup-node before running tools or tests:
insert a step that uses "actions/setup-node@v4" (or latest stable) with the
node-version input set to the desired semver (e.g., "18.x" or a specific patch)
so the environment is deterministic; place this new step before the "Log tool
versions" and "Run JS tests" steps to ensure node/npm are the expected versions
when task test:js and `@go-task/cli` run.
5 of 7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces the initial testing boilerplate for the repository by adding a Vitest global setup entrypoint with a minimal passing test. The test config is also expanded to include playwright browser tests.
The goal is to confirm test setup and execution are wired correctly, both locally and in GitHub CI.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes
Tests
Chores