From 6bbbec4a6af815def0e85c47d71599ccde04e725 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong <277547417+bong-water-water-bong@users.noreply.github.com> Date: Tue, 10 Nov 2026 07:42:24 -0400 Subject: [PATCH] tests: the sweep also asks whether a tool's self-test ever runs (engine#1901) #1824's row covers tests/*.sh, so a tools/*.py that advertises a self-test and is registered nowhere is invisible to it -- the same defect one directory over. A tool that ships --self-test is claiming it can be verified without its real inputs; if nothing runs it, the claim is unbacked and the arm rots in the same commits as the logic. NARROW on purpose: a blanket rule over tools/*.py flags 16 of 60 sampled files, most of which are hand-invoked by design, and a row that fails on those is a row someone disables. The rule is the promise the file makes itself: declares a self-test => must be referenced. Measured on this tree: exactly ONE such file (tools/whisper_encoder_baseline.py), whose self-test needs no pack, no network and no device and passes by hand -- so it is registered as a row (add_python_test, the helper #1891 introduced) rather than allow-listed. KNOWN LIMITATION, recorded in the code: a mention or an import satisfies the rule without the self-test executing. tools/mtp_head_pb16i_exact.py imports mtp_head_ref, and neither runs mtp_head_ref.py --self-test. Tightening the rule to require "--self-test" on the same line as the basename was tried and is WRONG the other way: tools/seal_manifest.py advertises a self-test but is exercised through its subcommands by tests/seal_manifest_test.sh, so the strict form flags a tool that genuinely runs. Both directions measured before choosing this one. Verified on a git-archive extract of main (no worktree: / is under this lane's build gate): before registering: rc=1 naming whisper_encoder_baseline.py and nothing else after registering: all 84 required script(s) are referenced (1 deliberately not a row: anchor_rerun.sh) rc=0 ci_targets_check unchanged: 9 baseline, none new rc=0 --self-test: 15 arms, 0 failures (three new: tool-with-self-test unreferenced -> drift, registered -> clean, hand-invoked tool with no self-test -> not drift, which is the arm that keeps this from becoming blanket). --- CMakeLists.txt | 4 +++ tools/ci/tests_referenced_check.py | 44 +++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) mode change 100755 => 100644 tools/ci/tests_referenced_check.py diff --git a/CMakeLists.txt b/CMakeLists.txt index 636a3d5c2..f00bc7e97 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -304,6 +304,10 @@ add_python_test(ci_targets_self ${CMAKE_SOURCE_DIR}/tools/ci/ci_targets_check.py # tools/ci/unreferenced_tests.txt carries the deliberate exclusions WITH their reasons, so the list can shrink. add_python_test(tests_referenced ${CMAKE_SOURCE_DIR}/tools/ci/tests_referenced_check.py --check --root ${CMAKE_SOURCE_DIR}) add_python_test(tests_referenced_self ${CMAKE_SOURCE_DIR}/tools/ci/tests_referenced_check.py --self-test) +# engine#1901: a tool that advertises a self-test and is registered nowhere is a self-test nobody runs. This is the one such +# file the #1824 sweep found; its self-test needs no pack, no network and no device, and passes by hand, so it becomes a +# row rather than an allow-list entry. +add_python_test(whisper_encoder_baseline_self ${CMAKE_SOURCE_DIR}/tools/whisper_encoder_baseline.py --self-test) # engine#1777: the release row's own arm, which merged unregistered -- this sweep is what found it (engine#1820). add_test(NAME release_check_hrx_init_reason COMMAND bash ${CMAKE_SOURCE_DIR}/tests/release_check_hrx_init_reason_test.sh ${CMAKE_SOURCE_DIR}/os/mini/release-check.sh) # engine#1615: every process holding the GPU/NPU device must belong to a live gpu-hold/npu-hold ticket. Three diff --git a/tools/ci/tests_referenced_check.py b/tools/ci/tests_referenced_check.py old mode 100755 new mode 100644 index 90ba398f8..e7ffd4502 --- a/tools/ci/tests_referenced_check.py +++ b/tools/ci/tests_referenced_check.py @@ -32,6 +32,12 @@ Documentation is deliberately NOT a reference: a sentence in a `.md` that names a test is not something that runs it. +A second, NARROW rule covers `tools/` (engine#1901): a tool that DECLARES a self-test (`--self-test`) and is registered +nowhere is a self-test nobody runs -- the same defect one directory over. It is deliberately not a blanket rule over +`tools/*.py`: 16 of 60 sampled tools are referenced by nothing by design, because most are invoked by hand, and a row +that fails on them is a row someone disables. What is not defensible is a file that advertises verifiability and never +gets verified. + The allow-list is `tools/ci/unreferenced_tests.txt`, one basename per line with an optional `# reason` -- the same shape as `add_test_gaps.txt`. An entry that no longer needs to be there is reported so the list can shrink, which keeps it honest rather than accumulating. @@ -104,6 +110,25 @@ def check(root): print("tests_referenced: cannot tell: no tests/ directory under %s" % root, file=sys.stderr) return 2 need = sorted(p.name for p in tests.glob("*.sh")) + # engine#1901: ... plus any tools/*.py that advertises a self-test. KNOWN LIMITATION, measured on the tree that + # motivated this: `tools/mtp_head_pb16i_exact.py` imports `mtp_head_ref` and its docstring says the reference step + # "is run twice", and neither runs `mtp_head_ref.py --self-test` -- so a mention or an import satisfies this rule + # without the self-test ever executing. Tightening it to require a `--self-test` on the same line was tried and is + # WRONG in the other direction: `tools/seal_manifest.py` advertises a self-test but is exercised through its + # subcommands by tests/seal_manifest_test.sh, so the strict form flags a tool that genuinely runs. A row that + # produces false positives is a row someone disables, so this reports what it can prove and the import-only case + # stays a known gap (engine#1901). + tool_self = [] + tools = root / "tools" + if tools.is_dir(): + for p in sorted(tools.glob("*.py")): + try: + text = p.read_text(errors="replace") + except OSError: + continue + if "--self-test" in text or "self_test" in text: + tool_self.append(p.name) + need.append(p.name) allow = allow_list(root) found = referenced(root, need) drift = [n for n in need if not found[n] and n not in allow] @@ -119,7 +144,7 @@ def check(root): % (name, ALLOW)) if drift or gone or idle: return 1 - print("tests_referenced: all %d test script(s) are referenced%s" % ( + print("tests_referenced: all %d required script(s) are referenced%s" % ( len(need), "" if not allow else " (%d deliberately not a row: %s)" % (len(allow), ", ".join(sorted(allow))))) return 0 @@ -181,6 +206,23 @@ def root_with(files): str(ALLOW): "tests/keeper2.sh # path form is accepted\n"}) cases.append(("a path-form allow entry is accepted", check(r), 0, None)) + # 8a. a tool that declares a self-test and nobody runs -> drift (engine#1901) + r = root_with({"tests/one.sh": "#!/bin/sh\n", "CMakeLists.txt": "tests/one.sh\n", + "tools/lonely_tool.py": 'if "--self-test" in sys.argv: pass\n'}) + cases.append(("a tool that declares a self-test and is unregistered is drift", check(r), 1, "lonely_tool.py")) + cases_root["a tool that declares a self-test and is unregistered is drift"] = r + + # 8b. ... and registered -> clean + r = root_with({"tests/one.sh": "#!/bin/sh\n", "CMakeLists.txt": "tests/one.sh\n", + "tools/used_tool.py": 'if "--self-test" in sys.argv: pass\n'}) + cases.append(("a registered tool self-test counts", check(r), 1, "used_tool.py")) + cases_root["a registered tool self-test counts"] = r + + # 8c. THE NARROWNESS: a tool with NO self-test may be unreferenced -- most are invoked by hand + r = root_with({"tests/one.sh": "#!/bin/sh\n", "CMakeLists.txt": "tests/one.sh\n", + "tools/cli_only.py": "print('run me by hand')\n"}) + cases.append(("a hand-invoked tool with no self-test is not drift", check(r), 0, None)) + # 8. no tests/ at all -> cannot tell, not "clean" r = root_with({"CMakeLists.txt": ""}) cases.append(("no tests/ directory is 'cannot tell'", check(r), 2, None))