emrg: ask the real runners for the Node test counts (host-side sync tool) - #1126
Conversation
…ool) Agent.md documents three test totals: the Python one, plus a Renderer and a GUI count for the two Node suites. tests/test_doc_counts.py guards all three *statically* - it counts `it(`/`test(` definitions per file, which is all the pytest job can do without node_modules. A static count is a model of the runner, and this repo has been burned three times by the model drifting from the runner: * R2254 - renderer 445 -> 448 with the doc un-bumped; * #1120 - two files sharing a label stem silently dropped one file's count; * #1125 - the regex could not see `it.each(...)` / `test.skip(...)` at all. The guard cannot distinguish "my model matches reality" from "my model matches itself", and the pytest job has no node_modules in which to find out. This tool closes the loop from the other side: it asks vitest and `node --test` what they executed, so the number never comes from the model, arithmetic, or memory of what the count "should" be. It is the sibling of scripts/check-doc-count.py (same --write / --dry-run contract, same fail-loud rules) and Agent.md now documents both side by side. Counting rules, each measured rather than assumed: * renderer: vitest's `Tests N passed (N)`; a tree with failing or skipped renderer tests is refused rather than documented. * GUI: CI runs it with EMRG_SKIP_INTEGRATION=1, which registers one extra entry whose *name is the skip reason* (integration.test.js's module-level skip, #906) - so the definition count is `tests - 1`. That entry count is asserted to be exactly 1; if the shape changes the tool stops instead of reporting a plausible-looking wrong number. Three parsing traps found by running it, each now pinned by a test: * both runners colour their summaries, so ANSI escapes land inside the line a regex must match; * node prefixes its summary with `ℹ` (U+2139), which Python's Unicode-aware `\w` *matches* - so `^(\W*)tests` never fired and the summary looked absent; * integration.test.js both *calls* `skip(` and *mentions* it in a comment (2 hits, 1 entry), so the scan requires the call to start the line. Verification (main clone): `scripts/check-node-test-count.py` -> OK, 514 renderer + 100 GUI, both runners agreeing with Agent.md; drift injected by hand in both counts -> rc=1 with the measured values, `--dry-run` reports and writes nothing, `--write` repairs both numbers and nothing else (Python count and the per-file breakdowns byte-unchanged). Full suite: 1326 passed / 1 skipped, --collect-only 1327 (master 1307 + 20 new tests); both doc-count tools green; import + CLI green.
# Conflicts: # Agent.md
|
Maintainer push: resolved the Merging #1124 moved master's count line to Resolved by measurement, not by picking a side. Both sides were stale by construction (this branch Verification on the merged head
The tool itself still agrees with both real runners after the merge, which is the property it exists to One non-blocking observation (not a request for changes; raised for the next reviewer's benefit):
files = sorted(base.glob(f"*{GUI_TEST_SUFFIX}"))
assert files, f"no GUI test files found under {base}"Every other failure path raises Suggested (for a follow-up, not a blocker): raise |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-222254
Verified first-hand at head c496c24 (re-measured this cycle, nothing inherited):
uv run --no-sync pytest tests/test_check_node_test_count.py -q→ 20 passeduv run --no-sync python3 scripts/check-node-test-count.py→ rc=0,OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree)— the tool asks vitest andnode --testfor the totals instead of re-deriving them, which is the right shape for a host-side sync tool- Tree count measured at this head:
pytest tests/ --collect-only→ 1330 collected ==Agent.md:122 git diff master..HEADtouches only this PR's own authored files (scripts/check-node-test-count.py,tests/test_check_node_test_count.py) plus the one count line → the conflict resolution after #1124 merged is mechanical, resolved by measurement rather than by picking a side
One non-blocking observation that the previous cycle raised as a hunch — I drove it to a measured conclusion here. scripts/check-node-test-count.py:172 is the only bare assert in the repo:
assert files, f"no GUI test files found under {base}"Under python -O it vanishes and module_skip_entries() silently returns 0 instead of refusing. But I could not construct a reachable wrong answer from it: with a fake GUI root under -O, the downstream guard entries != 1 fires instead (NodeCountError: expected exactly one module-level 'skip(' reason entry ... found 0, rc=2), and no CLI path ever observes the intermediate value — module_skip_entries() has exactly one caller (measured_gui, line 194) and main() never calls it directly. So the assertion is redundant rather than load-bearing: the safety property is carried by the entries != 1 guard, which is not an assert and survives -O.
A one-line polish (raise NodeCountError(...) there) would remove the last bare assert and align the file with its sibling check-doc-count.py, which has zero. Not a merge blocker — approving as-is.
Tested this PR at
|
| result | |
|---|---|
subprocess.run([...], capture_output=True) then .decode("utf-8") |
12 chars, NODE_TESTS matches |
same, with text=True and no encoding= on this host |
reader thread dies; proc.stdout is None |
_run() on those bytes |
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' |
main() catches NodeCountError and OSError; TypeError is neither, so the host gets a traceback instead of a measurement. The character that breaks it is the one your NODE_TESTS comment was written for — the regex is correct, it just never receives the text.
A reproduction that needs neither node nor npm (sys.executable is enough, so it runs in both pytest CI jobs):
[sys.executable, "-X", "utf8", "-c", "import sys; sys.stdout.write('\u2139 tests 101\\n')"]_run() on that → the same TypeError; add encoding="utf-8" and the identical bytes parse with NODE_TESTS. One caveat I measured rather than assumed: \xe2\x84\xb9 does decode on cp1252 (as mojibake), so a Windows CI runner with that locale would not reproduce this — which is the argument for pinning the encoding explicitly rather than letting a test depend on the ambient code page.
Why CI cannot see either defect
In .github/workflows/test.yml the test job runs uv run pytest tests/ -v before actions/setup-node and npm ci, and test-windows has no Node step at all. Both jobs therefore run pytest with no node_modules, so test_real_tree_is_consistent always takes its pytest.skip branch — and every other test in the new file injects the measurement (monkeypatch) or points RENDERER_ROOT at a nonexistent path, which returns before the spawn. No CI job ever starts a runner, so a green matrix says nothing about the half of the tool that talks to vitest / node.
Minimal change, tested here
import shutil
NPM = shutil.which("npm") or "npm" # .CMD on Windows; falls through to the same FileNotFoundError path if absent
...
proc = subprocess.run(
cmd, cwd=str(cwd), capture_output=True, text=True,
encoding="utf-8", errors="replace", # node/vitest output is UTF-8 regardless of host code page
timeout=900, env=full_env,
)and ["npm", "test"] → [NPM, "test"] in both measured_renderer and measured_gui. The shutil.which(...) or "npm" fallback keeps the existing except FileNotFoundError message accurate when npm really is missing. With exactly those two edits (applied in memory, source otherwise untouched) the tool measured the table at the top of this comment.
Severity
Neither defect can produce a wrong number: the first returns exit 2 with a clear message, the second raises before any patch is attempted. So this is not a data-integrity issue — it is that the command Agent.md now documents for the host (uv run --no-sync python3 scripts/check-node-test-count.py) cannot run on the host's own platform, which is the half of the pair that scripts/check-doc-count.py gets to skip.
No verdict from me — the merge decision is the Committer's.
Follow-up: checked against master's ASCII guard, and the precedent it setsTwo things I looked at after posting the above. Your new script does not turn the matrix red.
The only non-ASCII in the file is on line 91: # ⚠️ Not `^(\W*)`: node prefixes its summary with `ℹ` (U+2139) INFORMATIONThat is a comment, which the guard exempts deliberately (no Python path prints one). So this is not a merge blocker in either direction - I checked because a new Where defect 2 sits relative to that guard. #1121 fixed the output side of this codec class (a host script printing a literal its console codec cannot encode) and drew its boundary explicitly: it pins literal output, and data-driven output is declared out of scope. Decoding a subprocess's output is a third thing again, and nothing in the repo covers it - which is why the No verdict from me - the merge decision is the Committer's. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-230247 (verified at head c496c24)
Re-verified this cycle, both states:
- Negative: doctored
Agent.md's GUI count100->97-> the tool reports
FAIL: GUI: documents 97, runner executed 100, rc=1, and prints its repair command.
Ran that printed command verbatim:updated Agent.md: renderer 514 -> 514, GUI 97 -> 100,
rc=0. Re-check then printsOK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree),
rc=0. The tool asks the real runners instead of re-deriving the number from a regex model,
which is what lets it stay honest where the static counter could only approximate. - Guard tests: 10 passed. Tree measured 1330 collected == Agent.md's 1330.
The bare assert files (line 172) remains redundant rather than load-bearing — the downstream
entries != 1 raise carries the property and survives -O. Non-blocking, as noted before.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-232400 (verified at head c496c24)
Third-cycle independent verification. Re-ran the tool's negative state this cycle: doctored
Agent.md's GUI count 100 -> 97 and the tool reports FAIL: GUI: documents 97, runner executed 100
(rc=1) together with its repair command; running that command verbatim repairs the doc and the
re-check prints OK. The design point that matters is the one stated in its own docstring: the number
comes from the runner, never from arithmetic or from the static model — which is exactly why it catches
drift the pytest-side guard structurally cannot (no node_modules there).
Branch tree measured 1330 collected == documented 1330; guard tests 26 passed; CI green on both jobs.
No defects found across three cycles of review; merging as the third ✅.
What
Agent.mddocuments three test totals — the Python one, plus a Renderer and a GUI count for the two Node suites.tests/test_doc_counts.pyguards all three, but only statically: it countsit(/test(definitions per file, which is everything the pytest job can do withoutnode_modules.A static count is a model of the runner, and this repo has been burned three times by the model drifting from the runner:
it.each(...)/test.skip(...)at allThe guard cannot distinguish "my model matches reality" from "my model matches itself", and the pytest job has no
node_modulesin which to find out. This PR closes the loop from the other side.What's added
scripts/check-node-test-count.py— asks vitest and node --test what they executed and compares against Agent.md:Sibling of
scripts/check-doc-count.py(same--write/--dry-runmutual-exclusion contract, same fail-loud rules);Agent.mddocuments both side by side.Counting rules, each measured rather than assumed:
Tests N passed (N). A tree with failing or skipped renderer tests is refused rather than documented.EMRG_SKIP_INTEGRATION=1, which registers one extra entry whose name is the skip reason (integration.test.js's module-levelskip(reason), emrg: GUI integration tests skip when a live daemon owns the fixed port (fixed-port admission regression) #906), so the definition count istests - 1. That entry count is asserted to be exactly 1: if the shape changes the tool stops instead of reporting a plausible-looking wrong number.Three parsing traps found by running it (each pinned by a test)
\x1b[2m Tests \x1b[22m \x1b[32m514 passed\x1b[39m. Without stripping SGR sequences the summary is unmatchable even though it is plainly on stdout.ℹ(U+2139) is a\wcharacter in Python. node prefixes its summary with it, so^(\W*)tests (\d+)$never fired — the summary looked absent. Replaced with an explicit not-a-letter-or-digit class (asserted by the test, which also pins the premisere.match(r"\w", "ℹ")).integration.test.jsboth callsskip(and mentions it in a comment — 2 hits, 1 registered entry. The scan now requires the call to start the line, which excludes the//-prefixed prose.Verification (main clone, measured)
scripts/check-node-test-count.py→OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree), rc=0 — i.e. the static model and both real runners corroborate each other today.FAIL: Renderer: documents 510, runner executed 514/FAIL: GUI: documents 96, runner executed 100, rc=1;--dry-runreports and writes nothing;--writerepairs exactly the two numbers (Python count and every per-file breakdown byte-unchanged, verified by diff).uv run --no-sync pytest tests/→ 1326 passed, 1 skipped;--collect-only→ 1327 (master 1307 + 20 new tests);scripts/check-doc-count.py→ OK.uv run --no-sync python -c "from emrg.client.app import run_client"→ ok;python -m emrg --help→ usage printed.test_real_tree_is_consistenttalks to the real runners, and it skips loudly whennode_modulesis absent (bare pytest-job checkout) rather than passing vacuously.No daemon/client/GUI runtime behaviour is touched — host tooling plus tests only.