test: derive the suite totals and the Valgrind list instead of restating them

[Kartikey1306]

Two review findings arrived one after another, on different PRs, with the same shape underneath: a number or a list that describes what the tests do, written out separately from the thing it describes, with nothing checking the two agree. Fixing them one instance at a time was going to keep producing the same finding, so this fixes the class and adds the guard.

Stacked on #94, which repairs master — without it nothing here builds.

1. Suites stated a total instead of counting one

TEST() incremented tests_passed; main() assigned tests_run = <literal>. A test defined but never wired into main() was skipped with a zero exit, and the suite still reported N/N passed for an N that was a claim rather than a count.

18 of 21 suites carried it, and it was not theoretical:

suite	what the hardcoded total was hiding
test_ed25519.c	a hardcoded 11 masking one test called twice and two never called at all — repaired in #94, which is where this started
test_tlv_auth.c	assigns tests_run twice in one function — 8, then 7. The stale 8 survives only because the later assignment wins; it reports 7/7 today by luck
test_secure_boot.c	counted nothing and ended return 0, so a suite that ran none of its cases still reported success. The ASSERT macro exits on failure, so that return could only ever have signalled the one case it ignored

TEST() now increments tests_run where it invokes the test; every literal assignment and every %d/<literal> in a summary is gone.

2. The Valgrind list named its suites again by hand

It had drifted to 17 of 21 — test_ecc, test_rollback, test_secure_boot and test_storage got no memory-safety run, and nothing failed when a name was forgotten, because a missing entry is simply a test that never gets one.

Each add_test() now appends to EBLDR_UNIT_TESTS and the foreach iterates that, so a suite added without touching the block still gets a Valgrind target.

3. A guard so neither returns

tests/unit/test_suite_bookkeeping.py — four checks, no C toolchain needed:

• no suite hardcodes its own total
• every TEST() macro counts the test it runs
• every defined test is actually called
• the Valgrind list is derived rather than repeated

Three suites with no TEST() macro sit in NO_TEST_MACRO with a reason each, following the NOT_BUILT convention already used in this repo.

Verification

check	result
ctest	21/21 PASS
ctest under -DEBLDR_SANITIZE=ON (ASan+UBSan)	21/21 PASS
pytest tests/	42 passed
valgrind targets, with a stub valgrind on PATH	21 (was 17)

Every suite's reported total now equals what it ran — test_bootctl 12/12, test_tlv_auth 7/7, test_secure_boot 4/4.

Each guard was probed against the defect it exists for, and all four pass again on restore:

reintroduce `tests_run = 12`         ->  test_bootctl.c: tests_run = 12
remove `tests_run++` from the macro  ->  FAIL
stop calling a defined TEST()        ->  FAIL
drop a suite from EBLDR_UNIT_TESTS   ->  FAIL

What this does not do

The NO_TEST_MACRO list is itself hand-maintained — the same shape one level up. It is three entries with reasons rather than a silent omission, which is the best available without imposing one harness on suites that legitimately differ. Worth revisiting if it grows.

Refs #94, #80, #82

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 80.39216% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tests/unit/test_suite_bookkeeping.py	80.39%	5 Missing and 5 partials ⚠️

📢 Thoughts on this report? Let us know!

[srpatcha]

Review — eBoot#95 "test: derive the suite totals and the Valgrind list instead of restating them"

head: 93632f3 author: Kartikey1306 ci: pass

