Skip to content

Commit cd33964

Browse files
kingpanther13claude
andcommitted
test(internal): address Gemini + pr-review-toolkit findings on JS test infrastructure
Outcome of going through all 60 findings from Gemini Code Assist + pr-review-toolkit (code-reviewer, pr-test-analyzer, silent-failure- hunter, comment-analyzer). 3 wrong (skipped: PATH-resolved node binary mis-flagged as hardcoded; project-relative esbuild path mis-flagged as hardcoded; theoretical FakeBroadcastChannel constructor-throw). Remaining real items addressed: Harness: - vm.runInContext replaces window.eval / indirect eval to clear the "no eval()" style-guide flag (Gemini #1, #2). - Timer-callback and broadcast-listener throws now record into the errors list instead of being silently swallowed (homeassistant-ai#30, homeassistant-ai#32). - SAFETY_CAP exhaustion records a clear "runaway setInterval" error instead of breaking silently (#4, homeassistant-ai#31). - Non-navigation jsdomErrors route to errors (not console) so tests asserting `not result.errors` catch them (homeassistant-ai#38). - Transpile failure short-circuits init eval to avoid cascading syntax errors from un-transpiled TS (homeassistant-ai#34). - FakeBroadcastChannel.postMessage now delivers to peer same-name channels in the same context per spec (#5). - Time-faked surface documented accurately (Date.now / setTimeout / setInterval only; new Date / performance.now still wall-time) (homeassistant-ai#46). - New broadcastChannelUnavailable param simulates the `typeof BroadcastChannel === 'undefined'` browsing context so the production null-guard branch is exercised (homeassistant-ai#15). - Dead comments and rot-prone duplications removed (homeassistant-ai#47, homeassistant-ai#49, homeassistant-ai#51, homeassistant-ai#56, homeassistant-ai#57, homeassistant-ai#58, homeassistant-ai#59, homeassistant-ai#66). extract_astro_vars.mjs: - vm.runInContext replaces (0, eval) (Gemini #2). - Multi-line `import { a, b } from 'x';` now stripped robustly (#7). - Eval errors wrapped with the source path for actionable failures (homeassistant-ai#35). _js_harness.py: - Wrong test file name and workflow path in docstring fixed (homeassistant-ai#41, homeassistant-ai#42). - _strip_astro_frontmatter raises ValueError when frontmatter opens but never closes (homeassistant-ai#36). - discover_script_surfaces raises when site/src/ is missing instead of silently producing partial results (homeassistant-ai#37). - extract_script_body accepts source_label for actionable errors (homeassistant-ai#40). - Astro `<script lang="js">` is no longer mis-tagged as TypeScript (#9). - Inert chr(92) Windows backslash replace removed (homeassistant-ai#14). - Field docstrings on ScriptSurface trimmed to the one that earns its keep (homeassistant-ai#52). - _PY_RENDERERS registry refactor + accurate enumeration comment (homeassistant-ai#45). test_settings_ui_js_behavior.py: - Rot-bait PR/issue numbers removed from module docstring (homeassistant-ai#43). - _TOP_LEVEL_ELEMENT_IDS + import-time drift check replaces the "refresh this manually" comment (homeassistant-ai#55). - _assert_clean_init helper called at the top of every test so init failures surface as init errors, not as misleading "side effect didn't fire" failures (homeassistant-ai#33). - 4xx restartBtn assertion now reads disabled state via JS and snaps to body.dataset instead of OR-shortcircuiting against a wiped DOM (homeassistant-ai#27). - New test_script_boots_without_broadcastchannel_global covers the null-guard branch (homeassistant-ai#15). - Assertion-restating comments removed (homeassistant-ai#60). test_astro_setup_js_behavior.py: - Rot-bait homeassistant-ai#1422 reference removed from module docstring (homeassistant-ai#44). - _section_has_hidden_class replaces fragile substring slicing (#6). - test_initial_state_only_client_section_visible now asserts on the promised visibility, not just absence of errors (homeassistant-ai#26). - Per-client smoke now captures config-output text AND instructions HTML into body.dataset and asserts on non-empty content, catching a typo that drops the whole per-client branch (homeassistant-ai#21). test_astro_tools_js_behavior.py: - _card_class helper replaces ±200-char substring slicing (#12). - test_design_mode_toggle now asserts design-only elements lose 'hidden' class, not just the button label flip (homeassistant-ai#25). - New tests cover .filter-btn / .cat-btn / .size-filter-btn / group-category|file|none / sort-alpha / expand-all wiring (homeassistant-ai#22, homeassistant-ai#23, homeassistant-ai#24) — the adjacent coverage gaps issue homeassistant-ai#1422 didn't name but that fit the harness's same regression-class. test_consent_form_js_behavior.py: - _build_form_dom docstring fixed (said "three", listed four) (homeassistant-ai#54). test_rendered_scripts_parse.py: - Missing-dependency skip flips to fail when CI=true so a workflow drift that drops the install step doesn't silently lose parse coverage (homeassistant-ai#29). - Subsumed-test-class reference removed from module docstring. AGENTS.md: - "60s probe windows take milliseconds" wording fixed; time-faked surface documented (homeassistant-ai#13). - Per-surface module naming guidance updated; reflects actual files (#10). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a893e00 commit cd33964

9 files changed

Lines changed: 1076 additions & 561 deletions

AGENTS.md

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -570,11 +570,14 @@ assert result.reloads == 1
570570
assert result.broadcasts_of_type("restart-required")
571571
```
572572

573-
The harness fakes time (`setTimeout` / `Date.now` on a virtual clock —
574-
60s probe windows take milliseconds), stubs `fetch` from a URL pattern
575-
map, captures `location.reload` via JSDOM's `jsdomError` channel
576-
(unforgeable IDL property), and provides a `BroadcastChannel` shim that
577-
can be primed with cross-tab events.
573+
The harness fakes `setTimeout` / `setInterval` / `Date.now` on a
574+
virtual clock (a 60 s production probe completes in milliseconds of
575+
wall time), stubs `fetch` from a URL pattern map (with optional
576+
`responses: [...]` sequencing for state-flip flows), captures
577+
`location.reload` via JSDOM's `jsdomError` channel (unforgeable IDL
578+
property), and provides a `BroadcastChannel` shim that can be primed
579+
with cross-tab events. `new Date()` / `performance.now()` continue to
580+
report wall time — only the three sources above are faked.
578581

579582
Astro `<script>` blocks without `define:vars` / `is:inline` are
580583
TypeScript by default — pass `language="ts"` to `run_script` and the
@@ -592,9 +595,17 @@ result = run_script(script, prelude=prelude, ...)
592595
CI installs Node + jsdom in the `unit-tests` job (`.github/workflows/pr.yml`).
593596
Local devs without `tests/js/node_modules/` get clean skips.
594597

595-
When adding a new UI surface: drop the file, add behavioural tests in
596-
`tests/src/unit/test_*_js_behavior.py` mirroring the existing per-
597-
surface modules; parse coverage is automatic.
598+
When adding a new UI surface:
599+
- Python-rendered HTML: register the renderer in
600+
`_js_harness.py::_PY_RENDERERS` so the auto-discovery walker picks
601+
it up for parse coverage.
602+
- Astro page: drop the `.astro` file under `site/src/`; discovery walks
603+
the tree automatically.
604+
- Behavioural tests: add a `test_<surface>_js_behavior.py` module
605+
alongside the existing ones (`test_settings_ui_js_behavior.py`,
606+
`test_astro_setup_js_behavior.py`, `test_astro_tools_js_behavior.py`,
607+
`test_astro_layout_js_behavior.py`, `test_consent_form_js_behavior.py`)
608+
— pattern is one module per UI surface.
598609

599610
## Setup Wizard (`site/src/pages/setup.astro`)
600611

tests/js/extract_astro_vars.mjs

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
// Outputs: {"clientsData": [...], "platformsData": [...]}
1515

1616
import { readFileSync } from "node:fs";
17+
import { createContext, runInContext } from "node:vm";
1718
import { transformSync } from "esbuild";
1819

1920
function readStdin() {
@@ -34,24 +35,29 @@ function extractFrontmatter(source) {
3435
return m[1];
3536
}
3637

38+
function stripImports(fm) {
39+
// Drop both single-line (`import X from 'y';`) and multi-line
40+
// (`import {\n a,\n b,\n} from 'y';`) import statements. The
41+
// grammar matches `import` at line start optionally followed by
42+
// anything up to the first semicolon, including newlines. Doesn't
43+
// need to be a perfect TS parser — Astro frontmatter imports always
44+
// sit at the top before any other statements.
45+
return fm.replace(/^[ \t]*import\b[\s\S]*?;[ \t]*\n?/gm, "");
46+
}
47+
3748
function sanitiseFrontmatter(fm) {
38-
// Drop imports (would fail to resolve in this context) and lines that
39-
// reach into `import.meta` (Astro-only). Leave const / let / function
40-
// declarations intact so consts the test asks for are still in scope.
41-
//
42-
// Then prepend stubs for the most common Astro-injected globals so
43-
// helper functions in the frontmatter (e.g. `withBase` referencing
44-
// `base = import.meta.env.BASE_URL`) don't ReferenceError when re-
45-
// evaluated outside Astro.
49+
// Drop imports (would fail to resolve in this context) and lines
50+
// that reach into `import.meta` (Astro-only). Then prepend stubs for
51+
// the most common Astro-injected globals so frontmatter helpers
52+
// (e.g. `withBase` referencing `base = import.meta.env.BASE_URL`)
53+
// don't ReferenceError when re-evaluated outside Astro.
4654
const stubs = `const base = "";\n`;
47-
return (
48-
stubs +
49-
fm
50-
.split("\n")
51-
.filter((line) => !/^\s*import\b/.test(line))
52-
.filter((line) => !/\bimport\.meta\b/.test(line))
53-
.join("\n")
54-
);
55+
const noImports = stripImports(fm);
56+
const noImportMeta = noImports
57+
.split("\n")
58+
.filter((line) => !/\bimport\.meta\b/.test(line))
59+
.join("\n");
60+
return stubs + noImportMeta;
5561
}
5662

5763
async function main() {
@@ -61,21 +67,32 @@ async function main() {
6167
const cleaned = sanitiseFrontmatter(extractFrontmatter(src));
6268

6369
// Append a JSON serialiser for each requested name so we get a single
64-
// structured payload back. ``stringify`` runs after every const in
65-
// ``cleaned`` is in scope.
70+
// structured payload back. `stringify` runs after every const in
71+
// `cleaned` is in scope.
6672
const names = req.names || [];
67-
const payload = names.map((n) => `"${n}": typeof ${n} !== 'undefined' ? ${n} : null`);
68-
const program = `${cleaned}\n;process.stdout.write(JSON.stringify({${payload.join(",")}}));`;
73+
const payload = names.map(
74+
(n) => `"${n}": typeof ${n} !== 'undefined' ? ${n} : null`,
75+
);
76+
const program = `${cleaned}\n;__result = JSON.stringify({${payload.join(",")}});`;
6977

7078
const transpiled = transformSync(program, {
7179
loader: "ts",
7280
target: "es2020",
7381
format: "esm",
7482
}).code;
7583

76-
// Evaluate in this same module — esbuild output is plain JS now.
77-
// Using indirect eval keeps the top-level scope clean.
78-
(0, eval)(transpiled);
84+
// vm.runInContext rather than eval so the project's "no eval()" lint
85+
// stays clean. New context per invocation (no globals from the host)
86+
// — `__result` is the only handoff back.
87+
const ctx = createContext({ __result: null });
88+
try {
89+
runInContext(transpiled, ctx, { filename: `astro-vars:${req.path}` });
90+
} catch (e) {
91+
throw new Error(
92+
`evaluating frontmatter of ${req.path}: ${(e && e.stack) || e}`,
93+
);
94+
}
95+
process.stdout.write(ctx.__result ?? "{}");
7996
}
8097

8198
main().catch((e) => {

0 commit comments

Comments
 (0)