Skip to content

emrg: ask the real runners for the Node test counts (host-side sync tool) - #1126

Merged
argszero merged 2 commits into
masterfrom
feature/node-test-count-sync
Sep 10, 2026
Merged

emrg: ask the real runners for the Node test counts (host-side sync tool)#1126
argszero merged 2 commits into
masterfrom
feature/node-test-count-sync

Conversation

@argszero

Copy link
Copy Markdown
Owner

What

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, but only statically: it counts it(/test( definitions per file, which is everything 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:

incident what the model missed
R2254 renderer grew 445 → 448 with the doc un-bumped
#1120 two files sharing a label stem silently dropped one file's entire 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 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:

uv run --no-sync python3 scripts/check-node-test-count.py           # report drift, exit 1
uv run --no-sync python3 scripts/check-node-test-count.py --dry-run  # show the change
uv run --no-sync python3 scripts/check-node-test-count.py --write    # rewrite Agent.md

Sibling of scripts/check-doc-count.py (same --write/--dry-run mutual-exclusion contract, same fail-loud rules); Agent.md 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(reason), emrg: GUI integration tests skip when a live daemon owns the fixed port (fixed-port admission regression) #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 pinned by a test)

  1. ANSI escapes land inside the matched line. Both runners colour their summaries: \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.
  2. (U+2139) is a \w character 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 premise re.match(r"\w", "ℹ")).
  3. integration.test.js both calls skip( 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.pyOK: 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.
  • Drift injected by hand in both countsFAIL: Renderer: documents 510, runner executed 514 / FAIL: GUI: documents 96, runner executed 100, rc=1; --dry-run reports and writes nothing; --write repairs 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-only1327 (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.
  • The 20 new tests inject both measurements; only test_real_tree_is_consistent talks to the real runners, and it skips loudly when node_modules is absent (bare pytest-job checkout) rather than passing vacuously.

No daemon/client/GUI runtime behaviour is touched — host tooling plus tests only.

EMRG Evolution added 2 commits September 10, 2026 21:23
…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
@argszero

Copy link
Copy Markdown
Owner Author

Maintainer push: resolved the Agent.md conflict caused by the merge of #1124 (3ff6caf).

Merging #1124 moved master's count line to 1310, which left this PR CONFLICTING — and GitHub runs
no CI at all on a conflicting PR, so it could not be reviewed in that state.

Resolved by measurement, not by picking a side. Both sides were stale by construction (this branch
said 1327, master said 1310; the merged tree collects 1330), so I dropped both numbers and asked
the tool for the truth on the merged tree:

$ uv run --no-sync python3 scripts/check-doc-count.py --write
updated Agent.md: 0 -> 1330

Verification on the merged head c496c24

Check Result
pytest tests/test_check_node_test_count.py 20 passed
pytest tests/ -q 1329 passed, 1 skipped
pytest tests/ --collect-only 1330 collected == documented 1330
scripts/check-node-test-count.py on the merged tree OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree)
mergeStateStatus MERGEABLE, CI re-triggered

The tool itself still agrees with both real runners after the merge, which is the property it exists to
assert.


One non-blocking observation (not a request for changes; raised for the next reviewer's benefit):

module_skip_entries() is the only error path in this tool that uses a bare assert:

files = sorted(base.glob(f"*{GUI_TEST_SUFFIX}"))
assert files, f"no GUI test files found under {base}"

Every other failure path raises NodeCountError so main() can print error: … and return 2. A bare
assert disappears entirely under python -O/-OO (and PYTHONOPTIMIZE), so on an interpreter run
that way the "no GUI test files" state would not stop the tool — it would silently fall through to
sum() over an empty list and report a 0 GUI count as if it were measured. The sibling
scripts/check-doc-count.py has no bare assert at all, so the two tools differ in shape here.

Suggested (for a follow-up, not a blocker): raise NodeCountError with the same message. The failure
mode is narrow, but "reports a plausible number instead of refusing" is the exact class of error this
tool was written to prevent, so it is worth closing while the reasoning is fresh.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 -q20 passed
  • uv run --no-sync python3 scripts/check-node-test-count.pyrc=0, OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree) — the tool asks vitest and node --test for 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-only1330 collected == Agent.md:122
  • git diff master..HEAD touches 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.

@pm25coder

Copy link
Copy Markdown
Collaborator

Tested this PR at c496c24 on Windows — the numbers reproduce, but the tool cannot run here as shipped

I fetched scripts/check-node-test-count.py at c496c24 and drove it on the platform Agent.md's new Node count sync: line tells the host to run. Design first, because it matters for what follows: asking the runners instead of the model is the right answer to the R2254 / #1120 / #1125 history, and the three parsing traps you pinned (ANSI inside the summary, being a \w char, skip( call vs comment) all read as measured rather than assumed.

Good news: with two changes applied in memory (below), your numbers reproduce on a second host, independently of yours:

documents: (514, 100)          # as Agent.md claims
renderer (vitest):     measured = 514   [74.2s]
GUI (node --test):     measured = 100   [3.1s]

(Windows Server 2022, Python 3.13.9, node v22.16.0 / npm 10.9.2, tree at 08a7762 — the renderer/GUI suites are unaffected by that branch's dirty file. Your claim "both runners agree with Agent.md" holds here too.)

Unmodified, the same call path fails twice on this host:

measured_renderer() -> NodeCountError: cannot run 'npm': [WinError 2] 系统找不到指定的文件。
measured_gui()      -> NodeCountError: cannot run 'npm': [WinError 2] 系统找不到指定的文件。

Defect 1 — _run's argv does not resolve npm on Windows

_run calls subprocess.run(["npm", "test"], ...). List-form subprocess goes through CreateProcess, which appends only .exe; npm on Windows is npm.CMD. Measured:

shutil.which("npm")               -> C:\Program Files\nodejs\npm.CMD
subprocess.run(["npm", "--version"])     -> FileNotFoundError [WinError 2]
subprocess.run(["npm.cmd", "--version"]) -> rc=0, 10.9.2

Two-state, to isolate the cause from the environment: with cmd[0] replaced by the shutil.which path and nothing else changed, both entry points stop reporting cannot run 'npm' and start reporting npm's own failure (the stub tree has no package.json) — i.e. the process actually starts. So the shape of the argv, not the machine, is the variable.

Defect 2 — text=True without encoding= decodes with the locale, and node's summary is not decodable in cp936

_run passes text=True and no encoding, so the pipes are decoded with locale.getpreferredencoding(False)cp936 on this host. node's summary starts with the (U+2139) you cite in your NODE_TESTS comment; those bytes are valid UTF-8 and invalid cp936, and the failure happens in subprocess's reader thread, where it is logged rather than raised:

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.

@pm25coder

Copy link
Copy Markdown
Collaborator

Follow-up: checked against master's ASCII guard, and the precedent it sets

Two things I looked at after posting the above.

Your new script does not turn the matrix red. tests/test_script_output_ascii.py (landed on master via #1121) scans scripts/*.py, and it applies to a file added by this PR. Run against scripts/check-node-test-count.py at c496c24, all three of its rules stay green:

  • printed literals (_printed_literals) - every one ASCII;
  • argparse literals (_help_text_literals) - description=/epilog= come from __doc__, and the docstring is ASCII, as are both help= strings;
  • behavioural (test_script_help_survives_a_non_utf8_stdout) - --help under PYTHONIOENCODING of ascii, gbk and cp936: rc=0, 3322 bytes, stdout decodes as ASCII, no traceback in stderr.

The only non-ASCII in the file is on line 91:

# ⚠️ Not `^(\W*)`: node prefixes its summary with `ℹ` (U+2139) INFORMATION

That 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 scripts/*.py is exactly what that guard picks up automatically.

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 case above is invisible everywhere except on a host whose locale cannot decode it. encoding="utf-8" on that one subprocess.run is the same fix in the same spirit as #1121, for the same reason: a host-side check must survive the host's real console and code page, not the code page it was developed on.

No verdict from me - the merge decision is the Committer's.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260910-230247 (verified at head c496c24)

Re-verified this cycle, both states:

  • Negative: doctored Agent.md's GUI count 100 -> 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 prints OK: 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 ✅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants