fix: remove unsupported --related flag from vitest invocation#37975
Conversation
…cted-js Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The only change is in Makefile (1 line: removing the unsupported --related flag from vitest invocation). Test Quality Sentinel skipped — no tests to analyze. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37975 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100). Neither Condition A nor Condition B is met. |
There was a problem hiding this comment.
Pull request overview
This PR updates the repo’s JavaScript “impacted tests” Make target to be compatible with Vitest 4.x by removing a CLI flag that Vitest no longer supports.
Changes:
- Remove the unsupported
--relatedflag from thetest-impacted-jsVitest invocation in theMakefile.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Drops --related from make test-impacted-js to avoid Vitest 4.x CLI errors. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| fi; \ | ||
| echo "Running impacted JavaScript unit tests for changed files: $$CHANGED_JS_FILES"; \ | ||
| cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism --related $(JS_IMPACTED_TEST_EXCLUDES) | ||
| cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism $(JS_IMPACTED_TEST_EXCLUDES) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose — approving with one observation on the semantic shift.
📋 Key Themes & Highlights
Summary
This is a minimal, correct fix for a real breakage. vitest 4.x removed --related, so the old target would fail immediately with CACError: Unknown option. Removing the flag restores the target to a working state.
One Thing to Verify
CHANGED_JS_FILES (line 237) collects all changed .ts/.js files — including non-test source files. The old --related would discover their companion test files; the new positional-arg approach passes them directly as test specs, which silently produces zero tests instead. If your project conventions ensure only *.test.ts files reach this command in practice, you are fine. If not, vitest related (the 4.x subcommand replacement) is worth a follow-up.
Positive Highlights
- Fix is the minimum necessary change — no scope creep
- PR description clearly explains both the cause and the rationale
- Root cause correctly identified: unsupported flag, not a logic error
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 152.7 AIC · ⌖ 13.2 AIC
| fi; \ | ||
| echo "Running impacted JavaScript unit tests for changed files: $$CHANGED_JS_FILES"; \ | ||
| cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism --related $(JS_IMPACTED_TEST_EXCLUDES) | ||
| cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism $(JS_IMPACTED_TEST_EXCLUDES) |
There was a problem hiding this comment.
[/diagnose] The removal of --related is correct for vitest 4.x compatibility, but worth noting the semantic shift: --related would discover test files that import the changed files (indirect coverage), while positional args now tell vitest to run those files directly as test specs.
For .test.ts files this is identical. For plain source files (e.g. utils.ts) that appear in CHANGED_JS_FILES, the old path found their companion test; the new path hands utils.ts to vitest as a test spec, which will produce zero test results rather than an error.
💡 Suggested Makefile comment + optional follow-up
Add a brief comment so future maintainers understand why --related is absent:
# Note: vitest 4.x removed --related; files are matched as test-spec patterns directly.
cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism $(JS_IMPACTED_TEST_EXCLUDES)If CHANGED_JS_FILES can include non-test source files, consider using vitest related subcommand (introduced in vitest 4.x as the CLI replacement for --related):
cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | xargs npm run test:js -- related --no-file-parallelism $(JS_IMPACTED_TEST_EXCLUDES)
There was a problem hiding this comment.
Incomplete fix — test-impacted-js silently skips tests for source-file changes.
Details
Removing the unsupported --related flag is correct, but the replacement mechanism is missing. With vitest run [source-files], vitest treats the file names as path-filter substrings against test-file paths. Because foo.cjs is not a substring of foo.test.cjs, zero test suites match — and vitest exits 0 silently. Any PR touching only source files passes CI with no JS tests run.
The correct replacement is the vitest related [files] subcommand (import-graph–aware). See inline comment for the two-line fix.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 12.9 AIC
| fi; \ | ||
| echo "Running impacted JavaScript unit tests for changed files: $$CHANGED_JS_FILES"; \ | ||
| cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism --related $(JS_IMPACTED_TEST_EXCLUDES) | ||
| cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" | sed 's|^actions/setup/js/||' | tr '\n' '\0' | xargs -0 -r npm run test:js -- --no-file-parallelism $(JS_IMPACTED_TEST_EXCLUDES) |
There was a problem hiding this comment.
Silent test skip for source-file changes: removing --related without a replacement means test-impacted-js no longer runs tests for changed source files — only changed test files get exercised, giving false CI confidence.
💡 Root cause and suggested fix
What broke
vitest run treats positional arguments as path-filter substrings matched against test-file paths. A changed source file foo.cjs is passed as the filter pattern — but foo.cjs is not a substring of the corresponding test file foo.test.cjs. Vitest finds zero matching test suites and exits 0 silently.
This means: any PR that touches only source files (e.g. add_comment.cjs) will run no JavaScript tests at all, yet make test-impacted-js reports success.
Why --related was wrong to begin with
--related is not a flag for vitest run; it is a subcommand (vitest related [files]). Passing it to vitest run was already invalid. The commit correctly removes the unsupported flag, but the fix is incomplete.
Correct fix
Switch from vitest run to vitest related. The simplest approach — add a dedicated npm script and call it here:
// package.json
"test:js-related": "vitest related"# Makefile (line 243)
cd actions/setup/js && printf '%s\n' "$$CHANGED_JS_FILES" \
| sed 's|^actions/setup/js/||' \
| tr '\n' '\0' \
| xargs -0 -r npm run test:js-related -- --no-file-parallelism $(JS_IMPACTED_TEST_EXCLUDES)vitest related [files] builds an import graph and finds all test files that (transitively) import the listed source files — the original intended semantics.
vitest 4.x dropped the
--relatedCLI flag, causingmake test-impacted-jsto fail withCACError: Unknown option '--related'.Changes
Makefile: Remove--relatedfrom thetest-impacted-jstarget. Changed files are already passed as positional arguments tovitest run, so vitest filters to those files directly without needing--related.