test: clean web runner warnings - #49
Conversation
📝 WalkthroughWalkthroughThe web package updates how Vitest and Vite are launched, adds a Vitest no-warnings setting, and strengthens two store failure tests by asserting their logged console output. ChangesTest tooling and console assertions
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/stores/__tests__/authStore.test.ts (1)
58-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSpy restore isn't exception-safe.
warnSpy.mockRestore()at Line 74 only runs if all precedingexpect()calls pass. If any assertion between Lines 65-72 throws, the spy leaks into later tests, potentially masking or altering subsequent console-based assertions. Wrapping intry/finally, or restoring in anafterEach, would make cleanup reliable regardless of assertion outcome.Suggested fix
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) cookiesMock.get.mockReturnValueOnce('bad-token') fetchMock.get.mockRejectedValueOnce(new Error('unauthorized')) - await expect(store.verifyToken()).resolves.toBe(false) - - expect(warnSpy).toHaveBeenCalledWith( - 'Token verification failed on server, clearing session:', - expect.any(Error), - ) - expect(cookiesMock.remove).toHaveBeenCalledWith('accessToken') - expect(cookiesMock.remove).toHaveBeenCalledWith('refreshToken') - expect(store.currentUser).toBeNull() - expect(store.isAuthenticated).toBe(false) - - warnSpy.mockRestore() + try { + await expect(store.verifyToken()).resolves.toBe(false) + + expect(warnSpy).toHaveBeenCalledWith( + 'Token verification failed on server, clearing session:', + expect.any(Error), + ) + expect(cookiesMock.remove).toHaveBeenCalledWith('accessToken') + expect(cookiesMock.remove).toHaveBeenCalledWith('refreshToken') + expect(store.currentUser).toBeNull() + expect(store.isAuthenticated).toBe(false) + } finally { + warnSpy.mockRestore() + }🤖 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 `@web/src/stores/__tests__/authStore.test.ts` around lines 58 - 74, The console.warn spy cleanup in the authStore verifyToken test is not exception-safe, so it may leak into later tests if an assertion fails. Move the warnSpy.mockRestore() cleanup in this test to a try/finally block, or restore the spy in an afterEach hook, so the spy is always reset regardless of failures. Keep the fix localized around the verifyToken test and the warnSpy setup.web/package.json (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mkdir -pmay not be portable on native Windows shells.If any contributor or CI runner executes
npm run test:unitunder a plaincmd.exe(no bash/WSL),mkdir -pisn't guaranteed to behave the same, and the whole compound&&command could break. Consider usingmkdirp/shx mkdir -p(or a Node one-liner) for cross-platform safety, or confirm all execution environments provide a POSIX-compatible shell.🤖 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 `@web/package.json` at line 10, The test:unit script uses a shell-specific mkdir -p command that may fail on native Windows cmd.exe. Update the script in package.json to use a cross-platform directory creation approach, such as shx mkdir -p, mkdirp, or a small Node-based command, while keeping the rest of the vitest invocation unchanged. Use the test:unit script entry as the place to fix it so the command works reliably across all supported environments.
🤖 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 `@web/package.json`:
- Line 10: The test:unit script in package.json uses --localstorage-file, which
is not supported by the currently declared Node engine range. Update the engines
field to require a Node version that supports this flag (for example, >=22.4.0),
or change the test:unit command to conditionally use the flag based on the Node
version; keep the fix localized to package.json and the test:unit entry.
---
Nitpick comments:
In `@web/package.json`:
- Line 10: The test:unit script uses a shell-specific mkdir -p command that may
fail on native Windows cmd.exe. Update the script in package.json to use a
cross-platform directory creation approach, such as shx mkdir -p, mkdirp, or a
small Node-based command, while keeping the rest of the vitest invocation
unchanged. Use the test:unit script entry as the place to fix it so the command
works reliably across all supported environments.
In `@web/src/stores/__tests__/authStore.test.ts`:
- Around line 58-74: The console.warn spy cleanup in the authStore verifyToken
test is not exception-safe, so it may leak into later tests if an assertion
fails. Move the warnSpy.mockRestore() cleanup in this test to a try/finally
block, or restore the spy in an afterEach hook, so the spy is always reset
regardless of failures. Keep the fix localized around the verifyToken test and
the warnSpy setup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce32812e-72b9-4cfd-ab35-3f5bdfd3e0c3
📒 Files selected for processing (3)
web/package.jsonweb/src/stores/__tests__/authStore.test.tsweb/src/stores/__tests__/dataStores.test.ts
|
@coderabbitai that's save? use mkdir on package json? why not vitest? give me the reason. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat -n web/package.json | sed -n '1,40p'Length of output: 1669 🏁 Script executed: # Check whether `cross-env` is already a devDependency, and whether a vitest setup file / vitest.config already exists
rg -n '"cross-env"' web/package.json
fd -HI 'vitest.setup|vitest.config' webLength of output: 355 🏁 Script executed: #!/bin/bash
cat -n web/vitest.config.ts
echo "---"
fd -HI 'setup' web/src -e tsLength of output: 702
This reinforces the earlier point: the warning is a side effect of Node's process-level global, not something your test code controls — so suppressing it via Let me know if you'd like me to go ahead with that plan (swap the script for |
|
Addressed the review in the latest commit. Reasoning: direct |
DEV v4.1.8 /home/rafii/Documents/ping-uptime/web
(node:95580) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
(node:95584) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/composables/__tests__/useAppTitle.test.ts (3 tests) 101ms
✓ src/composables/__tests__/useUpdateChecker.test.ts (2 tests) 120ms
✓ src/stores/__tests__/dataStores.test.ts (15 tests) 65ms
✓ src/composables/__tests__/storeBindings.test.ts (18 tests) 44ms
(node:95611) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/stores/__tests__/realtimeStores.test.ts (4 tests) 25ms
(node:95617) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/stores/__tests__/authStore.test.ts (4 tests) 27ms
✓ src/validations/__tests__/schemas.test.ts (7 tests) 26ms
(node:95635) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/stores/__tests__/backupStore.test.ts (3 tests) 41ms
(node:95656) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
(node:95650) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/composables/__tests__/useNotificationLogs.test.ts (3 tests) 15ms
(node:95665) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/composables/__tests__/useIncidents.test.ts (3 tests) 24ms
✓ src/composables/__tests__/useSslMonitors.test.ts (3 tests) 16ms
✓ src/lib/__tests__/utils.test.ts (2 tests) 17ms
(node:95689) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
(Use `node --trace-warnings ...` to show where the warning was created)
✓ src/composables/__tests__/useApiTokens.test.ts (1 test) 9ms
Test Files 13 passed (13)
Tests 68 passed (68)
Start at 10:37:36
Duration 7.14s (transform 1.24s, setup 0ms, import 3.48s, tests 531ms, environment 17.40s)after try checkout |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat -n web/vitest.config.tsLength of output: 658
Root cause: The current script: "test:unit": "mkdir -p node_modules/.tmp && node --localstorage-file=node_modules/.tmp/vitest-localstorage ./node_modules/vitest/vitest.mjs"only passes Two proper fixes, either works:
"test:unit": "cross-env NODE_OPTIONS=--no-warnings vitest"or if you specifically want the localStorage file behavior everywhere: "test:unit": "cross-env NODE_OPTIONS=--localstorage-file=node_modules/.tmp/vitest-localstorage vitest"(needs
// vitest.config.ts
test: {
environment: 'jsdom',
exclude: [...configDefaults.exclude, 'e2e/**'],
root: fileURLToPath(new URL('./', import.meta.url)),
poolOptions: {
forks: {
execArgv: ['--no-warnings'], // or ['--localstorage-file=node_modules/.tmp/vitest-localstorage']
},
},
},This explicitly forwards the flag to every forked worker process, which is the officially supported way to pass Node args through Vitest's pool, and keeps I'd lean towards option 2 since it keeps the fix scoped to test configuration rather than the npm script, and doesn't require a new dependency. Want me to draft this change?
✏️ Learnings added
The requested changes have been implemented and a pull request has been created: View PR |
|
Follow-up pushed in Root cause: the previous wrapper passed Node flags only to the parent Vitest process, while Vitest 4 runs tests in forked workers. The warning came from those worker processes, not from missing app auth tokens. The tests use Changed the PR to use Vitest 4's top-level Verified locally:
|
coderabbitai
Bot
changed the title
|
Follow-up pushed in This warning is upstream runtime noise from the Node/Vite/Vitest bootstrap path, not from the app code. I couldn't reproduce a concrete in-repo What changed:
Verified locally:
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/package.json (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoding internal
node_modulesentrypoints instead of thevite/vitestbinaries.Directly invoking
./node_modules/vite/bin/vite.jsand./node_modules/vitest/vitest.mjsbypasses npm's bin resolution, is fragile if these packages change their entrypoint paths in a future release, and is not portable across package managers (e.g. pnpm's nested/symlinked node_modules layout) or Windows path handling.♻️ Prefer the standard binaries with NODE_OPTIONS
- "dev": "node --disable-warning=DEP0205 ./node_modules/vite/bin/vite.js", + "dev": "cross-env NODE_OPTIONS=--disable-warning=DEP0205 vite", "build": "run-p type-check \"build-only {@}\" --", - "preview": "node --disable-warning=DEP0205 ./node_modules/vite/bin/vite.js preview", - "test:unit": "node --disable-warning=DEP0205 ./node_modules/vitest/vitest.mjs", - "build-only": "node --disable-warning=DEP0205 ./node_modules/vite/bin/vite.js build", + "preview": "cross-env NODE_OPTIONS=--disable-warning=DEP0205 vite preview", + "test:unit": "vitest", + "build-only": "cross-env NODE_OPTIONS=--disable-warning=DEP0205 vite build",Per the PR comments summary, the recommended direction for
test:unitwas to move the suppression intovitest.config.ts'stest.poolOptions.forks.execArgv(so it reaches forked workers) and simplify this script back to plainvitest. As written,test:unitstill hardcodes the vitest entrypoint rather than adopting that simplification.🤖 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 `@web/package.json` around lines 7 - 11, The test:unit script still hardcodes the vitest internal entrypoint, which should be simplified to use the standard binary. Update the package.json scripts so test:unit invokes plain vitest, and move the DEP0205 suppression into vitest.config.ts under test.poolOptions.forks.execArgv so it applies to forked workers. Keep the same approach for the Vite-related scripts by relying on package manager bin resolution instead of direct node_modules paths.
🤖 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 `@web/package.json`:
- Around line 7-11: The npm scripts in web/package.json are using the
`--disable-warning` flag unconditionally even though the declared Node range
includes versions that do not support it. Update the `dev`, `preview`,
`test:unit`, and `build-only` script entries to avoid passing that flag on
unsupported Node versions, either by wrapping the Vite invocation in a small
Node version/feature check or by removing the flag from these scripts and
applying it only when the runtime supports it.
---
Nitpick comments:
In `@web/package.json`:
- Around line 7-11: The test:unit script still hardcodes the vitest internal
entrypoint, which should be simplified to use the standard binary. Update the
package.json scripts so test:unit invokes plain vitest, and move the DEP0205
suppression into vitest.config.ts under test.poolOptions.forks.execArgv so it
applies to forked workers. Keep the same approach for the Vite-related scripts
by relying on package manager bin resolution instead of direct node_modules
paths.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9933267-23fb-44f6-af82-7a7e8f093494
📒 Files selected for processing (4)
web/package.jsonweb/src/stores/__tests__/authStore.test.tsweb/src/stores/__tests__/dataStores.test.tsweb/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- web/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/stores/tests/authStore.test.ts
- web/src/stores/tests/dataStores.test.ts
| "dev": "node --disable-warning=DEP0205 ./node_modules/vite/bin/vite.js", | ||
| "build": "run-p type-check \"build-only {@}\" --", | ||
| "preview": "vite preview", | ||
| "test:unit": "vitest", | ||
| "build-only": "vite build", | ||
| "preview": "node --disable-warning=DEP0205 ./node_modules/vite/bin/vite.js preview", | ||
| "test:unit": "node --disable-warning=DEP0205 ./node_modules/vitest/vitest.mjs", | ||
| "build-only": "node --disable-warning=DEP0205 ./node_modules/vite/bin/vite.js build", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the declared engines range and confirm no other guard exists
cat web/package.json | jq '.engines'
fd -HI '.nvmrc|.tool-versions' webRepository: terarush/ping-uptime
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web/package.json =="
cat -n web/package.json | sed -n '1,80p'
echo
echo "== vitest config candidates =="
fd -HI 'vitest.config.*' webRepository: terarush/ping-uptime
Length of output: 2572
Guard --disable-warning behind the supported Node range web/package.json:7-11 still declares node: "^20.19.0 || >=22.12.0", but --disable-warning only exists in Node 21.3+. These scripts will fail on 20.19.0 with an unknown option error. Use a version check/feature-detect wrapper, or drop the flag from the script and apply it only where supported.
🤖 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 `@web/package.json` around lines 7 - 11, The npm scripts in web/package.json
are using the `--disable-warning` flag unconditionally even though the declared
Node range includes versions that do not support it. Update the `dev`,
`preview`, `test:unit`, and `build-only` script entries to avoid passing that
flag on unsupported Node versions, either by wrapping the Vite invocation in a
small Node version/feature check or by removing the flag from these scripts and
applying it only when the runtime supports it.
1 participant
Summary
try/finallycleanupDEP0205suppression for Node's deprecatedmodule.register()warningVerification
npm run type-check --prefix webnpm run build-only --prefix webnpm run test:unit --prefix web -- --runnpm run dev --prefix websmoke test with grep confirming noDEP0205ormodule.register()deprecation outputnpm run test:unit --prefix websmoke test with grep confirming nolocalStorage is not available,--localstorage-file was not provided,ExperimentalWarning, orDEPRECATEDoutputInitiative Context
Final Prompt
okay, that's good, but have warning again
(node:107773) [DEP0205] DeprecationWarning:
module.register()is deprecated. Usemodule.registerHooks()instead.this one, fix it
Final Plan
DEP0205warning and determine whether it comes from repo code or upstream Vite/Vitest bootstrap.Summary by CodeRabbit