Verdict: Right fix, right shape — both hand-maintained lists are replaced by derived ones and a guard is added so they cannot come back. Every verification claim in the body that I could run reproduced exactly. The one substantive gap is that the guard's own escape hatch, NO_TEST_MACRO, reintroduces the pattern the PR exists to remove: I mutated two suites past it and all four checks still passed.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	tests/unit/test_suite_bookkeeping.py:36-40,74	NO_TEST_MACRO is an unvalidated allowlist, and test_every_test_macro_counts_the_test_it_runs continues on it before looking at the file. Two mutations against this head, both silently green: (a) appended #define TEST(name) static void name(void) — a macro that counts nothing — to test_ecc.c, which is on the list; (b) added "test_deleted_long_ago.c" to the dict, naming a file that does not exist. The body's "What this does not do" calls the list hand-maintained but treats it as acceptable; the difference from the two defects this PR fixes is only that this list is three entries long today.	Two assertions, no restructuring: for each key, assert the file exists under tests/unit/, and assert it still contains no #define TEST(. The exemption then expires by itself the moment the suite grows a harness, and a rename fails loudly instead of widening the hole.
2	Low	tests/unit/test_suite_bookkeeping.py:44	_suites() globs tests/unit/test_*.c only. tests/test_performance_benchmarks.c and tests/test_emulation_simulation.c sit outside it. Harmless today — I checked both: neither has a TEST() macro or a tests_run assignment, and neither is referenced by any CMakeLists.txt, .cmake or workflow, so nothing compiles them. Worth recording because "no suite hardcodes its total" reads as a statement about the repo and is a statement about one directory.	Either widen the glob to tests/**/test_*.c, or say "unit suites" in the docstring and the assertion message. The two orphaned files are a separate matter and not this PR's to fix.
3	Low	tests/unit/test_suite_bookkeeping.py:60	r"tests_run\s*=\s*([1-9]\d*)\s*;" catches only a non-zero decimal literal. tests_run = N_TESTS;, tests_run = 0x0C; and tests_run += 4; all pass. The %d/<literal> companion check has the same shape.	Low priority; if it is worth closing, match `tests_run\s*(?:=
4	Low	tests/CMakeLists.txt:7-10	set(EBLDR_UNIT_TESTS "") lands between target_link_libraries(eboot_test_bootctl ...) and add_test(NAME test_bootctl ...), i.e. inside the first suite's block and under its # --- test_bootctl comment. It works, and I confirmed the leading empty string produces no empty list element, but it reads as if it belongs to test_bootctl.	Move it above the # --- test_bootctl comment with the explanatory comment that is already attached to it.
5	Low	—	The duplicate-call defect named in the body ("one test called twice") is not covered by any guard: test_every_defined_test_is_actually_called checks defined - called only. I verified this by duplicating run_test_init_defaults(); in test_bootctl.c — all four checks passed. This is now harmless, because a derived count reports 13/13 honestly instead of 12/12 falsely, so it is a note rather than a defect.	Nothing required. If it is ever worth catching, it is collections.Counter(called) and one assertion.

Cross-PR note, not a finding

eBoot#82 adds test_fdt_loader test_fw_decrypt by hand to the exact foreach this PR deletes — and those two names have no matching targets, so on any host with valgrind installed #82 fails at CMake generate time (I reported that on #82 with a reproduction). If #95 lands first, #82's hunk conflicts and the broken lines disappear in the rebase; if #82 lands first, #95 removes them. Either order is safe, but #95-then-#82 is the one that does not leave a red weekly.yml Valgrind Memcheck job in between. #94 has to precede both, as the body says.

Architecture conformance

Conforms. Test-harness bookkeeping only; no runtime code, no headers, no dependency edges. Master design §5.1 is untouched — nothing here is linked into an image, and tests/ is the layer .ai/architect.md places it in for both reference layouts. Tier placement is correct: eBoot is Tier 1 Foundation (§21) and these are its own suites.

It also serves §28's evidence policy directly. A suite reporting N/N for an N it asserted rather than counted is a "Validated" claim resting on nothing, which is exactly what §28 forbids; deriving the count makes the number evidence again.

Proposed changes

1. tests/unit/test_suite_bookkeeping.py — close finding 1:

   def test_the_no_test_macro_exemptions_are_still_true():
       """An exemption that outlives its reason is the same drifting list one level up."""
       stale, outgrown = [], []
       for name in sorted(NO_TEST_MACRO):
           path = UNIT / name
           if not path.exists():
               stale.append(name)
           elif "#define TEST(" in path.read_text(encoding="utf-8"):
               outgrown.append(name)
       assert not stale, f"NO_TEST_MACRO names files that do not exist: {stale}"
       assert not outgrown, (
           f"these suites now have a TEST() macro and must be checked like the "
           f"rest -- drop them from NO_TEST_MACRO: {outgrown}"
       )

   Probe it the way the body probed the other four: it must fail on a bogus key and on a #define TEST( appended to test_ecc.c, and pass again on restore.
2. tests/CMakeLists.txt — move the set(EBLDR_UNIT_TESTS "") two lines up, above # --- test_bootctl.
3. Findings 2 and 3 are judgement calls; a one-word docstring change covers 2 and 3 can be left.

Nothing here blocks merge. Finding 1 is worth doing in this PR rather than later, because the PR's argument is that fixing instances one at a time keeps reproducing the finding — and the allowlist is the next instance.

Not checked

• Not verified: pytest tests/ → "42 passed". pytest is not installed on this host, so I ran the four guards by importing the module and calling them directly. They pass; the other Python suites in tests/ were not run by me. CI's "Host Build & Tests" is green, which is evidence for the claim but not the same evidence.
• Not verified: the four "probed against the defect it exists for" restorations in the body. I ran my own three mutations (findings 1 and 5) and they behaved as I describe; I did not reproduce the author's four.
• Not verified: the claim that the Valgrind list "had drifted to 17 of 21". I confirmed origin/master lists 17 names and this head produces 21 targets, which is consistent, but I did not check each of the four named suites individually.
• Not checked: whether test_boot_log.c, test_ecc.c and test_image_abi.c are correctly exempt on the merits — only that all three genuinely have no TEST() macro today, which is what the list claims.
• Not run: valgrind itself. It is not installed here; I configured with -DVALGRIND=/usr/bin/true to exercise the block, so the targets are proven to exist and not to pass.

Evidence

Against this head, from a read-only git archive extraction — no repository was modified:

cmake -B build      -DCMAKE_BUILD_TYPE=Debug -DEBLDR_BUILD_TESTS=ON        -> OK
cmake --build build -j4                                                    -> OK
ctest --no-tests=error                                                     -> 21/21 passed
cmake -B build-san  ... -DEBLDR_SANITIZE=ON  && ctest --no-tests=error     -> 21/21 passed (29.1s)
cmake -B build-vg   ... -DVALGRIND=/usr/bin/true                           -> OK
  ctest -N | grep -c valgrind_                                             -> 21   (master: 17)
  first entries: valgrind_test_bootctl, valgrind_test_crypto, valgrind_test_image_verify
  no empty-named target -- set(EBLDR_UNIT_TESTS "") does not leave a leading element
test_suite_bookkeeping.py, all four checks called directly                 -> 4/4 PASS

So ctest 21/21, sanitizer 21/21 and "21 valgrind targets, was 17" all reproduce exactly as claimed. Mutations for findings 1 and 5:

append a non-counting `#define TEST(name)` to test_ecc.c (in NO_TEST_MACRO)  -> 4/4 PASS  (should fail)
add "test_deleted_long_ago.c" to NO_TEST_MACRO                              -> 4/4 PASS  (should fail)
duplicate `run_test_init_defaults();` in test_bootctl.c                     -> 4/4 PASS  (harmless now)

---

Automated architecture review of 93632f33b919 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.