From 9b46d3e1374933714341fb3bcc7f16c97d55a014 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 20 Jul 2026 22:03:52 -0700 Subject: [PATCH] fix(test): close the Node-version guard's remaining coverage gap (#7627) #7619 wired scripts/check-node-version.mjs as a pretest* hook on 5 npm script names (test, test:ci, test:coverage, test:workers, ui:test). A repo-wide audit found this misses 8 of 12 vitest-invoking script names (test:unit, test:integration, test:upstream-contract, test:engine-parity, test:live-gate-parity, test:driver-parity, test:changed, test:watch) -- npm's pre hook only fires for the exact script name it's wired to. Worse: this class of mechanism can never cover a direct `npx vitest run test/unit/.test.ts` invocation -- which reference.md and SKILL.md both explicitly recommend for fast iteration -- since that command doesn't go through any npm script at all. Add test/helpers/vitest-global-setup-node-version.ts, wired as `globalSetup` in every vitest.config.ts in the repo (root, workers, and every workspace with its own config). globalSetup runs once before any test file regardless of invocation path, closing the gap completely instead of requiring anyone to remember a matching pretest* entry for every current and future script name. Verified directly on Node 26: a direct `npx vitest run ` now fails immediately via globalSetup, both at the root and inside apps/loopover-ui -- no confusing downstream test failures, one clear message. The existing pretest* hooks stay as a genuinely-faster fail for the 5 high-traffic commands (they run before npm even spawns vitest); check-node-version.mjs's header comment now describes globalSetup as the real guarantee and the hooks as a nicety on top. Closes #7627 --- .../contributing-to-loopover/reference.md | 13 ++++++++++++ .../loopover-miner-extension/vitest.config.ts | 1 + apps/loopover-miner-ui/vitest.config.ts | 1 + apps/loopover-ui/vitest.config.ts | 1 + packages/loopover-ui-kit/vitest.config.ts | 1 + scripts/check-node-version.mjs | 14 +++++++++++-- .../vitest-global-setup-node-version.ts | 20 +++++++++++++++++++ vitest.config.ts | 1 + vitest.workers.config.ts | 1 + 9 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 test/helpers/vitest-global-setup-node-version.ts diff --git a/.claude/skills/contributing-to-loopover/reference.md b/.claude/skills/contributing-to-loopover/reference.md index 63c5a32c4a..94963bb002 100644 --- a/.claude/skills/contributing-to-loopover/reference.md +++ b/.claude/skills/contributing-to-loopover/reference.md @@ -56,6 +56,19 @@ jobs run only if their path filter matched; on push to `main`, everything runs. | ui → tests | vitest jsdom (UI) | `npm run ui:test` | failing UI component test | | ui → build | UI build | `npm run ui:build` | build failure (note: it re-runs `ui:openapi` internally) | +**The Node-version guard: `test/helpers/vitest-global-setup-node-version.ts`, not just `pretest*`.** +Every `vitest.config.ts` in the repo (root, `vitest.workers.config.ts`, and every workspace with its own +config) wires this as `globalSetup`, so it runs once before any test file regardless of how vitest was +invoked — including a direct `npx vitest run test/unit/.test.ts`, which this doc's own §6 ("Iterate, +then verify") recommends for fast iteration. `scripts/check-node-version.mjs`'s `pretest*` hooks +(`package.json`) only cover 5 high-traffic npm script names (`test`, `test:ci`, `test:coverage`, +`test:workers`, `ui:test`) as a genuinely-faster fail there (before npm even spawns vitest) — they are a +nicety on top of the globalSetup guarantee, not a substitute for it. An earlier version of this guard was +`pretest*`-only and missed 8 of 12 vitest-invoking script names, plus every direct `npx vitest` call +structurally (caught by a repo-wide audit shortly after it shipped, #7619-follow-up) — if you add a new +top-level test script or workspace, you do not need to remember a matching `pretest*` entry; the +`globalSetup` wiring already covers it. + **Verifying the built artifact actually serves — do not use `apps/loopover-ui`'s `preview` in the raw TanStack Start template sense.** `apps/loopover-ui` targets nitro's `cloudflare-module` preset (`vite.config.ts`), which repackages the server build as `dist/server/index.mjs`. `@tanstack/start-plugin-core`'s diff --git a/apps/loopover-miner-extension/vitest.config.ts b/apps/loopover-miner-extension/vitest.config.ts index 67700fbf12..c84486df30 100644 --- a/apps/loopover-miner-extension/vitest.config.ts +++ b/apps/loopover-miner-extension/vitest.config.ts @@ -4,6 +4,7 @@ export default defineConfig({ test: { environment: "node", globals: true, + globalSetup: ["../../test/helpers/vitest-global-setup-node-version.ts"], include: ["test/**/*.test.ts"], coverage: { provider: "v8", diff --git a/apps/loopover-miner-ui/vitest.config.ts b/apps/loopover-miner-ui/vitest.config.ts index e3b2e08bfb..fa0a388aff 100644 --- a/apps/loopover-miner-ui/vitest.config.ts +++ b/apps/loopover-miner-ui/vitest.config.ts @@ -7,6 +7,7 @@ export default defineConfig({ test: { environment: "jsdom", globals: true, + globalSetup: ["../../test/helpers/vitest-global-setup-node-version.ts"], setupFiles: ["./vitest.setup.ts"], include: ["src/**/*.test.{ts,tsx}"], coverage: { diff --git a/apps/loopover-ui/vitest.config.ts b/apps/loopover-ui/vitest.config.ts index 158f9c0518..a7a6168772 100644 --- a/apps/loopover-ui/vitest.config.ts +++ b/apps/loopover-ui/vitest.config.ts @@ -10,6 +10,7 @@ export default defineConfig({ test: { environment: "jsdom", globals: true, + globalSetup: ["../../test/helpers/vitest-global-setup-node-version.ts"], setupFiles: ["./vitest.setup.ts"], include: ["src/**/*.test.{ts,tsx}"], }, diff --git a/packages/loopover-ui-kit/vitest.config.ts b/packages/loopover-ui-kit/vitest.config.ts index 4d8c00fc3e..5eb6d63a40 100644 --- a/packages/loopover-ui-kit/vitest.config.ts +++ b/packages/loopover-ui-kit/vitest.config.ts @@ -12,6 +12,7 @@ export default defineConfig({ test: { environment: "jsdom", globals: true, + globalSetup: ["../../test/helpers/vitest-global-setup-node-version.ts"], setupFiles: ["./vitest.setup.ts"], include: ["src/**/*.test.{ts,tsx}"], }, diff --git a/scripts/check-node-version.mjs b/scripts/check-node-version.mjs index 7d8606889a..661a592f7e 100644 --- a/scripts/check-node-version.mjs +++ b/scripts/check-node-version.mjs @@ -5,8 +5,18 @@ // followed by simply switching the active `node` (nvm/homebrew default change) with no reinstall, sails // straight past engine-strict on every later `npm run`. That's exactly the shape of gap that let the // Node 26 jsdom/localStorage bug (#7592/#7597/#7612) go unnoticed the first two times: a pile of -// confusing downstream test failures instead of one clear "wrong Node version" message up front. Wired as -// a `pretest*` hook (see package.json) on the commands people actually run vitest through. +// confusing downstream test failures instead of one clear "wrong Node version" message up front. +// +// The actual, complete guarantee is test/helpers/vitest-global-setup-node-version.ts, wired as every +// vitest.config.ts's `globalSetup` (root, workers, and every workspace with its own config) -- it covers +// every invocation path, including a direct `npx vitest run test/unit/.test.ts` (which the +// contributing skill docs explicitly recommend for fast iteration), not just specific npm script names. +// This module is ALSO wired as a `pretest*` hook (see package.json) on the highest-traffic commands +// (test, test:ci, test:coverage, test:workers, ui:test) as a genuinely-faster fail there -- it runs +// before npm even spawns vitest, vs. globalSetup which still pays vitest's own startup cost first -- but +// that hook is a nicety on top of the globalSetup guarantee, not a substitute for it; it was originally +// (incompletely) the only mechanism, missing 8 of 12 vitest-invoking script names (#7592-class gap, +// caught by a repo-wide audit after #7619 shipped). import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import semver from "semver"; diff --git a/test/helpers/vitest-global-setup-node-version.ts b/test/helpers/vitest-global-setup-node-version.ts new file mode 100644 index 0000000000..d3c03f53bf --- /dev/null +++ b/test/helpers/vitest-global-setup-node-version.ts @@ -0,0 +1,20 @@ +import { checkNodeVersion } from "../../scripts/check-node-version.mjs"; + +/** + * Vitest `globalSetup` -- the only mechanism that covers *every* vitest invocation path, not just + * specific npm script names. scripts/check-node-version.mjs's `pretest*` hooks (package.json) only fire + * for the exact npm script names they're wired to -- they can never cover a direct + * `npx vitest run test/unit/.test.ts` invocation, which the contributing skill docs explicitly + * recommend for fast iteration (reference.md, SKILL.md). A globalSetup runs once before any test file, + * regardless of how vitest was invoked, so this is the real backstop; the pretest hooks are a + * (redundant but harmless) earlier fail for the specific commands they cover. + */ +export default function setup(): void { + const { ok, requiredRange, nodeVersion } = checkNodeVersion(); + if (!ok) { + throw new Error( + `Running Node ${nodeVersion}, but this repo requires ${requiredRange} (see .nvmrc / package.json engines). ` + + "Switch to the pinned Node version (e.g. `nvm use`) before running tests.", + ); + } +} diff --git a/vitest.config.ts b/vitest.config.ts index 2289653013..851063f032 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -16,6 +16,7 @@ export default defineConfig({ environment: "node", globals: true, testTimeout: 15000, + globalSetup: ["./test/helpers/vitest-global-setup-node-version.ts"], // Retry a failed test once before failing the run. The loopover gate auto-CLOSES a contributor PR // on a red required CI, so a single transient flake must not kill an honest PR; a deterministic // failure still fails both attempts (and vitest flags the retried test as flaky so it stays visible). diff --git a/vitest.workers.config.ts b/vitest.workers.config.ts index 2b6106f69a..a1924d8567 100644 --- a/vitest.workers.config.ts +++ b/vitest.workers.config.ts @@ -9,6 +9,7 @@ export default defineConfig({ ], test: { globals: true, + globalSetup: ["./test/helpers/vitest-global-setup-node-version.ts"], // Retry once before failing — a transient flake must not red the required CI and one-shot-close a PR. retry: 1, include: ["test/workers/**/*.test.ts"],