Repository navigation
xhci: arms for the verdict function that says it was written to be testable - #2305
Conversation
…stable tools/xhci_boot_capture.sh's verdict function documents its own shape: # A function, not an inline block, so the three classifications can be driven on a fixture without a device (the # same reason every other rule in this tree is header-only/function-shaped: the decision is what can be wrong). It was written to be testable and then was not tested -- the four verdicts only entered the record through a manual drive during engine#823's review, which is exactly the evidence a merge cannot keep. This is that drive as arms. control=auto + suspended -> rc 1, "the keep-awake rule did NOT hold it" <- the fault #823 named control=0 + suspended -> rc 0, "held out of D0 deliberately" active -> rc 0, and NOTHING printed resuming -> rc 2, "a transition that did not settle" current -> rc 3, "is not one of ..." <- an unnameable state is its own verdict The empty-line arm is deliberate: an awake controller is silent, and a verdict file that says nothing about a fault is the failure the capture exists to avoid, so the silence is asserted rather than assumed. (It also needed its own branch in the harness -- `grep -qF ""` always matches, which is how the first version of this arm passed for the wrong reason.) Verified, both directions: unchanged function -> 5 arms pass, ctest `xhci_verdict_line` Passed the auto/0 distinction broken (`if [ "$_ctl" = auto ]` -> `if false`) -> rc 1: "FAIL control=auto + suspended is the fault (engine#823): want rc 1, got 0" The capture script runs its own capture at the bottom, so the arms extract the function rather than sourcing the script: they assert the decision, which is what the comment says can be wrong, and never the device.
|
VERDICT: READY at "It was written to be testable and then was not tested"# A function, not an inline block, so the three classifications can be driven on a fixture without a device
# (the same reason every other rule in this tree is header-only/function-shaped: the decision is what can be wrong).That comment is mine — I reviewed #2232 and praised exactly this property: "It is a function so it can be tested without a device." The property was true, the opportunity was real, and no test was written, so the four verdicts (settled / transition / held / This is the What the arms have to cover, given #823's questionThe device question is "which of four ports has the device, on a cold boot with Secure Boot on", and the tool's answer is a per-controller verdict. So the cases that matter are:
Arm 4 is the one I would insist on: a fixture with an unnameable state must produce Two asks
And the row's status is unchanged#823 still needs the owner's four physical plugs — no test can substitute for a cold boot with one device per port under Secure Boot. But the arms make the interpretation trustworthy before that window, which is what this PR is for: so that when the owner does plug, a wrong verdict is a bug and not an ambiguity. |
|
Arms verified by running them — Driving the decision instead of the device is the right call, and the arm set is the right set: the deliberate-suspension case ( One hardening, because the extraction relies on formatting. The function is pulled out with: sed -n '/^xhci_verdict_line()/,/^}/p' "$src" > "$fn"
...
. "$fn"Two things this depends on that nothing states or checks:
Two lines close both, without changing the extraction: [ "$(tail -1 "$fn")" = "}" ] || { echo "FAIL the extracted text does not end at the function's closing brace"; exit 1; }
grep -q "^xhci_verdict_line()" "$fn" || { echo "FAIL the extraction is not the verdict function"; exit 1; }or extract by brace depth instead of by a regex range. Worth doing precisely because the failure mode would look like a working test right up until it touched a machine. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
tools/xhci_boot_capture.sh's verdict function documents its own testability and had no test:It was written to be testable and then was not tested — the four verdicts only entered the record through a manual drive during engine#823's review, which is exactly the evidence a merge cannot keep. This is that drive, as arms.
The five
The silent arm is deliberate. An awake controller prints nothing and returns 0 — the line exists to explain a state, and "healthy" is not one of the things it explains. A verdict file that says nothing about a fault is the failure this whole capture exists to avoid, so the silence is asserted rather than assumed.
Two things the arms caught in themselves
grep -qF ""always matches — the first version of the empty-line arm passed for the wrong reason. The harness now handles the empty needle explicitly.activearm was wrong before the function was. I expected a line; the function is silent by design. Corrected the arm, not the code — the usual direction for a fixture, and worth saying because the reflex is to "fix" the script.Verified, both directions
The capture script runs its own capture at the bottom, so the arms extract the function (
sed -n '/^xhci_verdict_line()/,/^}/p') rather than sourcing the script: they assert the decision, which is what the comment says can be wrong, and never the device.Registration
One
add_testrow, no build target — a script row, whichci_targets_checkdocuments as free ("A script test names its interpreter … and bash is not an add_executable, so a self-test runner costs nothing here").ci_targets_check --checkrc 0,tmp_path_check --checkrc 0,copyright.py --checkrc 0 after--fix(the header must be the full Apache paragraph, not the short SPDX form).