Skip to content

Commit 04f4dcf

Browse files
docs(agents): say which runner the EXIT-trap failure mode needs
§3 bans test-level EXIT traps because such a trap displaces the handler the bats binary installs, so a failing test prints no TAP line and a grep for `not ok` reads a green suite. That is true, and it is unreachable in CI. hack/cozytest.sh runs each @test inside an explicit subshell and reads the status out of band on the next line, so a trap in a test body fires when that subshell exits and takes nothing with it. The real bats binary is invoked nowhere in the tree -- make bats-unit-tests and every recipe in packages/core/testing/Makefile go through cozytest.sh. The disappearing `not ok` needs someone running bats by hand. Worth writing down because the rule has cost four PRs and three successive mechanisms, and the omission reads as an unpaid backlog: 68 declared traps that a reader assumes CI is one refactor away from needing. It is a review aid over a hazard outside CI. The note also records that two of the covered files resist a mechanical conversion, so the next person to try does not discover it halfway. No other line of the document changes. Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent 516be4a commit 04f4dcf

1 file changed

Lines changed: 1 addition & 0 deletions

File tree

‎docs/agents/e2e-testing.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ Trap-based cleanup was the single biggest source of false failures in the BATS s
4545
- Chainsaw deletes the resources it `apply`-ed during its cleanup phase (bounded by the `delete`/`cleanup` timeouts in `hack/e2e-chainsaw/.chainsaw.yaml`). Do not hand-roll teardown for resources Chainsaw created.
4646
- A self-contained `trap '… ' EXIT` **inside a single `script` step** — to kill a port-forward or remove a temp dir — is fine, because it runs in a contained subprocess with its variables in scope. See `hack/e2e-chainsaw/bucket/chainsaw-test.yaml`. What is banned is test-level trap-based cleanup of the BATS kind. The same carve-out holds inside a BATS `@test` when the trap sits in an explicit subshell — `( … trap "kill $pid" EXIT … )` — because a subshell trap does not replace the one the `bats` binary installs, so a failure inside it still prints its `not ok`. `hack/e2e-test-openapi.bats` relies on this to kill a backgrounded `kubectl proxy`; moving that cleanup to the end of the body would leak a process holding a fixed port rather than fix anything.
4747
- The ban extends to every BATS file under `hack/`, subdirectories and the `e2e-` prefixed ones included, for a second reason worth knowing before you debug one: an `EXIT` trap inside an `@test` body replaces the one the `bats` binary installs for its own bookkeeping, and a test that then **fails** prints no TAP line at all. It does not appear as `not ok`; it disappears, and the run ends with `# bats warning: Executed N instead of expected M tests` and a non-zero exit. Verified with Bats 1.14.0. Anyone reading the tail of the output, or grepping it for `not ok`, sees a green suite — and the CI runner `hack/cozytest.sh`, which is not the `bats` binary, reports the same failure correctly, so the two disagree exactly when it matters. Clean up at the end of the test body instead: both runners set `-e`, so the cleanup is unreachable on failure and the scratch directory is left behind for inspection, which is what you want from a failed test anyway. When a suite reports zero failures, confirm it also reports how many tests it ran: an exit code answers "did anything fail", never "did anything run". `hack/bats-no-exit-trap.bats` enforces this across every `hack/**/*.bats`, subdirectories included: a file that carries no `# EXIT-TRAP DEBT: N` comment must contain no EXIT-trap line at all, and a file that carries one must install exactly N — so a trap appearing or disappearing fails until the file's own number is corrected. Note what that does and does not buy: it is a ratchet on the number's *accuracy*, not on the debt itself, because adding a trap and raising `N` in the same change is green. Nothing mechanical stops the count growing — review does, which is the point of the number living in the file being reviewed. A trap inside an explicit subshell is exempt from the ban but still counted, so a declaration is not by itself an admission of debt; `hack/e2e-test-openapi.bats` is the current example and says so in its own header. The scan reads `.bats` files only, so a handler reaching a test body from a sourced `.sh` is outside it — `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two, each benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and runs in a subshell, and the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Three `hack/*.bats` source that library. Treat the guard as a ratchet over a common spelling, not as proof that a file installs no handler. A handful of files still declare a debt while their conversion waits on the branches that own them; when one of those reports zero failures, reconcile its `1..N` plan against its `ok` count before believing the run.
48+
- **Know which runner that failure mode needs, before you spend a PR on it.** `hack/cozytest.sh` executes each `@test` inside an explicit subshell and reads the status out of band on the line after it (`( … "$fn" )` then `printf '__RC__%s\n' "$?"`), so under the CI runner a trap in a test body neither displaces the runner's own handler nor costs a failing test its verdict — it fires when that subshell exits, which is what it was written to do. And the real `bats` binary is invoked nowhere in this tree: `make bats-unit-tests` and every recipe in `packages/core/testing/Makefile` go through `hack/cozytest.sh`. The disappearing-`not ok` failure is therefore reachable only by running `bats` by hand on a file locally, which is worth knowing precisely because that is the situation in which someone is already confused. Read the ratchet accordingly: it is a review aid over a hazard that lives outside CI, and the traps it counts are a note about where the two runners differ, not a backlog to burn down. Converting one for its own sake is not an improvement, and two of the files it covers would need more than a mechanical edit — a body that `cd`s into its scratch directory and never returns would delete its own cwd under a single-process runner, and a file whose last command decides its verdict changes meaning when a cleanup line is appended after it.
4849

4950
### 4. Do not mask cleanup or teardown failures
5051

0 commit comments

Comments
 (0)