chore(client): migrate test setup to Vitest 4 - #550
Merged
Conversation
serendipty01
requested review from
cassidyjames,
dom-notion,
juanmrad,
julietshen and
vinaysrao1
as code owners
May 21, 2026 19:31
Contributor
📝 WalkthroughWalkthroughAdds a dedicated TypeScript test configuration wired into Vite's test typecheck and ESLint's parser project list, excludes test files from the main tsconfig, updates npm scripts and devDependencies, and migrates the ESLint custom rule test harness to an ESM flat-config Linter setup. ChangesTest TypeScript Config and ESLint Harness Update
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates client-side tests and tooling toward Vitest, updating configuration, scripts, and test implementations accordingly.
Changes:
- Add Vitest globals typing and update test runner scripts for CI/pre-push behavior.
- Migrate Jest-style mocks to Vitest mocks in a RuleFormUtils test.
- Update ESLint custom rule test harness to use an in-config plugin rule definition and TypeScript parser.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client/tsconfig.json | Adds Vitest global typings to TypeScript configuration. |
| client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts | Replaces Jest mocking with Vitest mocking utilities. |
| client/src/graphql/inputHelpers.test.ts | Updates inline snapshots formatting (likely aligning with Vitest output). |
| client/package.json | Updates pre-push test command and adds jsdom dependency. |
| client/eslint/tests/customRules.test.js | Refactors ESLint rule test harness to use flat-config style plugin definition and TS parser. |
Files not reviewed (1)
- client/package-lock.json: Language not supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts (1)
99-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest intent mismatch in non-boolean case.
Line 99 says this case “should show comparator/threshold”, but Line 111 asserts
false. Please align the test name or the expected value so the behavior is unambiguous.🤖 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 `@client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts` around lines 99 - 113, The test name and assertion disagree: the "Condition with input and selected signal with non-boolean output should show comparator/threshold" case currently asserts false; update either the test description or the assertion to match intended behavior. Locate the test in RuleFormUtils.test.ts that mocks getConditionInputScalarType and calls shouldConditionPromptForComparatorAndThreshold; if non-boolean output should prompt for comparator/threshold, change the expect(...).toEqual(false) to expect(...).toEqual(true); otherwise rename the it(...) description to reflect that the function should return false for this setup.
🤖 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.
Outside diff comments:
In `@client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts`:
- Around line 99-113: The test name and assertion disagree: the "Condition with
input and selected signal with non-boolean output should show
comparator/threshold" case currently asserts false; update either the test
description or the assertion to match intended behavior. Locate the test in
RuleFormUtils.test.ts that mocks getConditionInputScalarType and calls
shouldConditionPromptForComparatorAndThreshold; if non-boolean output should
prompt for comparator/threshold, change the expect(...).toEqual(false) to
expect(...).toEqual(true); otherwise rename the it(...) description to reflect
that the function should return false for this setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 605e56d2-0e64-4112-a420-d65a77bbf868
📒 Files selected for processing (4)
client/eslint/__tests__/customRules.test.jsclient/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.tsclient/tsconfig.test.jsonclient/vite.config.ts
✅ Files skipped from review due to trivial changes (1)
- client/tsconfig.test.json
cassidyjames
added
javascript
serendipty01
force-pushed
the
fix-client-vitest
branch
from
May 22, 2026 08:05
10ac746 to
d7a6c7f
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts (1)
99-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the non-boolean test intent/assertion mismatch.
Line 99 says this case should show comparator/threshold, but Lines 111-113 assert
false. This makes the test ambiguous and can mask regressions. Please align either the title or the expectation.Suggested fix (if the title is the intended behavior)
- expect(shouldConditionPromptForComparatorAndThreshold(condition)).toEqual( - false, - ); + expect(shouldConditionPromptForComparatorAndThreshold(condition)).toEqual( + true, + );🤖 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 `@client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts` around lines 99 - 113, The test title says the condition "should show comparator/threshold" but the assertion expects false; update the assertion in RuleFormUtils.test.ts so that shouldConditionPromptForComparatorAndThreshold(condition) returns true (or alternatively change the test title to reflect that it should not show comparator), referencing the test case that constructs a RuleFormLeafCondition with sampleSignal and the mocked getConditionInputScalarType returning GQLScalarType.Number; ensure the expectation matches the intended behavior of shouldConditionPromptForComparatorAndThreshold.
🤖 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.
Outside diff comments:
In `@client/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.ts`:
- Around line 99-113: The test title says the condition "should show
comparator/threshold" but the assertion expects false; update the assertion in
RuleFormUtils.test.ts so that
shouldConditionPromptForComparatorAndThreshold(condition) returns true (or
alternatively change the test title to reflect that it should not show
comparator), referencing the test case that constructs a RuleFormLeafCondition
with sampleSignal and the mocked getConditionInputScalarType returning
GQLScalarType.Number; ensure the expectation matches the intended behavior of
shouldConditionPromptForComparatorAndThreshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 177ba57e-fd21-4f76-a557-024809bb87f6
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
client/.eslintrc.cjsclient/eslint/__tests__/customRules.test.jsclient/package.jsonclient/src/graphql/inputHelpers.test.tsclient/src/webpages/dashboard/rules/rule_form/RuleFormUtils.test.tsclient/tsconfig.jsonclient/tsconfig.test.jsonclient/vite.config.ts
✅ Files skipped from review due to trivial changes (2)
- client/src/graphql/inputHelpers.test.ts
- client/tsconfig.test.json
serendipty01
force-pushed
the
fix-client-vitest
branch
2 times, most recently
from
May 22, 2026 08:39
fbadd52 to
2edd83f
Compare
- Replace --watchAll=false (Jest-only flag) with vitest run in test:prepush - Add jsdom to devDependencies (no longer bundled in Vitest 2+) - Add vitest/globals to tsconfig types for vi global type support - Rewrite eslint/customRules test to use ESLint 9 flat config Linter API with @typescript-eslint/parser (defineRule removed in ESLint 9) - Migrate RuleFormUtils.test.ts from jest.mock/jest.requireActual to vi.mock/vi.importActual (jest.mock not hoisted by Vite transform) - Update inputHelpers.test.ts inline snapshots to Vitest serializer format
resolve: fix-client-vitest .dockerignore conflict
serendipty01
force-pushed
the
fix-client-vitest
branch
from
July 7, 2026 13:18
2edd83f to
447adf2
Compare
serendipty01
changed the title
Contributor
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
julietshen
approved these changes
Jul 7, 2026
julietshen
left a comment
julietshen
left a comment
Member
There was a problem hiding this comment.
tested locally and everything works, thanks!
taobojlen
approved these changes
Jul 7, 2026
taobojlen
left a comment
taobojlen
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thank you for this!
i'll open a PR stacked on top of this that also fixes the failing tests and runs them in CI.
| "build": "vite build", | ||
| "test": "vitest --passWithNoTests", | ||
| "test:prepush": "vitest --watchAll=false --passWithNoTests", | ||
| "test:prepush": "vitest run --passWithNoTests", |
Contributor
There was a problem hiding this comment.
nit: we could run this with --typecheck instead of needing the separate typechecking step. i'm not sure which is preferable!
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.
Context & Requests for Reviewers
Follow-up to the client's Vitest 4 migration (
vitest ^4.1.0is already onmain). Three Jest-era leftovers remained, one of which breaks a pre-push gate:npm run test:prepushis broken onmain— it passes Jest's--watchAll=falseflag, which Vitest rejects (CACError: Unknown option --watchAll), socheck:prepushcan't run at all. Fixed tovitest run.eslint/__tests__/customRules.test.js(the test for our in-repo custom ESLint rule) was still CommonJS and used the legacyLinterconfig shape with no TypeScript parser — meaning the rule'sas-cast scenarios were never actually parsed as TS. Rewritten to ESM + ESLint 9 flat-config API (new Linter({ configType: 'flat' })) with@typescript-eslint/parser.vitest/globalstypes leaked into the app's tsconfig. Added a dedicatedtsconfig.test.json(extends the app config, addsvitest/globals+node); the apptsconfig.jsonnow excludes test files, so production code can no longer silently reference test globals.npm run lintgains atsc --noEmit -p tsconfig.test.jsongate, and the new config is wired intovite.config.ts,.eslintrc.cjs, and.dockerignore.Note for reviewers: the full client suite has 6 failures that also fail on pristine
main(Sidebar ×3, Calendar ×2, MergedReportsComponent ×1) — pre-existing and out of scope for this PR.Tests
For contrast: on
main,npm run test:prepushexits immediately withCACError: Unknown option --watchAll.🤖 AI Usage Disclosure
Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes