Tom fiddles with the test suite - again

.github/workflows/test_native.yml

@@ -195,6 +195,13 @@ jobs:
         timeout-minutes: 5
         run: ./bin/test-config-check.sh .pio/build/coverage/meshtasticd
 
+      - name: Shared-state checker self-test
+        # Fixtures that write nothing / exactly what they declare / something undeclared /
+        # a declared write they never make, asserting CLEAN / CLEAN / DIRTY / MISSING. A
+        # checker that has silently stopped matching looks identical to a clean codebase.
+        timeout-minutes: 5
+        run: ./bin/test-state-check.sh
+
       - name: Integration test
         # Cap the whole step: if the simulator ever fails to exit (e.g. the
         # exit_simulator admin path regresses again) the job must fail fast,
@@ -273,9 +280,12 @@ jobs:
           restore-keys: |
             pio-coverage-tests-
 
-      - name: Build test programs once
-        # One shared build of src + every test program. This is the single source build; gcov then
-        # accumulates coverage counts into this shared .pio/build/coverage/src as the chunks run.
+      - name: Warm the shared test build
+        # Compiles src + every test program once so no single area absorbs the whole src build in
+        # its reported duration; gcov then accumulates counts into this shared
+        # .pio/build/coverage/src as the areas run. NOT a substitute for building in the run step:
+        # PlatformIO links every test program to the one .pio/build/coverage/meshtasticd path, so a
+        # --without-building run executes whichever suite was linked last under every suite's name.
         run: platformio test -e coverage --without-testing
 
       - name: Save PlatformIO cache
@@ -368,12 +378,21 @@ jobs:
             echo "::group::area $a (${group[$a]# })"
             # Capture platformio's real exit status (not grep's) via a log file, then show the log
             # with the noisy per-variant SKIPPED rows filtered out.
-            if ! platformio test -e coverage --without-building -v ${group[$a]# } \
+            if ! platformio test -e coverage -v ${group[$a]# } \
                  --junit-output-path "testreport-$a.xml" > "area-$a.log" 2>&1; then
               fail=1
               echo "::error::area $a had test failures"
             fi
+            # Suites outside this area are reported SKIPPED by design (PlatformIO lists every suite
+            # in the env and marks the unselected ones finished), so those rows are noise here. The
+            # attribution check below is what catches a suite that was selected and did not run.
             grep -v "[[:space:]]SKIPPED$" "area-$a.log" || true
+            # Per area, so a mismatch names the area it happened in rather than the whole run.
+            if ! ./bin/check-test-attribution.py --label "area $a" \
+                 --expect "${group[$a]# }" "testreport-$a.xml"; then
+              fail=1
+              echo "::error::area $a ran suites that did not match their own test binaries"
+            fi
             echo "::endgroup::"
           done
           exit $fail
@@ -398,16 +417,50 @@ jobs:
           ET.ElementTree(out).write('testreport.xml', encoding='utf-8', xml_declaration=True)
           PY
 
+      - name: Verify every suite ran its own tests
+        # Whole-run gate over the merged report: every test_* directory must appear with at least
+        # one test case, and every case must come from the suite that reported it. The per-area
+        # check above cannot see an area that never executed - this can.
+        if: always() # a suite going missing is the finding; do not hide it behind an earlier failure
+        shell: bash
+        run: |
+          set -euo pipefail
+          mapfile -t suites < <(find test -maxdepth 1 -type d -name 'test_*' -printf '%f\n' | sort)
+          ./bin/check-test-attribution.py --label "coverage (all areas)" \
+            --expect "${suites[*]}" testreport.xml
+
       - name: Capture coverage information
         if: always() # run this step even if previous step failed
         run: |
           sudo apt-get install -y lcov
           lcov ${{ env.LCOV_CAPTURE_FLAGS }} --test-name tests --output-file coverage_tests.info
           sed -i -e "s#${PWD}#.#" coverage_tests.info # Make paths relative.
 
+      - name: Attribution canary
+        # Guards the guard above: runs two suites the broken way (--without-building, so PlatformIO
+        # does not relink and both execute the same leftover binary) and requires the checker to
+        # catch it. Fails if the checker regressed, or if the reproduction stops reproducing - in
+        # which case the reason both harnesses stopped passing that flag no longer holds.
+        #
+        # Lives in this job, not simulator-tests: it relinks $BUILD_DIR/$PROGNAME, and there that
+        # replaced the daemon binary with a test suite, so the integration test waited for a socket
+        # a test binary never opens. Here the binary is already per-suite and nothing later needs it.
+        timeout-minutes: 15
+        run: ./bin/test-attribution-canary.sh -e coverage
+
       - name: Event channel policy tests
         run: platformio test -e coverage-event-policy -v --junit-output-path event-policy-testreport.xml
 
+      - name: Verify the event-policy suites ran their own tests
+        # Expected set read through PlatformIO's own config parser, so it cannot drift from the
+        # env's test_filter the way a second hand-maintained list would.
+        run: |
+          set -euo pipefail
+          expect=$(python3 -c "from platformio.project.config import ProjectConfig; \
+            print(' '.join(ProjectConfig().get('env:coverage-event-policy', 'test_filter', [])))")
+          ./bin/check-test-attribution.py --label coverage-event-policy \
+            --expect "$expect" event-policy-testreport.xml
+
       - name: Save test results
         if: always() # run this step even if previous step failed
         uses: actions/upload-artifact@v7

bin/check-test-attribution.py

@@ -0,0 +1,165 @@
+#!/usr/bin/env python3
+"""Verify each PlatformIO JUnit report ran the suite it claims to have run.
+
+PlatformIO links every native test program to one path ($BUILD_DIR/$PROGNAME) and parses
+Unity output textually, without checking that the reported source file belongs to the suite
+it is running. Split a run into `--without-testing` then `--without-building` and every suite
+executes whichever binary was linked last, all reporting PASSED. This reads the JUnit reports
+that run already produces and fails on the two shapes that hides:
+
+  MISATTRIBUTED - a test case whose source file lives outside the suite that reported it
+  EMPTY         - a suite that was asked to run and produced no test cases at all
+
+Usage:
+  check-test-attribution.py [--expect "s1 s2"]... [--label TEXT] REPORT.xml...
+
+--expect names the suites the run was asked for (repeatable, whitespace- or `-f`-separated,
+so a CI area string can be passed through verbatim). Omit it to check attribution only.
+Exit: 0 clean, 1 findings, 2 bad usage / unreadable report.
+"""
+
+import argparse
+import glob
+import sys
+import xml.etree.ElementTree as ET
+
+
+def parse_expect(values):
+    """Flatten repeated --expect values into a suite list, tolerating `-f suite` tokens."""
+    suites = []
+    for value in values or []:
+        for token in value.split():
+            if token == "-f":
+                continue
+            suites.append(token.removeprefix("-f"))
+    return [s for s in suites if s]
+
+
+def suite_of(testsuite_name):
+    """`coverage:test_foo` -> `test_foo`; a bare name is returned unchanged."""
+    return testsuite_name.split(":", 1)[1] if ":" in testsuite_name else testsuite_name
+
+
+def owns(suite, source_file):
+    """Report whether source_file sits inside the suite's own directory.
+
+    Matched on a whole path segment so `test_mesh` does not claim `test_mesh_module`, and
+    with a leading separator so absolute and relative paths behave the same.
+    """
+    normalized = "/" + source_file.replace("\\", "/").lstrip("/")
+    return f"/{suite}/" in normalized
+
+
+def collect(paths):
+    """Map suite -> list of (case name, source file or None), merged across reports."""
+    cases = {}
+    for path in paths:
+        try:
+            # The input is the JUnit report PlatformIO just wrote in this same run, not untrusted
+            # data, and defusedxml is not installed for this job.
+            # nosemgrep: python.lang.security.use-defused-xml-parse.use-defused-xml-parse
+            root = ET.parse(path).getroot()
+        except (ET.ParseError, OSError) as exc:
+            sys.stderr.write(f"check-test-attribution: cannot read {path}: {exc}\n")
+            sys.exit(2)
+        # PlatformIO nests <testsuite> under <testsuites>; accept a bare <testsuite> too.
+        nodes = [root] if root.tag == "testsuite" else root.iter("testsuite")
+        for node in nodes:
+            suite = suite_of(node.get("name", ""))
+            if not suite:
+                continue
+            entries = cases.setdefault(suite, [])
+            for case in node.iter("testcase"):
+                entries.append((case.get("name", "?"), case.get("file")))
+    return cases
+
+
+def main():
+    parser = argparse.ArgumentParser(add_help=True)
+    parser.add_argument("--expect", action="append", default=[])
+    parser.add_argument("--label", default="")
+    parser.add_argument("reports", nargs="+")
+    args = parser.parse_args()
+
+    # Expand globs ourselves: CI passes a pattern that may match nothing if a step was skipped,
+    # and a silent pass over zero reports is exactly the false green this script exists to stop.
+    paths = sorted({p for pattern in args.reports for p in glob.glob(pattern)})
+    if not paths:
+        sys.stderr.write(
+            "check-test-attribution: no JUnit reports matched %s\n"
+            % " ".join(args.reports)
+        )
+        return 2
+
+    cases = collect(paths)
+    expected = parse_expect(args.expect)
+
+    misattributed = []  # (suite, case name, source file)
+    unsourced = []  # (suite, case name)
+    for suite, entries in sorted(cases.items()):
+        for name, source in entries:
+            if source is None:
+                unsourced.append((suite, name))
+            elif not owns(suite, source):
+                misattributed.append((suite, name, source))
+
+    empty = [s for s in expected if not cases.get(s)]
+
+    label = f" [{args.label}]" if args.label else ""
+    total = sum(len(v) for v in cases.values())
+    print(
+        f"test attribution{label}: {len(paths)} report(s), "
+        f"{len([s for s, v in cases.items() if v])} suite(s) with cases, {total} case(s)"
+    )
+    if unsourced:
+        print("")
+        print("UNSOURCED - these cases carry no source file, so ownership cannot be proved:")
+        for suite, name in unsourced[:20]:
+            print(f"    {suite}: case '{name}'")
+        if len(unsourced) > 20:
+            print(f"    ... +{len(unsourced) - 20} more")
+        print(
+            "A report without file attributes is not evidence that the suites ran their own"
+        )
+        print(
+            "tests. Treat it as a finding rather than a pass: the JUnit format has changed, or"
+        )
+        print("the runner emitted cases it could not attribute.")
+
+    if misattributed:
+        print("")
+        print(
+            "MISATTRIBUTED - these suites reported test cases belonging to another suite."
+        )
+        print(
+            "The run executed one suite's binary under another suite's name; the named"
+        )
+        print(
+            "suites did NOT run. Check for --without-building in the test invocation."
+        )
+        for suite, name, source in misattributed[:20]:
+            print(f"    {suite}: case '{name}' came from {source}")
+        if len(misattributed) > 20:
+            print(f"    ... +{len(misattributed) - 20} more")
+
+    if empty:
+        print("")
+        print("EMPTY - these suites were asked to run and produced no test cases:")
+        for suite in empty:
+            print(f"    {suite}")
+
+    if misattributed or empty or unsourced:
+        print("")
+        print(
+            "RESULT: test attribution FAILED"
+            f"{label} ({len(misattributed)} misattributed, {len(empty)} empty,"
+            f" {len(unsourced)} unsourced)"
+        )
+        return 1
+
+    print(f"RESULT: test attribution OK{label}")
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())

bin/lib/test-state.sh

@@ -167,3 +167,53 @@ state_classify() {
 		printf 'CLEAN\t\n'
 	fi
 }
+
+# --- Error-line budget -------------------------------------------------------------------------
+#
+# A second orthogonal axis, like CLEAN/DIRTY above: a suite can pass while emitting six figures of
+# LOG_ERROR, which buries a real failure and trains everyone to skim. The budget is declared in the
+# same manifest, as an `errors=` flag, and it is a RANGE rather than a ceiling - for a fuzz suite the
+# floor is the load-bearing half. test_fuzz_decode logging ~100k rejections is it working; the same
+# suite logging none means it stopped feeding malformed input, and every case would still pass.
+#
+# Undeclared suites get ERROR_BUDGET_DEFAULT. Declared forms: "N" (max), "MIN..MAX", "MIN.." (floor
+# only). Everything is inclusive.
+ERROR_BUDGET_DEFAULT=100
+
+# Count LOG_ERROR lines in a suite's captured output.
+state_count_errors() {
+	local log="$1"
+	[[ -f $log ]] || {
+		printf '0'
+		return 0
+	}
+	# `|| true`, not `|| printf 0`: grep -c already prints 0 before exiting 1 on no match, so a
+	# fallback that prints appends a second line and the caller gets "0\n0" to do arithmetic on.
+	grep -cE '^ERROR +\|' "$log" 2>/dev/null || true
+}
+
+# VERDICT<TAB>DETAIL. WITHIN / OVER / UNDER, mirroring state_classify()'s shape.
+state_classify_errors() {
+	local count="$1" declared="$2" min=0 max="$ERROR_BUDGET_DEFAULT"
+
+	if [[ -n $declared ]]; then
+		if [[ $declared == *".."* ]]; then
+			min="${declared%%..*}"
+			max="${declared##*..}"
+			[[ -z $max ]] && max=""
+		else
+			max="$declared"
+		fi
+	fi
+
+	if [[ -n $max ]] && ((count > max)); then
+		printf 'OVER\t%d error line(s), budget %s' "$count" "${declared:-$ERROR_BUDGET_DEFAULT}"
+		return 0
+	fi
+	if ((count < min)); then
+		printf 'UNDER\t%d error line(s), expected at least %d - is it still exercising the path?' \
+			"$count" "$min"
+		return 0
+	fi
+	printf 'WITHIN\t%d' "$count"
+}

bin/pio-test-isolate.sh

@@ -92,6 +92,11 @@ GRANULARITY="$(state_flag_value state "$FLAGS")"
 
 IFS=$'\t' read -r VERDICT DETAIL <<<"$(state_classify "$CHANGED" "$DECLARED")"
 
+# Error-line budget: same manifest, same declare-and-justify shape as the writes above. Counted from
+# the captured log, so it costs nothing extra.
+ERROR_COUNT="$(state_count_errors "$LOG")"
+IFS=$'\t' read -r ERROR_VERDICT ERROR_DETAIL <<<"$(state_classify_errors "$ERROR_COUNT" "$(state_flag_value errors "$FLAGS")")"
+
 # Per-test attribution, when the suite has not declared that it carries state across its own test
 # cases. For a state=per-suite suite every test after the first would be flagged by design - that
 # carry *is* the declared behaviour - so only the suite boundary is meaningful there.
@@ -114,14 +119,14 @@ fi
 
 STATUS=$([[ $RC -eq 0 ]] && echo PASS || echo FAIL)
 mkdir -p "$(dirname "$SUMMARY")" 2>/dev/null
-printf '%s\t%s\t%s\t%s\t%s\t%s\n' "$SUITE" "$STATUS" "$VERDICT" "${DETAIL-}" "${PER_TEST_DETAIL-}" \
-	"${SURVIVORS-}" >>"$SUMMARY"
+printf '%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n' "$SUITE" "$STATUS" "$VERDICT" "${DETAIL-}" "${PER_TEST_DETAIL-}" \
+	"${SURVIVORS-}" "${ERROR_VERDICT-}" "${ERROR_DETAIL-}" >>"$SUMMARY"
 
 # Keep the sandbox when there is something to look at: on a failure it plus the built binary is a
 # complete, replayable reproduction, and on a DIRTY verdict the leftovers *are* the bug report. A
 # clean pass leaves nothing behind.
 KEEP="${MESHTASTIC_TEST_KEEP_STATE:-0}"
-if [[ $RC -ne 0 || $VERDICT != CLEAN || -n ${SURVIVORS-} || $KEEP == 1 ]]; then
+if [[ $RC -ne 0 || $VERDICT != CLEAN || $ERROR_VERDICT != WITHIN || -n ${SURVIVORS-} || $KEEP == 1 ]]; then
 	DEST="$STATE_ROOT/$SUITE"
 	rm -rf "$DEST" 2>/dev/null
 	mv "$SCRATCH" "$DEST" 2>/dev/null || DEST="$SCRATCH"