From 81e16c4c32b6b2f9f03b3b9309a208b7d25da07f Mon Sep 17 00:00:00 2001 From: Alassane <79718216+yattdev@users.noreply.github.com> Date: Thu, 13 Aug 2026 07:52:31 -0400 Subject: [PATCH] fix(test): pin NODE_ENV so React unit tests can render (#2568) Co-authored-by: ayattara Co-authored-by: Carlos Florencio --- .../frontend-tests-workflow-contract_test.py | 152 ++++++++++++++++++ .github/workflows/frontend-tests.yml | 12 ++ .github/workflows/lint-action-pinning.yml | 9 ++ apps/.npmrc | 7 + apps/web/AGENTS.md | 4 +- apps/web/vitest-environment.test.tsx | 48 ++++++ apps/web/vitest.config.ts | 6 + apps/web/vitest.setup.ts | 13 ++ 8 files changed, 249 insertions(+), 2 deletions(-) create mode 100644 .github/scripts/frontend-tests-workflow-contract_test.py create mode 100644 apps/web/vitest-environment.test.tsx diff --git a/.github/scripts/frontend-tests-workflow-contract_test.py b/.github/scripts/frontend-tests-workflow-contract_test.py new file mode 100644 index 0000000000..88e5a63755 --- /dev/null +++ b/.github/scripts/frontend-tests-workflow-contract_test.py @@ -0,0 +1,152 @@ +#!/usr/bin/env python3 +"""Contract tests for the frontend unit-test workflow's NODE_ENV guard. + +React exports `act()` only from its development build, which it selects with +`process.env.NODE_ENV`. `apps/web/vitest.config.ts` pins that variable to +"test" so the development build always loads; without the pin, every +`render()` throws `TypeError: React.act is not a function` for anyone whose +shell inherits the runtime image's `NODE_ENV=production` (`Dockerfile`). + +The pin cannot guard itself. The CI image sets no `NODE_ENV`, so Vitest's own +`process.env.NODE_ENV ??= "test"` already yields "test" there and the suite +passes whether or not the pin exists. `frontend-tests.yml` therefore exports +`NODE_ENV=production` on its test step on purpose, to reproduce the +environment that broke and make a deleted pin fail in CI. + +These assertions exist because that export reads like a mistake: removing it +breaks nothing visibly, and CI silently stops guarding the fix. +""" + +from pathlib import Path +import re +import unittest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "frontend-tests.yml" +VITEST_CONFIG = REPO_ROOT / "apps" / "web" / "vitest.config.ts" +NPMRC = REPO_ROOT / "apps" / ".npmrc" +LINT_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "lint-action-pinning.yml" + +STEP_MARKER = " - name: Run tests\n" +NEXT_STEP_MARKER = "\n - name: " + +# The subjects above that live outside `.github/`, and so are not covered by +# this job's `.github/workflows/**` trigger. +UNTRIGGERED_SUBJECTS = ("apps/web/vitest.config.ts", "apps/.npmrc") + + +def run_tests_step(workflow: str) -> str: + """Return the body of the `Run tests` step, up to the following step.""" + _, separator, remainder = workflow.partition(STEP_MARKER) + if not separator: + raise AssertionError("frontend-tests.yml has no 'Run tests' step") + return remainder.partition(NEXT_STEP_MARKER)[0] + + +def trigger_paths(workflow: str, trigger: str) -> str: + """Return the `paths:` list of `trigger`'s block under `on:`. + + Scoped to the `paths:` key rather than the whole trigger block. GitHub + rejects `paths` and `paths-ignore` on the same event, so renaming the key + inverts the filter wholesale: the job would then run on everything *except* + these files, and a match against the block as a whole would not notice. + """ + block = re.search(rf"(?m)^ {trigger}:\n(?:^ {{4}}.*\n|^\n)*", workflow) + if block is None: + raise AssertionError(f"lint-action-pinning.yml has no {trigger} trigger") + + paths = re.search(r"(?m)^ paths:\n((?:^ - .*\n)+)", block.group(0)) + if paths is None: + raise AssertionError( + f"lint-action-pinning.yml's {trigger} trigger has no `paths:` list. A " + "`paths-ignore:` key in its place inverts the filter, and the job stops " + "running for the very files listed under it." + ) + return paths.group(1) + + +class FrontendTestsWorkflowContractTest(unittest.TestCase): + def test_test_step_reproduces_the_production_environment(self) -> None: + step = run_tests_step(WORKFLOW.read_text(encoding="utf-8")) + + self.assertIn( + "NODE_ENV: production", + step, + "The 'Run tests' step must export NODE_ENV=production so a deleted " + "NODE_ENV pin in apps/web/vitest.config.ts fails CI. Without it the " + "CI image sets no NODE_ENV, Vitest defaults it to 'test', and the " + "suite passes with or without the pin.", + ) + self.assertIn("pnpm --filter @kandev/web test", step) + + def test_vitest_config_still_pins_the_mode(self) -> None: + config = VITEST_CONFIG.read_text(encoding="utf-8") + + self.assertIn( + 'process.env.NODE_ENV = "test";', + config, + "apps/web/vitest.config.ts must pin NODE_ENV=test. React resolves " + "its production build otherwise, which does not export act(), and " + "every render()/renderHook() throws before reaching an assertion.", + ) + + def test_workspace_installs_keep_dev_dependencies(self) -> None: + npmrc = NPMRC.read_text(encoding="utf-8") + + self.assertIn( + "production=false", + npmrc, + "apps/.npmrc must set production=false. This is not for the " + "workflow's own install step, which runs before the step-scoped " + "export and sees no NODE_ENV. It is for every install performed " + "under the same variable the test step reproduces: inside the " + "runtime image (`Dockerfile`) or any shell inheriting it, pnpm " + "otherwise skips devDependencies, leaving no vitest, eslint, or " + "tsc to run at all.", + ) + + def test_this_contract_runs_when_its_subjects_change(self) -> None: + """The assertions above are only worth anything if CI runs them. + + `lint-action-pinning.yml` triggers on `.github/workflows/**`, which + covers `frontend-tests.yml` but neither subject in + `UNTRIGGERED_SUBJECTS`. `frontend-tests.yml` does not cover + `apps/.npmrc` either, so without an explicit trigger here a PR that + deletes `production=false` and nothing else runs no job at all and the + assertion above never executes. Mirrors the equivalent test in + `release-workflow-contract_test.py`. + + The step assertion at the end is weaker than the path ones by + construction: this file runs only from that step, so the PR that deletes + it also deletes the only thing that would fail. It holds for local runs + and for a step whose command is renamed or moved elsewhere. Keeping the + job a required check is what covers outright removal, and that lives in + branch protection, not here. + """ + lint_workflow = LINT_WORKFLOW.read_text(encoding="utf-8") + + for trigger in ("push", "pull_request"): + paths = trigger_paths(lint_workflow, trigger) + + for subject in UNTRIGGERED_SUBJECTS: + self.assertIn( + f' - "{subject}"\n', + paths, + f"{subject} must be a {trigger} path trigger in " + "lint-action-pinning.yml. It is a subject of this contract " + "but lives outside .github/, so nothing else brings this " + "job up when it changes.", + ) + + self.assertIn( + "\n run: python3 .github/scripts/frontend-tests-workflow-contract_test.py\n", + lint_workflow, + "lint-action-pinning.yml must run this file, as a live step rather " + "than a commented-out one. Without it this file is dead code and " + "every assertion here silently stops guarding.", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index b10a531413..b0d6d9de2e 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.github/workflows/frontend-tests.yml @@ -209,7 +209,19 @@ jobs: node scripts/run-i18n-ratchet.mjs --base "${BASE_SHA}" + # Deliberately run under the variable that broke the suite, so a + # regression fails here instead of only in a container. React exports + # `act()` from its development build alone, `vitest.config.ts` pins + # `NODE_ENV=test` to force that build, and this image sets no NODE_ENV of + # its own — so without this line CI would pass with the pin deleted while + # every `render()` throws `TypeError: React.act is not a function` for + # anyone whose shell inherits the runtime image's `NODE_ENV=production` + # (`Dockerfile`). That gap is exactly how the breakage went unnoticed. + # Scoped to this step: `pnpm install` above must keep seeing devDependencies + # through its own resolution, not through `apps/.npmrc`'s `production=false`. - name: Run tests + env: + NODE_ENV: production run: pnpm --filter @kandev/web test - name: Build diff --git a/.github/workflows/lint-action-pinning.yml b/.github/workflows/lint-action-pinning.yml index 0c8bd27c98..5f937bd575 100644 --- a/.github/workflows/lint-action-pinning.yml +++ b/.github/workflows/lint-action-pinning.yml @@ -8,6 +8,9 @@ on: - ".github/scripts/lint-action-pinning.py" - ".github/scripts/lint-action-pinning_test.py" - ".github/scripts/claude-code-review-workflow-contract_test.py" + - ".github/scripts/frontend-tests-workflow-contract_test.py" + - "apps/web/vitest.config.ts" + - "apps/.npmrc" - ".github/scripts/preview-env-workflow-contract_test.py" - ".github/scripts/collect-macos-desktop-diagnostics.sh" - ".github/scripts/release-workflow-contract_test.py" @@ -37,6 +40,9 @@ on: - ".github/scripts/lint-action-pinning.py" - ".github/scripts/lint-action-pinning_test.py" - ".github/scripts/claude-code-review-workflow-contract_test.py" + - ".github/scripts/frontend-tests-workflow-contract_test.py" + - "apps/web/vitest.config.ts" + - "apps/.npmrc" - ".github/scripts/preview-env-workflow-contract_test.py" - ".github/scripts/collect-macos-desktop-diagnostics.sh" - ".github/scripts/release-workflow-contract_test.py" @@ -88,6 +94,9 @@ jobs: - name: Test preview environment workflow contract run: python3 .github/scripts/preview-env-workflow-contract_test.py + - name: Test frontend tests workflow contract + run: python3 .github/scripts/frontend-tests-workflow-contract_test.py + - name: Test release workflow contract run: python3 .github/scripts/release-workflow-contract_test.py diff --git a/apps/.npmrc b/apps/.npmrc index 44955b4334..7d01590339 100644 --- a/apps/.npmrc +++ b/apps/.npmrc @@ -3,3 +3,10 @@ public-hoist-pattern[]=@lezer/* confirm-modules-purge=false +# This workspace is installed for development, testing, and builds. The runtime +# image ships prebuilt Go and SPA artifacts and does not install `apps/` itself. +# Without this, `pnpm install` reports "devDependencies: skipped because NODE_ENV +# is set to production" inside the runtime container image (`Dockerfile` sets +# `NODE_ENV=production`), leaving no vitest, eslint, or tsc. See the same variable's +# effect on React's build variant in `web/vitest.config.ts`. +production=false diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index e0845fbaca..4d7192621f 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -295,6 +295,6 @@ plugin leaks a stale registration. ## Testing notes +- `vitest.config.ts` pins `process.env.NODE_ENV = "test"`, and that line is load-bearing. React exports `act()` only from its development build, so under `NODE_ENV=production` — which the runtime image sets (`Dockerfile`) and every container/agent shell inherits, while CI's image does not — `@testing-library/react` falls back to `react-dom/test-utils` and **every** `render()`/`renderHook()` throws `TypeError: React.act is not a function` before any assertion, with CI still green. `vitest-environment.test.tsx` and the `vitest.setup.ts` preflight fail by name if the pin goes, and the `Run tests` step in `.github/workflows/frontend-tests.yml` exports `NODE_ENV=production` on purpose so those guards fire in CI too rather than only in a container. - jsdom secure cookies need cookie-setter interception; Radix Tooltip tests use keyboard focus, while Playwright covers pointer hover with `locator.hover()`. -- Scope terminal selectors to the active panel/container; mobile and dockview may - mount multiple instances, so shared helpers must not use global selectors. +- Scope terminal selectors to the active panel/container; mobile and dockview may mount multiple instances, so shared helpers must not use global selectors. diff --git a/apps/web/vitest-environment.test.tsx b/apps/web/vitest-environment.test.tsx new file mode 100644 index 0000000000..940964c787 --- /dev/null +++ b/apps/web/vitest-environment.test.tsx @@ -0,0 +1,48 @@ +import { afterEach, describe, expect, it } from "vitest"; +import * as React from "react"; +import { cleanup, render, screen } from "@testing-library/react"; + +/** + * Guards the test environment itself rather than any product code. + * + * `react/index.js` picks its build from `process.env.NODE_ENV`, and + * `cjs/react.production.js` does not export `act`. When that build is loaded, + * `@testing-library/react`'s act-compat shim silently falls back to the + * deprecated `react-dom/test-utils`, whose production build calls `React.act` + * and throws `TypeError: React.act is not a function` on the first `render()` + * of every suite — 178 of 268 tests, none of them reaching an assertion. + * + * That is not hypothetical: the Kandev runtime image exports + * `NODE_ENV=production` (`Dockerfile`), Vitest only *defaults* the variable + * (`process.env.NODE_ENV ??= "test"`) rather than pinning it, and CI's image + * sets none — so the whole frontend suite was dead inside the container while + * CI stayed green. `vitest.config.ts` now pins the value; this file is what + * makes a regression legible, as one named failure instead of every + * render-based suite dying on a stack inside `node_modules`. + * + * The `render` case is the durable one: it keeps holding if the cause changes + * to a duplicated `react` instance or a bad alias, which the two cheaper + * assertions would miss. + */ + +const PROBE_TEXT = "vitest-environment-probe"; + +afterEach(() => { + cleanup(); +}); + +describe("vitest environment", () => { + it("runs with NODE_ENV=test so React loads its development build", () => { + expect(process.env.NODE_ENV).toBe("test"); + }); + + it("exposes React.act, which only the development build exports", () => { + expect(typeof React.act).toBe("function"); + }); + + it("can render a component to the DOM", () => { + render(
{PROBE_TEXT}
); + + expect(screen.getByTestId("vitest-environment-probe").textContent).toBe(PROBE_TEXT); + }); +}); diff --git a/apps/web/vitest.config.ts b/apps/web/vitest.config.ts index 8934208956..7ca73bc66d 100644 --- a/apps/web/vitest.config.ts +++ b/apps/web/vitest.config.ts @@ -4,6 +4,9 @@ import viteConfig from "./vite.config"; if (process.env.DEBUG === "1") process.env.DEBUG = ""; +// Pins NODE_ENV=test — see apps/web/AGENTS.md "Testing notes" for why this is load-bearing. +process.env.NODE_ENV = "test"; + const configuredMaxWorkers = process.env.VITEST_MAX_WORKERS?.trim(); const maxWorkers = resolveMaxWorkers(configuredMaxWorkers, Boolean(process.env.CI)); @@ -28,6 +31,9 @@ export default mergeConfig( exclude: ["e2e/**/*.spec.ts", "e2e/fixtures/**", "e2e/pages/**", "node_modules/**"], pool: "threads", maxWorkers, + // Already the default, pinned because it is load-bearing: a run that + // collects nothing must exit non-zero rather than read as a green suite. + passWithNoTests: false, }, }), ); diff --git a/apps/web/vitest.setup.ts b/apps/web/vitest.setup.ts index 5b2800938f..01105b7916 100644 --- a/apps/web/vitest.setup.ts +++ b/apps/web/vitest.setup.ts @@ -1,7 +1,20 @@ import type { Window as HappyDOMWindow } from "happy-dom"; +import * as React from "react"; import { initI18nForTests, loadAllLocalesForTests } from "./lib/i18n"; +// Guards against `react` production build or duplicate copy — both make `act` unavailable; +// vitest.config.ts pins NODE_ENV=test for the common case. Namespace import is deliberate. +if (typeof React.act !== "function") { + throw new Error( + `React.act is unavailable, so no test can render. React only exports act() from its ` + + `development build, and NODE_ENV is "${process.env.NODE_ENV}" here. ` + + `Check the NODE_ENV pin in vitest.config.ts, then that only one copy of react is installed ` + + `(pnpm why react). Without it @testing-library/react falls back to react-dom/test-utils and ` + + `every render() throws "TypeError: React.act is not a function".`, + ); +} + /** * i18n test bootstrap. *