From 6905639b07c69fec68e9ebc77f7d27ac2775ee41 Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 20 Aug 2026 16:43:49 +0700 Subject: hil: run every board in one hil_test.py and hand results across as JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hil-validate ran one hil-operator per board. That parallelizes at the wrong layer: hil_test.py already schedules boards across host controllers and budgets concurrent flashes and usbtest batteries per controller (FLASH_PARALLEL/USBTEST_PARALLEL), and those permits live in one process - N parallel runs multiply the budget onto the same uPD720201 cards for no wall-clock gain over one run that already parallelizes. The workflow now spawns ONE operator with every board as repeated -b. The operator no longer retypes the report table. Four consecutive max-effort review rounds found ~15 defects in this file and every one was in reconstructing board identity from transcribed prose: report rows are named per VARIANT (nanoch32v203 only ever produces -fsdev/-usbfs rows), a variant need not start with its board's name, lock contention is a `board-locked` cell rather than a phrase, and each fix introduced the next round's bug - including a fake-green test that asserted an invariant with the one input shape that could not break it. The new helper test/hil/helper/hil_summary.py does the join where the roster lives and emits one machine verdict per board ({board, ran, pass, locked, detail}); the operator returns that JSON verbatim plus `wedged`, the only field it authors, and the workflow reads fields, never parses a string. Its cell classifier mirrors hil_test.py's own tally exactly: failures are always marked ('fail' or a ❌ prefix, TestFail's contract), everything unmarked is a pass - a passing test may return a plain metric cell like '13443 KB/s', and the mirrored rule is what keeps a green table from becoming a red verdict. hil_ci.sh kept only the LAST -b, so multi-board remote runs staged one board's binaries and every other board died on the rig after its lock and flash slot were spent. It now parses every -b spelling argparse accepts (with the -bt arms ordered first, longest-match, so the .failed retry form is never read as a board named "t..."), pre-flights roster membership and build dirs for ALL boards before anything is wiped or staged, warns per declared variant with no build dir (which hil_test.py would silently green-skip), forwards HIL_* knobs as export lines in one %q word the remote evals ('; '-joined so it round-trips under dash - an authorized HIL_NO_BOARD_LOCK force must not silently no-op), keeps HIL_REPORT_DIR local because the copy-backs look in REMOTE_DIR, and copies hil_report.json and the .failed re-run spec back beside the markdown, deleting stale local copies first so a green run cannot leave last run's spec looking current. Retries preserve the fleet: the documented path is the .failed spec, which already begins with --accumulate; a fresh scoped re-run would unlink the report and collapse the whole-fleet table to the retried boards alone. The risky logic is executable, not argued about: .claude/workflows/test-hil-validate.mjs pins the lookup/verdict helpers and runs in pre-commit (hil-validate-logic); nine staging tests drive hil_ci.sh through an ssh stub that models the real thing (argv joined into one string the remote re-splits, heredoc on stdin - the naive echo-stub passed while the feature was broken); and deliberate mutations of the verdict logic are all caught. Validated on the rig: a 2-board run (usbtest 30/30 on both; the pre-fix classifier, replayed against that run's real report, fails the fully-green stm32f723disco on its two passing '13443 KB/s' cells), the .failed --accumulate retry (merged report kept every earlier row), and a 10-run soak over random subsets of a 22-board pool - 43 board-slots, every failure signature matched pre-existing CI state or known flake, zero tooling failures, no locks left behind. --- test/hil/helper/hil_summary.py | 115 ++++++++++++++ test/hil/hil_ci.sh | 250 +++++++++++++++++++++++------- test/hil/test/test_hil_bounded.py | 317 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 626 insertions(+), 56 deletions(-) create mode 100644 test/hil/helper/hil_summary.py (limited to 'test') diff --git a/test/hil/helper/hil_summary.py b/test/hil/helper/hil_summary.py new file mode 100644 index 000000000..e566bead0 --- /dev/null +++ b/test/hil/helper/hil_summary.py @@ -0,0 +1,115 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +"""Fold hil_report.json into one machine-readable verdict per BOARD. + +A workflow driving hil_test.py through an operator agent has no filesystem access, so the +agent has to carry the results across. It must carry them, not retype them: the previous +design asked the agent to transcribe the markdown table, and every defect found in four +review rounds came from re-parsing that prose -- variant row names vs board names, +`board locked` vs `board-locked`, folding several variant rows into one verdict, rows that +matched no board. All of it is a join, and the join belongs here, where the roster is. + +Report rows are named per VARIANT (hil_test.py builds them from `vname`), and a variant name +is not required to start with the board name -- nanoch32v203 produces only `-fsdev`/`-usbfs`, +ch32v307v_r1_1v0 only `-usbhs`/`-usbfs`. The config is what maps them back. + +Emits, on stdout: + {"results": [{"board", "ran", "pass", "locked", "detail"}...], "banner": str} + +`locked` is a field, not a prefix to grep for. `ran` false means the board produced no row at +all, which is not the same as failing. + +Usage: hil_summary.py [-b BOARD]... [--report-dir DIR] +""" +import argparse +import json +import sys +from pathlib import Path + +FAIL_ICON, SKIP_ICON = '❌', '⚪' # a pass needs no icon: unmarked = pass +LOCKED_CELL = 'board-locked' + + +def cell_state(v: str) -> str: + """'pass' | 'fail' | 'skip' -- the EXACT classifier hil_test.py's own tally uses + (cell_kind in render_matrix): 'fail' or a ❌ prefix is a failure, 'skip' or a ⚪ + prefix is a skip, and EVERYTHING ELSE is a pass. That last arm is load-bearing: a + passing test may return a plain metric string ('480.0 MBps') that lands in the cell + unprefixed, while failures are guaranteed marked -- TestFail's docstring pins that its + metric is icon-prefixed precisely so render/tally treat it as a failure. Classifying + unknown shapes as fail here would publish a green table as a red verdict.""" + if v == 'fail' or v.startswith(FAIL_ICON): + return 'fail' + if v == 'skip' or v.startswith(SKIP_ICON): + return 'skip' + return 'pass' + + +def variants_of(cfg: dict, board: str) -> list: + for b in cfg.get('boards', []): + if b['name'] == board: + return [v['name'] for v in (b.get('variant') or [])] or [board] + return [board] + + +def summarize(cfg: dict, boards: list, report: dict) -> dict: + rows = {r['board']: r.get('cells') or {} for r in report.get('rows', [])} + owner = {v['name']: b['name'] for b in cfg.get('boards', []) + for v in (b.get('variant') or [])} + results = [] + for board in boards: + names = variants_of(cfg, board) + mine = {n: rows[n] for n in names if n in rows} + # a variant name that is neither declared nor prefixed cannot be attributed; the + # `-` fallback only helps ad-hoc builds, it is not the primary path. It must + # also never steal a row DECLARED by another board: a declared variant need not start + # with its own board's name, so it may happen to start with this board's name plus '-'. + mine.update({n: c for n, c in rows.items() + if n.startswith(f'{board}-') and n not in mine + and owner.get(n, board) == board}) + if not mine: + results.append({'board': board, 'ran': False, 'pass': False, 'locked': False, + 'detail': 'no report row for this board'}) + continue + locked = any(LOCKED_CELL in cells for cells in mine.values()) + bad = [] + for vname, cells in sorted(mine.items()): + for test, val in sorted(cells.items()): + if test == LOCKED_CELL: + continue + if cell_state(str(val)) == 'fail': + bad.append(f'{vname} {test}: {val}') + ok = not bad and not locked + if locked: + detail = 'held by another holder; not flashed' + elif bad: + detail = '; '.join(bad) + else: + detail = f'{len(mine)} variant(s), {sum(len(c) for c in mine.values())} cell(s) ok' + results.append({'board': board, 'ran': True, 'pass': ok, 'locked': locked, + 'detail': detail}) + return {'results': results, 'banner': report.get('banner', '')} + + +def main() -> int: + ap = argparse.ArgumentParser() + ap.add_argument('config_file') + ap.add_argument('-b', '--board', action='append', default=[], + help='boards to report on; default: every board in the config') + ap.add_argument('--report-dir', default='.', help='where hil_report.json lives (default: cwd)') + a = ap.parse_args() + + cfg = json.loads(Path(a.config_file).read_text()) + boards = a.board or [b['name'] for b in cfg.get('boards', [])] + jpath = Path(a.report_dir) / 'hil_report.json' + if not jpath.is_file(): + print(f'error: {jpath} not found -- did hil_test.py run in this directory?', + file=sys.stderr) + return 1 + json.dump(summarize(cfg, boards, json.loads(jpath.read_text())), sys.stdout, indent=2) + print() + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/test/hil/hil_ci.sh b/test/hil/hil_ci.sh index c7dfa95df..f474c3055 100644 --- a/test/hil/hil_ci.sh +++ b/test/hil/hil_ci.sh @@ -1,8 +1,9 @@ #!/usr/bin/env bash # Run HIL test remotely on ci.lan -# Usage: test/hil/hil_ci.sh [-b BOARD] [-t TEST] [extra hil_test.py args...] +# Usage: test/hil/hil_ci.sh [-b BOARD]... [-t TEST] [extra hil_test.py args...] # Example: # test/hil/hil_ci.sh -b stm32f723disco +# test/hil/hil_ci.sh -b stm32f723disco -b raspberry_pi_pico # test/hil/hil_ci.sh -b stm32f723disco -t host/cdc_msc_hid -r 1 # # Env overrides: REMOTE, REMOTE_DIR, CONFIG (path to HIL config json), @@ -44,17 +45,46 @@ for a in "$@"; do exit 1 done -# Parse -b BOARD from arguments to know which build to copy -BOARD="" +# Parse -b BOARD from arguments to know which builds to copy. Repeatable: hil_test.py +# takes the whole board set in ONE run (it schedules them across host controllers and +# budgets the flashes itself), so every -b needs its binaries staged, not just the last. +BOARDS=() ARGS=() while [[ $# -gt 0 ]]; do case "$1" in - -b) - [[ $# -ge 2 ]] || { echo "error: -b requires a BOARD argument" >&2; exit 1; } - BOARD="$2" + # hil_test.py declares `-b, --board` with action='append', so argparse also accepts + # --board=X and -bX. Recognising only the bare `-b X` forwarded the others to the rig + # while never staging them: the board ran with no firmware and reported a green row. + -b|--board) + [[ $# -ge 2 ]] || { echo "error: $1 requires a BOARD argument" >&2; exit 1; } + BOARDS+=("$2") ARGS+=("$1" "$2") shift 2 ;; + --board=*) + BOARDS+=("${1#--board=}") + ARGS+=("$1") + shift + ;; + # -bt (--board-test) BEFORE the glued -b?* arm, mirroring argparse's longest-match: it is + # the form .failed uses, and a bare -b?* would register a board named "t..." that + # the roster check below rejects -- killing every documented retry. + -bt|--board-test) + [[ $# -ge 2 ]] || { echo "error: $1 requires NAME:tests" >&2; exit 1; } + ARGS+=("$1" "$2") + shift 2 + ;; + -bt?*|--board-test=*) + ARGS+=("$1") + shift + ;; + # glued short form: argparse resolves -bNAME to --board NAME, so staging must too -- + # unparsed it fell through to the all-boards branch and silently staged everything built + -b?*) + BOARDS+=("${1#-b}") + ARGS+=("$1") + shift + ;; *) ARGS+=("$1") shift @@ -62,6 +92,107 @@ while [[ $# -gt 0 ]]; do esac done +# Resolve a board to its build dirs: its own dir, the cmake-build--* glob (ad-hoc +# local builds), and the variant dirs named in $CONFIG -- variant names are NOT required to +# be prefixed with the board name, so the glob alone is not enough. Prints one dir per line. +variant_names() { + python3 -c ' +import json, sys +cfg = json.load(open(sys.argv[1])) +for b in cfg.get("boards", []): + if b["name"] == sys.argv[2]: + for v in b.get("variant") or []: + print(v["name"]) +' "$CONFIG" "$1" +} + +resolve_build_dirs() { + local board="$1" d v + declare -A seen=() + shopt -s nullglob + for d in "$ROOT_DIR"/examples/cmake-build-"$board" "$ROOT_DIR"/examples/cmake-build-"$board"-*; do + [[ -d $d && -z ${seen[$d]:-} ]] && { seen[$d]=1; printf '%s\n' "$d"; } + done + shopt -u nullglob + # to a file, not a process substitution: `set -e`/pipefail cannot see the exit status of + # the latter, so a malformed roster silently yielded zero variant dirs + local vf; vf=$(mktemp) + variant_names "$board" > "$vf" || { rm -f "$vf"; echo "Error: could not read variants for $board from $CONFIG" >&2; exit 1; } + while IFS= read -r v; do + d="$ROOT_DIR/examples/cmake-build-$v" + [[ -d $d && -z ${seen[$d]:-} ]] && { seen[$d]=1; printf '%s\n' "$d"; } + done < "$vf" + rm -f "$vf" +} + +# Pre-flight: EVERY board must resolve to at least one build dir before anything is wiped or +# copied. This check used to live in the copy loop, so an unbuilt board late in the list +# aborted the run after the remote tree had been rm -rf'd and earlier boards fully rsynced -- +# zero coverage, a half-staged rig, and a stale local hil_report.md left in place. Report all +# missing boards at once so one build round fixes them. +MANIFEST=$(mktemp) +trap 'rm -f "$MANIFEST"' EXIT +# Roster membership first: hil_test.py rejects an unknown -b with sys.exit(1) for the WHOLE +# run (hil_test.py:2297), and it does so AFTER this script has wiped REMOTE_DIR and staged +# every board -- one typo then costs the entire batch. We already parse $CONFIG here, so +# catch it before anything is touched. Note -b matches board names only, never variant names. +if [ ${#BOARDS[@]} -gt 0 ]; then +ROSTER=$(python3 -c ' +import json, sys +print("\n".join(b["name"] for b in json.load(open(sys.argv[1])).get("boards", []))) +' "$CONFIG") || { echo "error: could not read the board roster from $CONFIG" >&2; exit 1; } +notinroster=() +for b in ${BOARDS[@]+"${BOARDS[@]}"}; do + grep -qxF -- "$b" <<< "$ROSTER" || notinroster+=("$b") +done +if [ ${#notinroster[@]} -gt 0 ]; then + echo "error: not in $(basename "$CONFIG"): ${notinroster[*]}" >&2 + echo " (-b takes board names, not variant names)" >&2 + exit 1 +fi +fi # BOARDS non-empty: nothing to validate for an all-boards run + +missing=() +for b in ${BOARDS[@]+"${BOARDS[@]}"}; do + dirs=$(resolve_build_dirs "$b") + if [ -z "$dirs" ]; then + missing+=("$b") + else + while IFS= read -r d; do printf '%s\t%s\n' "$b" "$d" >> "$MANIFEST"; done <<< "$dirs" + # A declared variant with no build dir is NOT an error -- no cmake preset is + # variant-suffixed, so this is the normal state for e.g. the -DMA variants. It is worth + # saying out loud: hil_test.py logs `Skip (no binary)` and counts zero errors for it, so + # the run exits 0 and the operator reads a green table for cells that never ran. + while IFS= read -r v; do + [ -z "$v" ] && continue + # whole lines: a substring match lets cmake-build--DMA silence the warning for + grep -qxF -- "$ROOT_DIR/examples/cmake-build-$v" <<< "$dirs" \ + || echo "warning: $b variant '$v' has no build dir -- its cells will be skipped, not tested" >&2 + done < <(variant_names "$b") + fi +done +if [ ${#missing[@]} -gt 0 ]; then + echo "Error: no build directory under $ROOT_DIR/examples/ for: ${missing[*]}" >&2 + for b in "${missing[@]}"; do + echo " cd examples && cmake --preset $b && cmake --build --preset $b" >&2 + done + exit 1 +fi + +# The all-boards form needs its emptiness check HERE too: below the setup ssh it fired after +# the remote tree was already rm -rf'd, destroying the previous run's report and re-run spec +# on the rig before deciding there was nothing to do. +if [ ${#BOARDS[@]} -eq 0 ]; then + shopt -s nullglob + allbuilds=("$ROOT_DIR"/examples/cmake-build-*/) + shopt -u nullglob + if [ ${#allbuilds[@]} -eq 0 ]; then + echo "error: no examples/cmake-build-* directories under $ROOT_DIR -- nothing to test" >&2 + echo " build first, e.g.: cd examples && cmake --preset && cmake --build --preset " >&2 + exit 1 + fi +fi + # Setup remote directory. `bash -s` + heredoc so REMOTE_DIR arrives as a positional # parameter, keeping the `rm -rf` target out of the command string the heredoc runs. echo "==> Setting up remote $REMOTE:$REMOTE_DIR" @@ -89,6 +220,7 @@ scp -q "$ROOT_DIR/test/hil/helper/__init__.py" \ "$ROOT_DIR/test/hil/helper/hil_util.py" \ "$ROOT_DIR/test/hil/helper/hil_health.py" \ "$ROOT_DIR/test/hil/helper/hil_lock.py" \ + "$ROOT_DIR/test/hil/helper/hil_summary.py" \ "$ROOT_DIR/test/hil/helper/hil_select.py" \ "$REMOTE:$REMOTE_DIR/test/hil/helper/" @@ -103,52 +235,23 @@ copy_board_binaries() { "$src" "$REMOTE:$REMOTE_DIR/examples/" } -if [ -n "$BOARD" ]; then - # Copy the board's build dir plus its variant dirs. Variant names come from - # $CONFIG (they are not required to be prefixed with the board name); the - # cmake-build--* glob is kept as a fallback for ad-hoc local builds. - # Collect only dirs that actually exist, deduplicated. - declare -A SEEN_DIRS=() - BUILD_DIRS=() - add_build_dir() { - [[ -d "$1" && -z "${SEEN_DIRS[$1]:-}" ]] || return 0 - SEEN_DIRS[$1]=1 - BUILD_DIRS+=("$1") - } - shopt -s nullglob - for d in "$ROOT_DIR"/examples/cmake-build-"$BOARD" "$ROOT_DIR"/examples/cmake-build-"$BOARD"-*; do - add_build_dir "$d" - done - shopt -u nullglob - # to a file, not a process substitution: `set -e`/pipefail cannot see the exit - # status of the latter, so a malformed roster silently yielded zero variant dirs - VARIANTS_FILE=$(mktemp) - python3 -c ' -import json, sys -cfg = json.load(open(sys.argv[1])) -for b in cfg.get("boards", []): - if b["name"] == sys.argv[2]: - for v in b.get("variant") or []: - print(v["name"]) -' "$CONFIG" "$BOARD" > "$VARIANTS_FILE" || { - echo "Error: could not read variants for $BOARD from $CONFIG" - rm -f "$VARIANTS_FILE" - exit 1 - } - while IFS= read -r v; do - add_build_dir "$ROOT_DIR/examples/cmake-build-$v" - done < "$VARIANTS_FILE" - rm -f "$VARIANTS_FILE" - if [ ${#BUILD_DIRS[@]} -eq 0 ]; then - echo "Error: no build directory found for $BOARD under $ROOT_DIR/examples/" - echo "Build first with: cd examples && cmake --preset $BOARD && cmake --build --preset $BOARD" - exit 1 - fi - echo "==> Copying binaries for $BOARD (${#BUILD_DIRS[@]} build dir(s))" - for d in "${BUILD_DIRS[@]}"; do - copy_board_binaries "$d" +if [ ${#BOARDS[@]} -gt 0 ]; then + # Replay the pre-flight manifest: the dirs were already resolved and proved non-empty + # for every board, so nothing here can abort mid-staging. Plain reads of the manifest -- + # a process substitution would hide a reader failure from set -e (the comment in + # resolve_build_dirs is about exactly that trap). + for b in "${BOARDS[@]}"; do + dirs=() + while IFS=$'\t' read -r bb d; do + [ "$bb" = "$b" ] && [ -n "$d" ] && dirs+=("$d") + done < "$MANIFEST" + echo "==> Copying binaries for $b (${#dirs[@]} build dir(s))" + for d in ${dirs[@]+"${dirs[@]}"}; do + copy_board_binaries "$d" + done done else + # emptiness was already refused in pre-flight, before the remote wipe echo "==> Copying all built binaries" # Use `%/` parameter expansion to strip the trailing slash from the glob — # rsync needs the bare dir name so the per-board cmake-build-/ subdir @@ -170,14 +273,38 @@ for a in ${ARGS[@]+"${ARGS[@]}"}; do ARGS_Q+=("$(printf '%q' "$a")"); done CONFIG_Q="$(printf '%q' "test/hil/$(basename "$CONFIG")")" echo "==> Running HIL test on $REMOTE" rc=0 -# --retry 1 FIRST, before the user's args: this targets the same shared rig CI uses, and -# the pool guard is a flat constant that does not scale with max_retry -- argparse's -# default of 3 lets a few flaky boards re-pay 510s each until the 3600s guard fires, -# abandoning the pool and holding board flocks against concurrent CI. Placed first, not -# appended, so argparse's last-wins means `hil_ci.sh -r 3` still gets 3. -ssh "$REMOTE" bash -s -- "$REMOTE_DIR" --retry 1 ${ARGS_Q[@]+"${ARGS_Q[@]}"} "$CONFIG_Q" <<'REMOTE' || rc=$? +# --retry 1 FIRST, before the user's args: this targets the same shared rig CI uses, and the +# pool guard is a flat constant that does not scale with max_retry, so a few flaky boards can +# re-pay ~510s each until the 3600s guard fires, abandoning the pool and holding board flocks +# against concurrent CI. hil_test.py's own default is already 1; passing it explicitly keeps +# that true if the default ever moves. Placed first, not appended, so argparse's last-wins +# means `hil_ci.sh -r 3` still gets 3. +# Forward the HIL_* knobs (HIL_NO_BOARD_LOCK for an authorized force, the parallel widths, +# HIL_POOL_TIMEOUT). ssh passes no environment and joins its argv into one string the remote +# shell re-splits, so a bare NAME=value element would arrive as a positional argument to +# hil_test.py and argparse would exit 2. Build `export` lines instead and hand them over as a +# single %q-quoted word for the remote to eval. +# Joined with '; ', NOT newlines: %q renders a newline as bash-only $'...' quoting, which the +# remote LOGIN shell must parse from the joined command string -- under dash the force arrives +# as garbage and silently does nothing. Backslash escaping round-trips in both shells. +# HIL_REPORT_DIR stays local: where the report lands on the rig is this script's contract +# (REMOTE_DIR, where all three copy-backs below look), so forwarding it would relocate the +# report and every copy-back would come home empty. +HIL_EXPORTS="" +while IFS= read -r v; do + [ -z "$v" ] && continue + HIL_EXPORTS+="export $(printf '%s=%q' "$v" "${!v}"); " +done < <(compgen -v | grep -x 'HIL_[A-Z0-9_]*' | grep -vxE 'HIL_EXPORTS|HIL_REPORT_DIR' || true) +[ -n "$HIL_EXPORTS" ] && echo "==> Forwarding: $HIL_EXPORTS" +# One %q-quoted word, so ssh's argv join and the remote shell's re-split hand it back +# byte-for-byte, and the remote evals it. Empty stays `''` -- a real, shiftable argument -- +# rather than vanishing from the joined string and shifting the run's own flags out of place. +HIL_EXPORTS_Q=$(printf '%q' "$HIL_EXPORTS") + +ssh "$REMOTE" bash -s -- "$REMOTE_DIR" "$HIL_EXPORTS_Q" --retry 1 ${ARGS_Q[@]+"${ARGS_Q[@]}"} "$CONFIG_Q" <<'REMOTE' || rc=$? cd -- "$1" shift +eval "$1"; shift # HIL_* exports, %q-quoted locally into one word # Flasher CLIs live in the user bin dirs on ci.lan (esptool/idf in ~/.local/bin, # STM32CubeProgrammer's STM32_Programmer_CLI in ~/bin); the non-interactive shell # subprocess used for flashing doesn't source profile/rc, so add them explicitly. @@ -191,4 +318,15 @@ scp -q "$REMOTE:$REMOTE_DIR/hil_report.md" "$ROOT_DIR/hil_report.md" \ && echo "==> Report copied to $ROOT_DIR/hil_report.md" \ || echo "==> warning: no hil_report.md copied back" >&2 +# The re-run spec and the JSON sidecar live in the run's cwd on the rig (REMOTE_DIR), and the +# next invocation rm -rf's it. Without copying them back, the `--accumulate` retry every doc on +# this branch prescribes has nothing to read and nothing to merge onto. Delete the local copies +# FIRST: a green run writes no .failed, so a silent no-op scp would leave last run's spec in +# the checkout looking current, and "retry from the spec" would re-flash boards that passed. +for extra in "$(basename "$CONFIG").failed" hil_report.json; do + rm -f "$ROOT_DIR/$extra" + scp -q "$REMOTE:$REMOTE_DIR/$extra" "$ROOT_DIR/$extra" 2>/dev/null \ + && echo "==> $extra copied to $ROOT_DIR/$extra" || true +done + exit $rc diff --git a/test/hil/test/test_hil_bounded.py b/test/hil/test/test_hil_bounded.py index 908a142d5..c6d454f0e 100644 --- a/test/hil/test/test_hil_bounded.py +++ b/test/hil/test/test_hil_bounded.py @@ -1364,7 +1364,14 @@ class RemoteDirIsScreened(unittest.TestCase): rc = 0 if keep_going else 77 for tool in ('ssh', 'scp', 'rsync'): write_script(Path(td) / tool, f'echo "stub-{tool} $*" >&2; exit {rc}') + # hil_ci.sh now refuses an all-boards run with nothing built, so this arg-quoting + # test needs a checkout stub with one build dir to reach the run invocation + root = Path(td) / 'root' + (root / 'test' / 'hil').mkdir(parents=True) + (root / 'test' / 'hil' / 'hil_test.py').touch() + (root / 'examples' / 'cmake-build-alpha').mkdir(parents=True) env = {**os.environ, 'REMOTE_DIR': remote_dir, 'REMOTE': 'stub', + 'ROOT_DIR': str(root), 'PATH': td + os.pathsep + os.environ['PATH']} return subprocess.run( ['bash', str(Path(TEST_DIR).parents[0] / 'hil_ci.sh'), *args], @@ -1414,6 +1421,316 @@ class RemoteDirIsScreened(unittest.TestCase): self.assertIn(r'host/cdc\ msc', run_line) +class EveryBoardIsStaged(unittest.TestCase): + """One hil_test.py run takes several `-b` flags, and hil-operator hands it the whole board + set that way. The `-b` parse loop kept a single BOARD, so only the LAST board's binaries + were rsynced and every other board died on the rig with a missing firmware path -- after + its flash slot and lock were already spent. + + Three of these five fail against the pre-fix script (the discriminating unbuilt case + puts the board FIRST, because the old single-BOARD parse happened to handle a trailing + one correctly); the run-line and variant-dir tests are characterization -- the old script + already forwarded ARGS whole and read variants from the config for its one board.""" + + def _run(self, boards, cfg_boards=None, variants=None): + import json + import subprocess + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + root = Path(td.name) / 'root' + (root / 'test' / 'hil').mkdir(parents=True) + (root / 'test' / 'hil' / 'hil_test.py').touch() + built = cfg_boards if cfg_boards is not None else boards + (root / 'examples').mkdir(parents=True, exist_ok=True) + for b in built: + (root / 'examples' / f'cmake-build-{b}').mkdir(parents=True) + roster = [{'name': b} for b in boards] + for entry in roster: + for v in (variants or {}).get(entry['name'], []): + entry.setdefault('variant', []).append({'name': v}) + cfg = root / 'test' / 'hil' / 'cfg.json' + cfg.write_text(json.dumps({'boards': roster})) + stubs = Path(td.name) / 'bin' + stubs.mkdir() + # real ssh/scp/rsync would reach the rig; these just record the argv + for tool in ('ssh', 'scp', 'rsync'): + write_script(stubs / tool, f'echo "stub-{tool} $*" >&2; exit 0') + env = {**os.environ, 'REMOTE': 'stub', 'ROOT_DIR': str(root), 'CONFIG': str(cfg), + 'PATH': str(stubs) + os.pathsep + os.environ['PATH']} + args = [a for b in boards for a in ('-b', b)] + r = subprocess.run(['bash', str(Path(TEST_DIR).parents[0] / 'hil_ci.sh'), *args], + capture_output=True, text=True, timeout=60, env=env) + r.rsyncs = [l for l in r.stderr.splitlines() if l.startswith('stub-rsync')] + # the RUN ssh is the one carrying hil_test.py's args; the setup ssh is not + r.run_lines = [l for l in r.stderr.splitlines() if '--retry 1' in l] + return r + + def test_binaries_for_every_requested_board_are_copied(self): + r = self._run(['alpha', 'beta', 'gamma']) + self.assertEqual(r.returncode, 0, r.stderr) + for b in ('alpha', 'beta', 'gamma'): + self.assertTrue(any(f'cmake-build-{b} ' in l for l in r.rsyncs), + f'{b} binaries never staged: {r.rsyncs}') + + def test_every_board_reaches_hil_test(self): + r = self._run(['alpha', 'beta']) + self.assertEqual(len(r.run_lines), 1, r.stderr) + self.assertIn('-b alpha', r.run_lines[0]) + self.assertIn('-b beta', r.run_lines[0]) + + def test_an_unbuilt_board_aborts_before_anything_is_staged(self): + """The discriminating case: the unbuilt board is FIRST. The pre-fix script kept only + the last -b, found it built, and ran happily while silently testing one board. It also + has to fail BEFORE staging -- the old in-loop check fired after the remote tree was + wiped and earlier boards were rsynced, costing a run and leaving a half-staged rig.""" + r = self._run(['alpha', 'beta'], cfg_boards=['beta']) + self.assertNotEqual(r.returncode, 0, 'unbuilt first board was accepted') + self.assertIn('alpha', r.stdout + r.stderr) + self.assertEqual(r.rsyncs, [], f'staged despite an unbuilt board: {r.rsyncs}') + self.assertEqual(r.run_lines, [], 'reached the run despite an unbuilt board') + + def test_a_board_whose_firmware_is_only_a_variant_dir_is_accepted(self): + """Variant names are not required to be prefixed with the board name, so a board can + own no `cmake-build-` dir at all. A pre-flight that only globs the board name + rejects it and tells the user to build firmware that is already there.""" + r = self._run(['alpha', 'beta'], cfg_boards=['alpha', 'odd-name-v'], + variants={'beta': ['odd-name-v']}) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertTrue(any('cmake-build-odd-name-v ' in l for l in r.rsyncs), + f"beta's variant dir never staged: {r.rsyncs}") + + def test_all_unbuilt_boards_are_named_at_once(self): + """One build round should fix every complaint, so the guard reports the whole set.""" + r = self._run(['alpha', 'beta', 'gamma'], cfg_boards=['beta']) + self.assertNotEqual(r.returncode, 0) + out = r.stdout + r.stderr + self.assertIn('alpha', out) + self.assertIn('gamma', out) + + +class StagingCoversEveryBoardForm(unittest.TestCase): + """hil_test.py declares `-b, --board` with action='append', so argparse accepts --board X, + --board=X and -bX too. Staging only the bare form sent boards to the rig with no firmware, + where every test logs `Skip (no binary)` and counts zero errors -- a green row for a board + that was never flashed. Also covers the roster check, which has to fire BEFORE the remote + tree is wiped, since hil_test.py rejects an unknown -b for the whole run.""" + + def _run(self, argv, built, roster=None, variants=None, env_extra=None, stale=None): + import json + import subprocess + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + root = Path(td.name) / 'root' + (root / 'test' / 'hil').mkdir(parents=True) + (root / 'test' / 'hil' / 'hil_test.py').touch() + (root / 'examples').mkdir(parents=True, exist_ok=True) + for b in built: + (root / 'examples' / f'cmake-build-{b}').mkdir(parents=True) + entries = [{'name': b} for b in (roster if roster is not None else built)] + for e in entries: + for v in (variants or {}).get(e['name'], []): + e.setdefault('variant', []).append({'name': v}) + cfg = root / 'test' / 'hil' / 'cfg.json' + cfg.write_text(json.dumps({'boards': entries})) + stubs = Path(td.name) / 'bin' + stubs.mkdir() + # ssh joins its argv into ONE string that the REMOTE shell re-splits, and feeds the + # heredoc on stdin. A stub that echoes "$*" hides exactly that, which is how a + # completely broken env-forwarding change once passed its own test -- so this stub + # re-splits like the real thing and reports the script body separately. + write_script(stubs / 'ssh', 'shift; printf "REMOTE-ARGV: %s\\n" "$*" >&2; ' + 'body=$(cat); printf "REMOTE-BODY: %s\\n" "$body" >&2; exit 0') + for tool in ('scp', 'rsync'): + write_script(stubs / tool, f'echo "stub-{tool} $*" >&2; exit 0') + for name, content in (stale or {}).items(): + (root / name).write_text(content) + env = {**os.environ, 'REMOTE': 'stub', 'ROOT_DIR': str(root), 'CONFIG': str(cfg), + 'PATH': str(stubs) + os.pathsep + os.environ['PATH'], **(env_extra or {})} + r = subprocess.run(['bash', str(Path(TEST_DIR).parents[0] / 'hil_ci.sh'), *argv], + capture_output=True, text=True, timeout=60, env=env) + r.rsyncs = [l for l in r.stderr.splitlines() if l.startswith('stub-rsync')] + r.run_lines = [l for l in r.stderr.splitlines() if '--retry 1' in l] + r.body = '\n'.join(l for l in r.stderr.splitlines() if l.startswith('REMOTE-BODY')) + r.stale_left = {name: (root / name).exists() for name in (stale or {})} + return r + + def test_long_board_forms_are_staged_and_only_that_board(self): + """Two boards are built so the pre-fix 'copy all built binaries' else-branch cannot + stage the right one by accident -- that is what made the first version of this test + pass against master while the feature was broken. -balpha is the glued short form + argparse resolves to --board alpha; unparsed it fell through to the all-boards branch + and silently staged everything built with no roster check.""" + for argv in (['--board', 'alpha'], ['--board=alpha'], ['-balpha']): + with self.subTest(argv=argv): + r = self._run(argv, built=['alpha', 'beta'], roster=['alpha', 'beta']) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertTrue(any('cmake-build-alpha ' in l for l in r.rsyncs), + f'{argv} never staged: {r.rsyncs}') + self.assertFalse(any('cmake-build-beta ' in l for l in r.rsyncs), + f'{argv} staged an unrequested board: {r.rsyncs}') + + def test_board_test_flag_is_not_mistaken_for_a_board(self): + """-bt is hil_test.py's --board-test and is exactly what .failed contains, so + a glued -b?* pattern turns the documented retry into 'not in the roster: t'.""" + for argv in (['-b', 'alpha', '-bt', 'alpha:device/cdc_msc'], + ['-b', 'alpha', '-btalpha:device/cdc_msc']): + with self.subTest(argv=argv): + r = self._run(argv, built=['alpha']) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertNotIn('not in', r.stderr) + self.assertTrue(any('alpha:device/cdc_msc' in l for l in r.run_lines), + f'-bt never reached the rig: {r.run_lines}') + + def test_a_board_outside_the_roster_is_refused_before_staging(self): + r = self._run(['-b', 'alpha', '-b', 'ghost'], built=['alpha', 'ghost'], roster=['alpha']) + self.assertNotEqual(r.returncode, 0) + self.assertIn('ghost', r.stdout + r.stderr) + self.assertEqual(r.rsyncs, [], 'staged despite an unknown board') + self.assertEqual(r.run_lines, [], 'reached the run despite an unknown board') + + def test_a_variant_with_no_build_dir_warns_instead_of_passing_silently(self): + r = self._run(['-b', 'alpha'], built=['alpha'], variants={'alpha': ['alpha-DMA']}) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertIn('alpha-DMA', r.stderr) + self.assertIn('skipped, not tested', r.stderr) + + def test_no_build_dirs_at_all_aborts_the_all_boards_form(self): + """`hil_ci.sh` with no -b stages everything built. With nothing built it used to wipe + the rig, stage nothing, and return a green all-skip table.""" + r = self._run([], built=[], roster=['alpha']) + self.assertNotEqual(r.returncode, 0) + self.assertIn('nothing to test', r.stdout + r.stderr) + self.assertEqual(r.run_lines, [], 'reached the run with nothing staged') + self.assertNotIn('Setting up remote', r.stdout + r.stderr, + 'the guard fired only after the remote tree was already wiped') + + def test_hil_env_reaches_the_rig_as_environment_not_argv(self): + """An authorized force is HIL_NO_BOARD_LOCK=1. Passed through ssh's argv it arrives as a + positional argument and argparse exits 2, so it has to travel in the script body.""" + r = self._run(['-b', 'alpha'], built=['alpha'], env_extra={'HIL_NO_BOARD_LOCK': '1'}) + self.assertEqual(r.returncode, 0, r.stderr) + # one %q-quoted word of `export NAME=value; ` fragments, evaluated by the remote — + # NOT a bare NAME=value element, which hil_test.py's argparse takes as a positional. + # %q backslash-escapes the spaces, so match the pieces rather than the plain phrase. + run = '\n'.join(r.run_lines) + self.assertIn('HIL_NO_BOARD_LOCK=1', run) + self.assertIn('export', run) + self.assertFalse(any(' HIL_NO_BOARD_LOCK=1 ' in l for l in r.run_lines), + 'env reached argv unquoted, where hil_test.py sees a positional') + + def test_a_value_with_spaces_survives_forwarding(self): + r = self._run(['-b', 'alpha'], built=['alpha'], + env_extra={'HIL_SCRATCH': '/tmp/my scratch'}) + self.assertEqual(r.returncode, 0, r.stderr) + run = '\n'.join(r.run_lines) + self.assertIn('HIL_SCRATCH', run) + self.assertIn('scratch', run) + + def test_hil_report_dir_is_never_forwarded(self): + """Where the report lands on the rig is this script's contract (REMOTE_DIR, where all + three copy-backs look); forwarding a local HIL_REPORT_DIR relocates it there and every + copy-back comes home empty -- two of the three silently.""" + r = self._run(['-b', 'alpha'], built=['alpha'], + env_extra={'HIL_REPORT_DIR': '/tmp/elsewhere'}) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertFalse(any('HIL_REPORT_DIR' in l for l in r.run_lines), + f'HIL_REPORT_DIR reached the rig: {r.run_lines}') + + def test_a_stale_local_failed_spec_does_not_survive_a_green_run(self): + """A green run writes no .failed on the rig, so the copy-back scp no-ops; the local + spec from a previous FAILED run must not survive it looking current -- a later + "retry from the spec" would re-flash boards that already passed.""" + r = self._run(['-b', 'alpha'], built=['alpha'], + stale={'cfg.json.failed': '--accumulate -b alpha'}) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertFalse(r.stale_left['cfg.json.failed'], + "last run's re-run spec survived a green run") + + +class SummaryFoldsReportToBoards(unittest.TestCase): + """hil_summary.py replaces the agent retyping the markdown table. Report rows are named per + VARIANT and a variant need not start with the board name, so the config is what maps them + back -- the previous string-matching design produced a defect in each of four review rounds.""" + + def _sum(self, boards, rows, cfg_boards=None, banner=''): + import json + import subprocess + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + d = Path(td.name) + (d / 'hil_report.json').write_text(json.dumps( + {'rows': [{'board': b, 'cells': c, 'duration': '1s'} for b, c in rows], + 'banner': banner})) + cfg = d / 'cfg.json' + cfg.write_text(json.dumps({'boards': cfg_boards or [{'name': b} for b in boards]})) + args = [a for b in boards for a in ('-b', b)] + r = subprocess.run(['python3', str(Path(TEST_DIR).parents[0] / 'helper' / 'hil_summary.py'), + str(cfg), *args, '--report-dir', str(d)], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + return json.loads(r.stdout)['results'] + + def test_variant_rows_fold_onto_their_board(self): + """nanoch32v203 never produces a row named after the board.""" + got = self._sum(['nanoch32v203'], + [('nanoch32v203-fsdev', {'usbtest': 'pass'}), + ('nanoch32v203-usbfs', {'usbtest': 'pass'})], + cfg_boards=[{'name': 'nanoch32v203', + 'variant': [{'name': 'nanoch32v203-fsdev'}, + {'name': 'nanoch32v203-usbfs'}]}]) + self.assertEqual([r['board'] for r in got], ['nanoch32v203']) + self.assertTrue(got[0]['pass']) + self.assertTrue(got[0]['ran']) + + def test_one_failing_variant_fails_the_board(self): + got = self._sum(['nano'], + [('nano-a', {'usbtest': 'pass'}), ('nano-b', {'usbtest': '❌ 29/30'})], + cfg_boards=[{'name': 'nano', 'variant': [{'name': 'nano-a'}, + {'name': 'nano-b'}]}]) + self.assertFalse(got[0]['pass']) + self.assertIn('29/30', got[0]['detail']) + + def test_lock_contention_is_a_field_not_a_prefix(self): + got = self._sum(['alpha'], [('alpha', {'board-locked': 'fail'})]) + self.assertTrue(got[0]['locked']) + self.assertFalse(got[0]['pass']) + + def test_a_board_with_no_row_is_marked_not_run(self): + got = self._sum(['alpha', 'beta'], [('alpha', {'usbtest': 'pass'})]) + self.assertTrue(got[0]['ran']) + self.assertFalse(got[1]['ran']) + self.assertFalse(got[1]['pass']) + + def test_a_metric_cell_counts_by_its_icon(self): + got = self._sum(['a', 'b'], [('a', {'cdc_msc_throughput': '✅ C 1.2 M 3.4'}), + ('b', {'cdc_msc_throughput': '❌ C 0.0 M 0.0'})]) + self.assertTrue(got[0]['pass']) + self.assertFalse(got[1]['pass']) + + def test_skipped_cells_do_not_fail_a_board(self): + got = self._sum(['a'], [('a', {'usbtest': 'skip', 'cdc_msc': 'pass'})]) + self.assertTrue(got[0]['pass']) + + def test_a_plain_metric_cell_is_a_pass(self): + """Mirrors hil_test.py's own tally (cell_kind): failures are ALWAYS marked -- 'fail' + or a ❌ prefix, per TestFail's docstring -- while a passing test may return a plain + metric string that lands in the cell unprefixed. Treating unknown shapes as fail + would publish a green table as a red verdict.""" + got = self._sum(['a'], [('a', {'device_speed': '480.0 MBps'})]) + self.assertTrue(got[0]['pass']) + + def test_a_declared_variant_of_another_board_is_not_stolen(self): + """A declared variant need not start with its own board's name, so it may start with + a DIFFERENT board's name plus '-'. The prefix fallback must not attribute it twice.""" + got = self._sum(['alpha', 'beta'], + [('beta-x', {'usbtest': 'fail'})], + cfg_boards=[{'name': 'alpha', 'variant': [{'name': 'beta-x'}]}, + {'name': 'beta'}]) + self.assertTrue(got[0]['ran']) + self.assertFalse(got[0]['pass']) + self.assertFalse(got[1]['ran'], "beta must not inherit alpha's row") + + class CaveatSurvivesAccumulate(unittest.TestCase): """CI reruns with --accumulate: the sidecar keeps every earlier attempt's cells, but the banner was recomputed per attempt. A first attempt on a degraded rig and a clean rerun -- cgit v1.3.1 From be86281b81b70adc1ce3746d620c912bf032fac2 Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 20 Aug 2026 16:45:32 +0700 Subject: hil-pool-check: document the probe power-cycle escalation; name the env script directly A probe whose firmware has wedged reports flash-failed with the probe present and "probe toggle unconfirmed". The tool's own recovery cannot fix that: an authorized toggle re-enumerates but never removes power, and hil_pool_check.py:304 already notes that ST-Link, WCH-Link, CP210x and picoprobe keep their sysfs kobject across one. So the check correctly gives up, and the operator was left to invent the next rung. Write it down, as what this rig's hardware actually does rather than what uhubctl advertises: the Renesas cards list their root hubs as ppps-capable but do not implement it (owner-confirmed - VBUS never drops, only D+/D-), so a root-port cycle is a harder forced re-enumeration that a wedged probe can ride out, worth exactly one attempt; and the AMD 0000:02:00.0, where the WCH-Links live, has no port-power switching at all - nothing to cycle, straight to a physical replug. Which card a probe hangs off decides which case applies, so the procedure starts from readlink. The ordering rules encode the shared-rig protocol: let the full run finish (a bounce re-enumerates siblings and corrupts checks still in flight), hold --all with this host's --config before the cycle (hil_lock.py hold validates nothing against the roster and nothing maps a sysfs busport to a board name, so a narrower hand-listed hold reserves nothing while reporting success - and --all defaults to tinyusb.json, which on the tusb rig would reserve 27 boards that do not exist there), release BEFORE the re-check (hil_pool_check.py self-locks every board it checks, so a hold still in place makes the verification report locked against your own hold and verify nothing), and drive the cycle through usb_recover.sh root-cycle by its full in-repo path - it is on no PATH and sudo's secure_path excludes the checkout. Never a bare `uhubctl -a cycle`: without -S it writes sysfs disable, whose disable_store takes the root hub's lock uninterruptibly and then usb_disconnect()s the wedged child - the one input that turns a probe wedge into a bus-wide wedge. Give the script the wedged probe's own busport, not the hub path: the serial guard and the success check both read the path you pass, and the hub's inode always changes when its own port cycles. Reporting asks for both passes: a final table showing every board healthy hides that a probe needed power-cycling to get there, which is the signal that it will recur. Also: the ESP-IDF env hints name `. $HOME/code/esp-idf/export.sh` instead of the `get-idf` alias, which lives only in interactive shells and fails from scripts. --- .claude/skills/hil-pool-check/SKILL.md | 75 +++++++++++++++++++++++++++++++++- test/hil/helper/hil_pool_check.py | 6 ++- 2 files changed, 78 insertions(+), 3 deletions(-) (limited to 'test') diff --git a/.claude/skills/hil-pool-check/SKILL.md b/.claude/skills/hil-pool-check/SKILL.md index 6a8f66087..3c056c16b 100644 --- a/.claude/skills/hil-pool-check/SKILL.md +++ b/.claude/skills/hil-pool-check/SKILL.md @@ -41,7 +41,10 @@ ssh ci.lan 'bash -lc "cd ~/code/tinyusb && python3 test/hil/helper/hil_pool_chec Missing firmware is **built on the spot** — never skipped (`--no-build` opts out; those boards then report `flash-failed`). Builds need the family env, exported on the rig in `~/.profile`/`~/.bashrc`: `PICO_SDK_PATH` for rp2040/rp2350 (`~/code/pico/pico-sdk`), the -ESP-IDF env (`get-idf`) for espressif — which also needs `esptool` on PATH (pip's +ESP-IDF env for espressif — source it as `. $HOME/code/esp-idf/export.sh`, NOT as `get-idf`: +that is an interactive shell alias (`~/.bashrc`), and aliases are not expanded in non-interactive +shells, so scripts and agents get `get-idf: command not found` even under `bash -lc`. It also +needs `esptool` on PATH (pip's `~/.local/bin/esptool`; a non-login shell may lack it — run via `bash -lc`). An explicit `-B` is searched exclusively for *existing* firmware; builds still land in `cmake-build/` and are noted `built `. Espressif boards park too when the IDF env is present. A first run on an @@ -57,8 +60,78 @@ are *unverified*, not healthy — read the footer, not just `$?`. A `⚠ pid … means stale firmware or a silent flash no-op (J-Link lore); a device off the bus entirely needs the usb-kernel-recover skill or a physical replug. +## When the tool's probe recovery fails + +`flash-failed` with the probe ✅ present and a `probe toggle unconfirmed` note means the probe's +own firmware is wedged, not the board. The tool's recovery is an `authorized` toggle, which is a +USB re-enumeration and never removes power, so probes that keep their sysfs kobject across it +(ST-Link, WCH-Link, CP210x, picoprobe) survive the toggle still wedged. Confirm with the flasher's +own list — `STM32_Programmer_CLI -l st-link`, or `JLinkExe -CommandFile