From 575b41e1cd84ea438cb2ea35f4c66355f69d2210 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Fri, 7 Aug 2026 11:50:55 +0300 Subject: [PATCH 1/2] test(hack): let each bats file declare the EXIT traps it still holds The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. A change adding a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second landed. A file arriving with traps hit the same wall from the other side -- the inventory did notice it, since the string it compared was built by scanning the directory, but absorbing it meant an edit in a file nothing in the author's diff pointed at. Replace both lists with a declaration each file makes about itself: one "# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file carrying none must install no EXIT trap, which covers the converted files, the files that never had one, and every file added later. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count collide textually instead of silently. The declaration is read from the leading comment block, not from anywhere in the file. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test. Reading it there as a statement about the whole file would let an unrelated fixture excuse a real trap, and would do it silently, since nothing in that test's own diff looks like a declaration. A comment block is the region with no interior: stopping instead at the first @test would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the bats binary's own, so a test failing inside `( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy that way, and moving the kill to the end of the body would leak a process holding a fixed port. Its declaration records that rather than scheduling a conversion, and because the ratchet is exact, removing the trap fails too -- the count protects the construct. What the count cannot do is tell the two apart: substituting a test-level trap for the subshell one keeps the total at 1 and stays green, which the header states rather than leaves to be found. Counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap ... EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which bash and dash both install. All are real handlers that scored zero, and the left boundary matters most, since the inventory being replaced had none and caught the semicolon form. A bare word boundary is not enough either way: `bootstrap ` ends in `trap `, and it must keep scoring nothing. A quoted signal counts for the same reason as the rest. Two handlers sharing one line are reported rather than counted, since the count is a count of lines and the second would otherwise arrive without moving the total; splitting the line properly needs a shell parser, a semicolon inside a handler's own action not being a separator, and guessing wrong undercounts -- the one direction a ratchet cannot afford. The scan recurses, so hack/e2e-apps/*.bats is covered rather than sitting one directory below the guard that claims the tree. It reads .bats and nothing else, so a handler arriving through a sourced .sh stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own reason rather than by design: one sits in a function declared with `(` and so runs in a subshell, the other in a brace function no @test calls. That boundary is stated in the header rather than papered over. The guard moves to hack/bats-no-exit-trap.bats: its subject is every unit suite under hack/, not the report collector it grew up in. Its fixture helpers assemble the trap keyword and the signal from separate arguments, because the guard scans its own source and a fixture written as a literal would be counted as a real trap in it. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- AGENTS.md | 2 +- docs/agents/e2e-testing.md | 6 +- hack/bats-no-exit-trap.bats | 700 ++++++++++++++++++ hack/build-matrix_test.bats | 1 + hack/capture-dataplane.bats | 1 + hack/cozyreport.bats | 97 --- hack/e2e-test-openapi.bats | 8 + hack/multus-install-cni-plugins.bats | 1 + hack/nightly-mirror_test.bats | 1 + hack/overlay-main-images_test.bats | 1 + hack/release-changelog-behaviour.bats | 1 + hack/release-changelog-contract.bats | 1 + ...run-kubernetes-talos-diagnostics_test.bats | 1 + hack/select-e2e_test.bats | 1 + 14 files changed, 721 insertions(+), 101 deletions(-) create mode 100644 hack/bats-no-exit-trap.bats diff --git a/AGENTS.md b/AGENTS.md index 2a63220da..458e87278 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,7 +28,7 @@ This file provides structured guidance for AI coding assistants and agents worki - **E2E tests and E2E CI** (e.g., "write/fix an e2e test", "stabilize a flaky test", "add a bats test", "change the e2e workflow", "why did e2e fail") - Read: [`e2e-testing.md`](./docs/agents/e2e-testing.md) - - Action: Read the entire file and follow the conventions exactly — no new retries on deterministic steps, event-driven backstops before every `kubectl wait`, no `EXIT`/`RETURN` traps, fail-fast on HelmRelease readiness + - Action: Read the entire file and follow the conventions exactly — no new retries on deterministic steps, event-driven backstops before every `kubectl wait`, no test-level `EXIT`/`RETURN` traps (a self-contained one inside a Chainsaw `script` step or an explicit subshell is fine — see the doc), fail-fast on HelmRelease readiness - **General questions about contributing** - Read: [`contributing.md`](./docs/agents/contributing.md) diff --git a/docs/agents/e2e-testing.md b/docs/agents/e2e-testing.md index 67bc5e729..9742d90b0 100644 --- a/docs/agents/e2e-testing.md +++ b/docs/agents/e2e-testing.md @@ -43,8 +43,8 @@ A Chainsaw `assert` polls until the resource matches its expected shape or the p Trap-based cleanup was the single biggest source of false failures in the BATS suite: an `EXIT` trap ran cleanup in a context where shell variables were unset, `set -u` killed it, and a **successful** test was marked failed — then retried twice into real failures. - 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. -- 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 ban extends to the unit BATS under `hack/*.bats`, 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". A guard test in `hack/cozyreport.bats` enforces this over a named list of files, which grows as the remaining ones are converted; a file not yet on that list may still carry a trap, so reconcile its `1..N` plan against its `ok` count before believing a green-looking run. +- 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. +- 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. ### 4. Do not mask cleanup or teardown failures @@ -122,7 +122,7 @@ The suite is pinned to Chainsaw **v0.2.15** (the latest release as of May 2026); 1. No new retry loop unless the step is pure infra (image pull / VM boot / network). 2. Resource readiness uses a Chainsaw `assert` (not an imperative `until kubectl get …; kubectl wait`); condition checks use the filter-as-list form `(conditions[?type == 'Ready'])`. 3. Imperative-only waits live in a `script` step; any bare `sleep` carries a `TODO(e2e-replace-fixed-timeouts):` justification. -4. No test-level `EXIT`/`RETURN` trap — rely on Chainsaw cleanup; a self-contained trap is allowed only inside a single `script` step (port-forward / temp dir). +4. No test-level `EXIT`/`RETURN` trap — rely on Chainsaw cleanup; a self-contained trap is allowed inside a single `script` step (port-forward / temp dir), and inside an explicit subshell within a BATS `@test`, where it does not displace the runner's own handler. A BATS file that still carries test-level traps declares the count itself in `# EXIT-TRAP DEBT: N`; do not ask for one to be removed without checking that line, because the count is exact in both directions. 5. Controller-created artifacts the test cannot reclaim are pruned explicitly; nested tenants delete child → parent with a wait-for-deletion between. 6. Standard HR-Ready assert timeout is **5–6m**; longer waits (harbor 10m, NFS 10m, VM image pulls, platform-wide install 15m) are justified in-line. 7. Failure path attaches scoped diagnostics via a `catch:` block, never a silent pass. diff --git a/hack/bats-no-exit-trap.bats b/hack/bats-no-exit-trap.bats new file mode 100644 index 000000000..3c26e220a --- /dev/null +++ b/hack/bats-no-exit-trap.bats @@ -0,0 +1,700 @@ +#!/usr/bin/env bats +# ----------------------------------------------------------------------------- +# The ban on EXIT-trap cleanup in hack/*.bats, and the debt each file that still +# breaks it declares for itself. +# +# Why the ban. 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. Not `not ok` -- nothing. The run ends with a warning that it executed +# fewer tests than it planned, so anyone reading the tail of the output, or +# grepping it for `not ok`, sees a green suite. Not hypothetical: +# hack/promote-retag_test.bats reported five passes and no failures while six of +# its eleven tests printed nothing. Clean up at the end of the test body instead. +# Both runners set -e, so on failure the cleanup is unreachable and the scratch +# directory survives for inspection, which is what a failed test wants anyway. +# Fuller reasoning, and the two exceptions -- a self-contained trap inside a +# Chainsaw `script` step, and one inside an explicit subshell -- are in +# docs/agents/e2e-testing.md. +# +# The debt belongs to the file that owes it, and not to a central inventory: put +# the count somewhere else and a change that adds a trap has to edit a suite it +# otherwise never opens, so two changes sharing no line invalidate each other -- +# both green against their own base, both merging cleanly, red only once the +# second lands. +# +# The declaration is one comment line in the file's leading comment block, under +# the shebang and above the first line of code: +# +# # EXIT-TRAP DEBT: 12 +# +# There and not merely somewhere, because a .bats file is shell that writes +# shell: the same line inside a heredoc, a fixture or an expected-output string +# is data belonging to one test, and honouring it there would let an unrelated +# fixture excuse a real trap. The leading comment block rather than everything +# above the first @test, because helper functions and their heredocs sit between +# the two; a comment block is the region with no interior. +# +# It is exact, not a ceiling. Adding a trap fails; removing one fails too, and +# the fix is to lower the number, or delete the line once it reaches zero. A file +# with no declaration must hold no traps, which covers the converted files, the +# files that never had one, and every file added tomorrow. +# +# NOT every counted handler is debt. A trap inside an explicit subshell does not +# replace the bats binary's own, so a test that fails inside `( ... )` still +# prints its `not ok` -- verified under Bats 1.14.0 against a test-level trap in +# the same file, where the TAP line vanishes. hack/e2e-test-openapi.bats holds +# one: it kills a background `kubectl proxy` from a trap inside a subshell, and +# moving that to the end of the body would leak the process on failure instead of +# suppressing a report. Its declaration says so, and because the ratchet is +# exact, REMOVING that trap fails too -- the count protects the construct rather +# than scheduling it for deletion. +# +# The limits are worth stating rather than discovering. The scan is lexical: a +# signal computed at runtime (`trap cleanup "$sig"`) is invisible, and so is a +# handler whose quoted action spans physical lines without a backslash, since +# only backslash continuations are folded. In the other direction the word is +# counted where no command exists -- inside a heredoc, or in a string a test +# compares against -- which inflates a file's debt rather than hiding one, and is +# why the fixture writers below assemble the keyword and the signal from separate +# arguments. `trap - EXIT`, which DISARMS a handler, scores as an install for the +# same inflating reason. An exact count catches addition and removal but never +# SUBSTITUTION: swap the openapi file's subshell trap for a test-level one and +# the total stays 1, the guard stays green, and that file's "this one is NOT +# debt" note quietly becomes false. +# +# The scan reads .bats and nothing else, so a handler reaching a test body from a +# sourced .sh is invisible to it. Not theoretical: +# hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own +# reason rather than by design -- cozy_capture_tenant_talos is declared with `(` +# and so runs in a subshell, and the diagnostics tests DO call it, so flipping +# its two delimiters to braces reinstates that handler in them with the guard +# green; run_kubernetes_test is declared with `{`, the shape that would bite, and +# is benign only because no @test calls it. +# +# The marker is anchored at column zero. That is NOT what protects the example +# above -- the nested `#` does, and relaxing the anchor leaves this file's own +# audit green. What the anchor buys is that a marker indented inside a function +# body or a heredoc somewhere else is not read as that file's declaration, and a +# file that indents its own reports "declares none" and fails loudly rather than +# passing quietly. Reflowing the example above to column zero would make the +# guard demand a debt of twelve from a file holding nothing. +# +# None of that makes the guard a proof that a file installs no handler; it is a +# ratchet over a known and common spelling. A suite reporting zero failures still +# has to have its `1..N` plan reconciled against its `ok` count, which is the +# check that catches whatever the pattern missed. +# ----------------------------------------------------------------------------- + +# The directory to audit. Under the bats binary that is this file's own +# location; under cozytest.sh, which does not set BATS_TEST_FILENAME, `$0` is +# the runner itself -- and the answer comes out the same only because the runner +# lives in the directory it runs files from. The first test below refuses to +# pass on an empty enumeration, so a wrong answer here fails rather than audits +# nothing. +BATS_DIR="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")" && pwd)" + +# Print a file with comment lines dropped and shell line continuations folded, so +# the count below matches a COMMAND rather than a physical line. `trap 'rm -rf +# "$tmp"' \` with `EXIT` on the next line is a working EXIT trap that no +# single-line pattern sees, and in shell that is where the argument usually sits. +bnet_fold() { + grep -v '^[[:space:]]*#' "$1" \ + | awk '{ line = line $0 + if (line ~ /\\$/) { sub(/\\$/, " ", line); next } + print line; line = "" } + END { if (line != "") print line }' +} + +# What an installed EXIT handler looks like. +# +# `0` as well as `EXIT`: POSIX names the exit condition signal 0, and both shells +# install the identical handler for it, so a pattern that knows only the spelling +# it has seen tests the spelling and not the construct. `EXIT` anywhere in the +# signal list rather than only as its last word: `trap ... EXIT HUP` installs the +# same exit handler plus one more, and anchoring to end-of-line accepts it. A +# quote around the signal, because `trap cleanup 'EXIT'` installs exactly what +# `trap cleanup EXIT` installs -- the quote is punctuation, not a different +# construct. The quote is optional around the SIGNAL only and never after the +# keyword, so `'trap'` as a shell argument stays unmatched; the fixture writers +# below depend on that to keep their own test data from reading as real traps. +# +# The signal ends at any character that cannot continue an identifier, not at +# whitespace. `trap ... EXIT; cd "$tmp"` and `(trap ... EXIT)` are both real +# handlers with punctuation straight after the word, and a terminator demanding +# whitespace or end-of-line scores them zero -- guard green, no declaration +# asked for. Excluding the identifier characters is what keeps `EXIT_CODE` from +# matching, which is the reason the narrower terminator looked right. +# +# The signal matches in either case, because the shell accepts it in either +# case: `trap ... exit` installs the handler `trap ... EXIT` installs, in bash +# and in dash alike. The folding is spelled into the alternation rather than +# running the whole pattern case-insensitively, since `trap` is a builtin name +# and `TRAP ... EXIT` is not a command anyone installs. +# +# The keyword needs its left boundary as much as the signal needs its right +# one, and for the same reason. `bootstrap ` ends in `trap `, so once the +# terminator accepts punctuation, `talosctl bootstrap -n ... ; do sleep 0.5` +# scores as a handler on the `0`. That is loud rather than silent, but not +# harmless: the documented answer to a red guard is to add a debt line, so a +# file would buy a permanent licence for one real trap in order to quiet a line +# that has none. +# +# Both boundaries are therefore spelled the same way -- any character that +# cannot be part of an identifier -- and NOT as whitespace. Whitespace on the +# left drops `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`, which +# are the banned construct behind a semicolon and a paren. The first of those is +# the sharper lesson: the inventory this replaces carried no left boundary at +# all and caught it, so a boundary chosen carelessly here would have narrowed +# coverage while the commit message claimed to widen it. +BNET_TRAP_RE="(^|[^A-Za-z0-9_])trap[[:space:]].*[[:space:]][\"']?([Ee][Xx][Ii][Tt]|0)[\"']?([^A-Za-z0-9_]|\$)" + +# How many EXIT traps a file installs. +# +# `|| true` because grep -c exits 1 on a count of zero, which is the answer for +# most of the tree; without it the caller dies under set -e on the first clean +# file and never reaches the comparison it exists for. +bnet_count() { + bnet_fold "$1" | grep -cE "$BNET_TRAP_RE" || true +} + +# How many lines install more than one EXIT handler. +# +# bnet_count counts matching LINES, so `trap a EXIT; trap b EXIT` counts as one +# and a handler appended to a line that already carries one arrives without +# moving the file's total. Deciding where one command ends and the next begins +# needs a shell parser -- a semicolon inside a handler's own quoted action is not +# a separator -- and getting that wrong UNDERCOUNTS, which is the one direction a +# ratchet cannot afford. So refuse the ambiguous line instead of guessing at it. +# One handler per line is what every file in the tree already does. +bnet_ambiguous() { + bnet_fold "$1" | grep -E "$BNET_TRAP_RE" \ + | awk '{ if (gsub(/(^|[^A-Za-z0-9_])trap[[:space:]]/, "&") > 1) n++ } END { print n + 0 }' +} + +# The part of a file the declaration may live in: the leading comment block, +# ending at the first line that is neither a comment nor blank. +# +# Traps are counted over the whole file; the declaration is read from the header +# alone. A .bats file is shell that writes shell -- fixture writers, heredocs of +# expected output, assertion strings -- and the same line appearing in any of +# those is data belonging to one test, not a statement about the file. Reading it +# there would let an unrelated fixture excuse a real trap, and would do it +# silently, because nothing in that test's own diff looks like a declaration. +# +# The leading comment block rather than everything above the first @test: helper +# functions and their heredocs sit between the two, so stopping at the first +# @test would still read a line out of one. A comment block cannot contain a +# heredoc, which makes the narrower rule the one with no interior. It is also +# where a reviewer opening the file lands, which is the reason the declaration +# moved out of the central inventory in the first place. +bnet_header() { + awk '/^[[:space:]]*#/ || /^[[:space:]]*$/ { print; next } { exit }' "$1" +} + +# Report every file in directory $1 whose traps and declaration disagree, one +# line each, and print nothing when they all agree. +# +# Through stdout rather than an exit status: cozytest.sh rewrites a function's +# closing brace into `return 0`, so a status set in here would be discarded +# before the caller could read it, and the check would pass unconditionally. +bnet_audit() { + find "$1" -name '*.bats' | sort | while IFS= read -r _f; do + [ -e "$_f" ] || continue + # Path relative to the audited root, not the basename: the scan recurses, + # and hack/foo.bats and hack/e2e-apps/foo.bats would otherwise report under + # the same name. + _b=${_f#"$1"/} + _amb=$(bnet_ambiguous "$_f") + if [ "$_amb" -gt 0 ]; then + echo "$_b: $_amb line(s) name the trap keyword more than once; keep one handler per line, and the word out of trailing comments, so the count is exact" + continue + fi + _n=$(bnet_count "$_f") + _decls=$(bnet_header "$_f" | grep -c '^# EXIT-TRAP DEBT:' || true) + if [ "$_decls" -gt 1 ]; then + echo "$_b: $_decls debt declarations; a file carries one or none" + continue + fi + if [ "$_decls" -eq 0 ]; then + if [ "$_n" -ne 0 ]; then + echo "$_b: holds $_n EXIT trap(s) and declares none; clean up at the end of the test body instead, or -- if the line is data rather than a command -- split the keyword from the signal the way the fixtures below do" + fi + continue + fi + _d=$(bnet_header "$_f" | sed -n 's/^# EXIT-TRAP DEBT:[[:space:]]*\([0-9][0-9]*\).*/\1/p') + if [ -z "$_d" ]; then + echo "$_b: the debt declaration names no count" + elif [ "$_d" -eq 0 ]; then + echo "$_b: declares a debt of zero; delete the line instead" + elif [ "$_n" -ne "$_d" ]; then + echo "$_b: declares $_d EXIT trap(s), holds $_n" + fi + done + return 0 +} + +# Fixture writers for the tests below. The word and the signal are assembled from +# separate arguments on purpose: this file is scanned by the guard it defines, so +# a fixture spelled out as a literal would be counted as a real trap in it and +# the guard would demand a declaration covering its own test data. +# +# The same constraint reaches the test titles and the failure messages, which are +# code lines rather than comments and so are read as well. `bnet_add_handler` +# rather than `bnet_add_trap`, and "a handler naming EXIT" rather than "a trap +# naming EXIT", because either of the shorter forms puts the word and the signal +# on one line and is matched. Editing this file means reading its own report +# afterwards; the first test below is the one that says so. +bnet_new_fixture() { + _dir=$1 + printf '%s\n' '#!/usr/bin/env bats' > "$_dir/subject.bats" + return 0 +} + +bnet_add_handler() { + printf "%s 'rm -rf \"\$tmp\"' %s\n" 'trap' "${2:-EXIT}" >> "$1/subject.bats" + return 0 +} + +bnet_add_folded_handler() { + printf "%s 'rm -rf \"\$tmp\"' \\\\\n%s\n" 'trap' 'EXIT' >> "$1/subject.bats" + return 0 +} + +bnet_add_commented_handler() { + printf "# %s 'rm -rf \"\$tmp\"' %s\n" 'trap' 'EXIT' >> "$1/subject.bats" + return 0 +} + +# Put a declaration where a real file puts it: in the leading comment block, just +# under the shebang, whatever the fixture has appended by now. Rebuilt around +# line 1 rather than inserted with `sed -i`, whose append syntax differs between +# the GNU and BSD versions this suite runs under. +bnet_add_debt() { + _f=$1/subject.bats + { head -n 1 "$_f"; printf '%s %s\n' '# EXIT-TRAP DEBT:' "$2"; tail -n +2 "$_f"; } > "$_f.new" + mv "$_f.new" "$_f" + return 0 +} + +# Put a declaration past the header, where it must NOT be honoured. +bnet_append_debt() { + printf '%s %s\n' '# EXIT-TRAP DEBT:' "$2" >> "$1/subject.bats" + return 0 +} + +bnet_add_test_body() { + printf '%s\n' '@test "something" {' ' :' '}' >> "$1/subject.bats" + return 0 +} + +bnet_add_code() { + printf '%s\n' 'BATS_DIR=.' >> "$1/subject.bats" + return 0 +} + +bnet_add_word_containing_trap() { + printf " timeout 60 sh -ec 'until talosctl %s -n 192.0.2.11; do sleep 0.5; done'\n" \ + 'bootstrap' >> "$1/subject.bats" + return 0 +} + +bnet_add_indented_debt() { + printf ' %s %s\n' '# EXIT-TRAP DEBT:' "$2" >> "$1/subject.bats" + return 0 +} + +bnet_add_command_then_handler() { + printf "a=1;%s 'rm -rf \"\$a\"' %s\n" 'trap' 'EXIT' >> "$1/subject.bats" + return 0 +} + +bnet_add_handler_in_parens() { + printf "(%s 'kill 1' %s; true)\n" 'trap' 'EXIT' >> "$1/subject.bats" + return 0 +} + +bnet_add_handler_then_command() { + printf "%s 'rm -rf \"\$a\"' %s; cd /\n" 'trap' 'EXIT' >> "$1/subject.bats" + return 0 +} + +bnet_add_nested_fixture() { + mkdir -p "$1/nested" + printf '%s\n' '#!/usr/bin/env bats' > "$1/nested/deep.bats" + printf "%s 'rm -rf \"\$a\"' %s\n" 'trap' 'EXIT' >> "$1/nested/deep.bats" + return 0 +} + +bnet_add_two_handlers_on_one_line() { + printf "%s 'rm -f \"\$a\"' %s; %s 'rm -f \"\$b\"' %s\n" \ + 'trap' 'EXIT' 'trap' 'EXIT' >> "$1/subject.bats" + return 0 +} + +@test "every bats file under hack holds exactly the EXIT traps it declares" { + # The whole point of the guard, run against the tree it governs. Everything + # below this test exists to prove that this one can fail. + # Counted with the enumeration bnet_audit actually walks, not a second one: + # a backstop that asks `ls` whether the `find` found anything answers about + # the wrong list, and would pass while the audit went vacuously green. That + # is the failure this file exists to talk about, so it may as well not commit + # it. + files=$(find "$BATS_DIR" -name '*.bats' | wc -l) + [ "$files" -gt 0 ] || { echo "FAIL: found no .bats files to audit at all"; false; } + + offences=$(bnet_audit "$BATS_DIR") + if [ -n "$offences" ]; then + echo "FAIL: EXIT-trap cleanup and the declared debt disagree." + printf '%s\n' "$offences" + echo "Clean up at the end of the test body, or correct the file's own" + echo "'# EXIT-TRAP DEBT:' line; see docs/agents/e2e-testing.md." + false + fi +} + +@test "a trap with no declaration is reported" { + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: an undeclared trap went unreported: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a declaration matching the file is accepted" { + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + bnet_add_handler "$tmp" + bnet_add_debt "$tmp" 2 + + out=$(bnet_audit "$tmp") + [ -z "$out" ] || { echo "FAIL: an accurate declaration was reported: $out"; false; } + rm -rf "$tmp" +} + +@test "a declaration below the real count is reported" { + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + bnet_add_handler "$tmp" + bnet_add_debt "$tmp" 1 + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: declares 1 EXIT trap(s), holds 2"*) ;; + *) echo "FAIL: a grown trap count went unreported: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a declaration above the real count is reported" { + # The ratchet is exact rather than a ceiling. A file that sheds a trap has to + # say so, which is what keeps the number from rotting upward and quietly + # licensing traps somebody removed years earlier. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + bnet_add_debt "$tmp" 3 + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: declares 3 EXIT trap(s), holds 1"*) ;; + *) echo "FAIL: a stale declaration went unreported: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a trap folded across a line continuation is counted" { + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_folded_handler "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a trap whose signal sits on the next line went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a trap installed on signal zero is counted" { + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" 0 + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: the POSIX spelling of the exit condition went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a handler naming EXIT ahead of another signal is counted" { + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" 'EXIT HUP' + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a handler listing EXIT before another signal went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a trap inside a comment is not counted" { + # Every converted file explains the ban in prose next to the cleanup it + # replaced. A check that read the explanation as a violation would push people + # to delete the explanation, which is the part that stops the next person + # reinstating the trap. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_commented_handler "$tmp" + + out=$(bnet_audit "$tmp") + [ -z "$out" ] || { echo "FAIL: a trap named in a comment was counted: $out"; false; } + rm -rf "$tmp" +} + +@test "a declared debt of zero is reported" { + # A file that owes nothing says so by carrying no line, not by carrying a line + # that says nothing. Otherwise a fully converted file keeps a declaration that + # reads as an allowance the next reader has no reason to question. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_debt "$tmp" 0 + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: declares a debt of zero"*) ;; + *) echo "FAIL: an empty declaration was accepted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "an indented declaration is not honoured" { + # The marker is anchored at column zero. Not, as an earlier version of this + # comment claimed, to protect the header's own `# EXIT-TRAP DEBT: 12` example + # -- that one is nested behind a second `#` and stays unmatched either way; + # relaxing the anchor leaves this file's audit green, which is how the claim + # was caught. What the anchor buys is that a marker indented inside a function + # body or a heredoc somewhere else is not read as that file's declaration. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_indented_debt "$tmp" 1 + bnet_add_handler "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: an indented declaration was honoured: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a handler preceded by punctuation is counted" { + # The keyword's left boundary is any character that cannot be part of an + # identifier, not whitespace specifically. `tmp=$(mktemp -d);trap ... EXIT` + # and `(trap ... EXIT; true)` both sit straight behind punctuation, and both + # are the banned construct. Requiring a space would be a REGRESSION -- the + # inventory this replaces had no left boundary at all and caught the first of + # them -- so the two boundaries have to be spelled the same way round. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_command_then_handler "$tmp" + bnet_add_handler_in_parens "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 2 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a handler behind punctuation went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a word merely ending in trap is not counted" { + # `bootstrap ` contains `trap `, and once the terminator accepts punctuation + # the `0` in `sleep 0.5` closes the match: a talosctl bootstrap line scores as + # a handler. Loud, but not harmless -- the documented answer to a red guard is + # to add a debt line, so a file would take a permanent licence for one real + # trap to silence a line that has none. The keyword needs its left boundary + # for the same reason the signal needed its right one. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_word_containing_trap "$tmp" + + out=$(bnet_audit "$tmp") + [ -z "$out" ] || { echo "FAIL: a word ending in trap was counted: $out"; false; } + rm -rf "$tmp" +} + +@test "a handler naming the signal in lower case is counted" { + # Signal names are case-insensitive to the shell: `trap ... exit` installs the + # same handler as `trap ... EXIT`, verified in both bash and dash. Only the + # SIGNAL is; `trap` itself is a builtin name and stays lower case, which is + # why the pattern folds case on the signal alternation rather than being run + # case-insensitively end to end. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" exit + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a lower-case signal name went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a handler followed by another command on the same line is counted" { + # The signal does not have to end the line. `trap ... EXIT; cd "$tmp"` and a + # handler wrapped in `( ... )` both put punctuation straight after the word, + # and a terminator that insists on whitespace or end-of-line silently scores + # them zero -- a real handler, no declaration needed, guard green. Same class + # as the quoted signal, one keystroke away. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler_then_command "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a handler followed by a semicolon went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a bats file in a subdirectory is audited too" { + # The ban is on every bats file under hack/, and hack/e2e-apps/ already holds + # two. A flat glob would leave the claim about "every file added later" false + # for anything a directory down. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_nested_fixture "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"nested/deep.bats: holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a nested bats file went unaudited or lost its path: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a declaration naming no count is reported" { + # The parser takes digits. A declaration whose count is a word, or missing, + # parses to nothing, and an unguarded comparison would then treat the file as + # if it had said something. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + bnet_add_debt "$tmp" "twelve" + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: the debt declaration names no count"*) ;; + *) echo "FAIL: a countless declaration was accepted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a handler whose signal is quoted is counted" { + # `trap cleanup 'EXIT'` installs the handler `trap cleanup EXIT` installs. A + # pattern that demands a bare word tests the punctuation rather than the + # construct, and the quoted form is one keystroke away from whoever wants the + # guard to look past them. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" "'EXIT'" + + out=$(bnet_audit "$tmp") + case "$out" in + *"holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a quoted signal went uncounted: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "two handlers sharing one line are reported rather than counted as one" { + # The count is a count of LINES, so a second handler appended to a line that + # already carries one arrives free: the file's total does not move and the + # declaration keeps matching. Rather than guess at command boundaries -- which + # needs a shell parser, and whose failure mode is an UNDERCOUNT, the direction + # this guard cannot afford -- refuse the ambiguous line and say so. One handler + # per line is what the tree already does. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_two_handlers_on_one_line "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: 1 line(s) name the trap keyword more than once"*) ;; + *) echo "FAIL: a doubled-up line was silently counted once: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a declaration below the leading comment block is not honoured" { + # The header ends at the first line of code, not at the first @test. A .bats + # file runs shell before its tests -- helper functions, fixture writers, a + # heredoc of expected output -- and a line inside any of that is data, not a + # statement about the file. Ending the header at the first @test would still + # read it. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_code "$tmp" + bnet_append_debt "$tmp" 1 + bnet_add_handler "$tmp" + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a declaration past the first code line excused a trap: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a declaration below the first test is not honoured" { + # Only the header counts, so that a line the file merely CONTAINS -- inside a + # heredoc, in a fixture a test writes, in an expected-output string -- cannot + # excuse a real trap. Such a line is written to be read as data by the test + # around it, and its author has no idea it is also being read as a promise + # about the file. Narrowing to the header also puts the declaration where a + # reviewer opening the file sees it, which is the whole reason it moved out of + # the central inventory. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + bnet_add_test_body "$tmp" + bnet_append_debt "$tmp" 1 + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: holds 1 EXIT trap(s) and declares none"*) ;; + *) echo "FAIL: a declaration past the first test excused a trap: $out"; false ;; + esac + rm -rf "$tmp" +} + +@test "a second declaration in one file is reported" { + # Two declarations make the file's own statement ambiguous, and a parser that + # silently takes the first would let the losing one drift without ever saying + # which number is in force. + tmp=$(mktemp -d) + bnet_new_fixture "$tmp" + bnet_add_handler "$tmp" + bnet_add_debt "$tmp" 1 + bnet_add_debt "$tmp" 9 + + out=$(bnet_audit "$tmp") + case "$out" in + *"subject.bats: 2 debt declarations"*) ;; + *) echo "FAIL: contradictory declarations were accepted: $out"; false ;; + esac + rm -rf "$tmp" +} diff --git a/hack/build-matrix_test.bats b/hack/build-matrix_test.bats index 718bcb971..6e2e4e075 100644 --- a/hack/build-matrix_test.bats +++ b/hack/build-matrix_test.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 14 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Unit tests for hack/build-matrix.sh — the CI build-matrix selector. # # Run via hack/cozytest.sh from the repo root (make bats-unit-tests); the diff --git a/hack/capture-dataplane.bats b/hack/capture-dataplane.bats index 8430e329e..473b10861 100644 --- a/hack/capture-dataplane.bats +++ b/hack/capture-dataplane.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 1 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # ----------------------------------------------------------------------------- # Unit tests for the pure decision/parsing helpers in # hack/e2e-capture-dataplane.sh -- specifically the LoadBalancer-datapath diff --git a/hack/cozyreport.bats b/hack/cozyreport.bats index 5f576786f..ece3183e7 100644 --- a/hack/cozyreport.bats +++ b/hack/cozyreport.bats @@ -3242,103 +3242,6 @@ STUB rm -rf "$tmp" } -@test "the unconverted bats files hold no more EXIT traps than they already did" { - # The include list above says which files are clean. It says nothing about the - # rest, which makes it a plan rather than a boundary: the six files below could - # each grow a seventh trap and nothing would notice. - # - # So freeze them. A new trap in any of them, or a new .bats file arriving with - # one, moves a number here and fails. Converting one also fails, which is - # correct: that is the moment to move it onto the include list. The class stops - # growing now, and the six are finished when their branches allow, not when - # somebody remembers. - # The counts are expected to move: a new trap landing upstream shifts one, and a - # file that did not exist when this guard was written arrives carrying its own. - # Both have happened, and both times the guard is what noticed rather than a - # reader. Counts are updated rather than the files - # converted: all of them are owned by other branches, and a conflict there costs - # more than an uncovered trap. What the freeze buys is that none of this arrived - # silently. - frozen='build-matrix_test.bats=14 capture-dataplane.bats=1 e2e-test-openapi.bats=1 multus-install-cni-plugins.bats=12 nightly-mirror_test.bats=5 overlay-main-images_test.bats=11 release-changelog-behaviour.bats=9 release-changelog-contract.bats=1 run-kubernetes-talos-diagnostics_test.bats=8 select-e2e_test.bats=15 ' - - found="" - for f in "$HACK_DIR"/*.bats; do - b=$(basename "$f") - case "$b" in - cozyreport.bats | cozyreport-talos.bats | capture-previous-logs.bats \ - | admin-kubeconfig-invariant.bats | check-gpu-operator-variants.bats \ - | check-gpu-recording-rules.bats | check-host-runtime.bats \ - | promote-retag_test.bats | remediation-guard.bats | select-install_test.bats) continue ;; - esac - # `|| true`: grep -c exits 1 on a count of zero, and every already-clean file - # hits that -- under set -e the guard would die on the first one and never - # reach the comparison it exists for. - n=$(fold_source "$f" | grep -cE 'trap[[:space:]].*[[:space:]](EXIT|0)([[:space:]]|$)' || true) - [ "$n" -eq 0 ] || found="$found$b=$n " - done - - if [ "$found" != "$frozen" ]; then - echo "FAIL: the set of unconverted EXIT-trap files changed." - echo " frozen: $frozen" - echo " found: $found" - echo "A new trap here is a new instance of a defect this repo has decided against;" - echo "a converted file belongs on the include list in the test above instead." - false - fi -} - -@test "no bats file on the converted list cleans up through an EXIT trap" { - # docs/agents/e2e-testing.md bans EXIT-trap cleanup in hack/*.bats, and the - # reason is that the violation is invisible: such a test, when it FAILS, prints - # no TAP line at all. Not `not ok`, nothing -- so a reader grepping for `not ok` - # sees a green suite while a test silently did not run to completion. That is - # not hypothetical here: hack/promote-retag_test.bats was showing five passes - # and no failures while six of its eleven tests printed nothing, and converting - # it is what surfaced them. - # - # An INCLUDE list, not a glob with exceptions. Some files still carrying the - # pattern are being edited elsewhere, and one kills a background process from - # its trap rather than removing a directory -- moving that to the end of the - # body would leak the process on failure instead of swallowing the report, which - # trades one defect for another. A glob would therefore need six names carved - # out, and a list of exceptions is the same claim as this list, only written so - # that forgetting to shorten it makes it false. This list is the other - # direction: it grows as files are converted, and until then it says exactly - # what it covers. - # - # Comment lines are stripped before matching. Every one of these files explains - # the ban in prose next to the cleanup it replaced, and a check that counts the - # explanation as a violation would push people to delete the explanation -- - # which is the opposite of what the rule needs, since the reason is the part - # that stops the next person reinstating the trap. Caught exactly that way: this - # guard went red on its own neighbour's new comment. - offenders="" - for f in cozyreport.bats cozyreport-talos.bats capture-previous-logs.bats \ - admin-kubeconfig-invariant.bats check-gpu-operator-variants.bats \ - check-gpu-recording-rules.bats check-host-runtime.bats \ - promote-retag_test.bats remediation-guard.bats select-install_test.bats; do - [ -f "$HACK_DIR/$f" ] || { echo "FAIL: $f is on the list but not in the tree"; false; } - # Folded, not per line: `trap 'rm -rf "$tmp"' \` followed by `EXIT` on the - # next line is a working EXIT trap that no single-line pattern matches, and - # this guard exists precisely to stop that construct reappearing. - # - # `0` as well as `EXIT`: POSIX names the exit condition signal 0, and both - # shells install the identical trap for it. A guard that knows only the - # spelling it has seen tests the spelling, not the construct. - # `EXIT` anywhere in the signal list, not only as its last word: `trap ... EXIT - # HUP` installs the same exit handler and one more, and anchoring to the end - # of the line accepts it. - if fold_source "$HACK_DIR/$f" \ - | grep -Eq 'trap[[:space:]].*[[:space:]](EXIT|0)([[:space:]]|$)'; then - offenders="$offenders $f" - fi - done - if [ -n "$offenders" ]; then - echo "FAIL: EXIT-trap cleanup in:$offenders" - echo "Clean up at the end of the test body instead; see docs/agents/e2e-testing.md." - false - fi -} @test "container logs name the container with -c, the documented form" { tmp=$(mktemp -d) collect_stub_dir "$tmp" diff --git a/hack/e2e-test-openapi.bats b/hack/e2e-test-openapi.bats index 5fb5a2435..490bc011d 100644 --- a/hack/e2e-test-openapi.bats +++ b/hack/e2e-test-openapi.bats @@ -1,4 +1,12 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 1 -- see hack/bats-no-exit-trap.bats. +# This one is NOT debt and must not be converted. The trap sits inside an +# explicit subshell and kills a backgrounded `kubectl proxy`; a subshell trap +# does not replace the one the bats binary installs, so a failure there still +# prints its `not ok`. Moving the kill to the end of the body would leak the +# proxy on failure -- it holds a fixed port, so the next run wedges. The count +# is declared anyway because the ratchet is exact in both directions: it fails +# if a real test-level trap is added here, and it fails if this one is removed. # ----------------------------------------------------------------------------- # Test OpenAPI endpoints in a Kubernetes cluster # ----------------------------------------------------------------------------- diff --git a/hack/multus-install-cni-plugins.bats b/hack/multus-install-cni-plugins.bats index 20408c0a1..e8737d6a8 100644 --- a/hack/multus-install-cni-plugins.bats +++ b/hack/multus-install-cni-plugins.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 12 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Behavioural tests for the multus install-cni-plugins init container script: # extracted from the rendered DaemonSet and run. The helm-unittest cases in # packages/system/multus/tests/multus_test.yaml match its source TEXT instead. diff --git a/hack/nightly-mirror_test.bats b/hack/nightly-mirror_test.bats index ec054e154..50be60db2 100644 --- a/hack/nightly-mirror_test.bats +++ b/hack/nightly-mirror_test.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 5 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Tests for hack/nightly-mirror.sh — the OCIR->GHCR nightly image-mirror selector. # # Guards the ref selection and host rewrite: only cozystack-owned component diff --git a/hack/overlay-main-images_test.bats b/hack/overlay-main-images_test.bats index 6953de9ed..2b6221167 100644 --- a/hack/overlay-main-images_test.bats +++ b/hack/overlay-main-images_test.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 11 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Unit tests for hack/overlay-main-images.sh — the PR-finalize step that points # packages a PR did NOT rebuild at the current-main images from the # cozystack-packages:main artifact. diff --git a/hack/release-changelog-behaviour.bats b/hack/release-changelog-behaviour.bats index 92181e2b5..f20f64103 100644 --- a/hack/release-changelog-behaviour.bats +++ b/hack/release-changelog-behaviour.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 9 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # EXECUTES the release-changelog logic instead of asserting about its YAML. # # Its companion, hack/release-changelog-contract.bats, greps the workflow files. diff --git a/hack/release-changelog-contract.bats b/hack/release-changelog-contract.bats index eecfb8de4..32781d602 100644 --- a/hack/release-changelog-contract.bats +++ b/hack/release-changelog-contract.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 1 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Guards the contract that makes a release publish WITH its changelog. # # The release body is no longer written by hand or synced after the fact: it is diff --git a/hack/run-kubernetes-talos-diagnostics_test.bats b/hack/run-kubernetes-talos-diagnostics_test.bats index 31eec3036..6ab7a1bc1 100644 --- a/hack/run-kubernetes-talos-diagnostics_test.bats +++ b/hack/run-kubernetes-talos-diagnostics_test.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 8 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Regression coverage for tenant-worker Talos diagnostics in # hack/e2e-chainsaw/_lib/run-kubernetes.sh. cozytest.sh ends an @test block at # the first bare closing brace, so command mocks stay at top level. diff --git a/hack/select-e2e_test.bats b/hack/select-e2e_test.bats index 323012714..54e330ca6 100644 --- a/hack/select-e2e_test.bats +++ b/hack/select-e2e_test.bats @@ -1,4 +1,5 @@ #!/usr/bin/env bats +# EXIT-TRAP DEBT: 15 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # ----------------------------------------------------------------------------- # Unit tests for hack/select-e2e.sh # From 6cfeaddf783645510746ff5ca5c70cdbf1c261bf Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Fri, 7 Aug 2026 11:54:03 +0300 Subject: [PATCH 2/2] test(hack): clean up the Talos diagnostics tests without an EXIT trap Eight of the ten tests in this file removed their scratch directory from a `trap ... EXIT`. Such a trap replaces the one the bats binary installs for its own bookkeeping, so a test that fails prints no TAP line at all: not `not ok`, nothing. The run ends with a warning that it executed nine of ten tests, and anyone reading the tail of the output, or grepping it for `not ok`, sees a suite with no failures in it. Break one assertion in this file and the difference is the whole point. Before: `1..10` followed by `# bats warning: Executed 9 instead of expected 10 tests`, with no line naming the test that failed. After: `not ok 2 reader Certificate is short-lived and never embeds credential data`. Remove the directory at the end of each body instead. Both runners set -e, so on failure the removal is unreachable and the directory survives for inspection, which is what a failed test wants anyway. The one test that runs the orchestrator from inside its scratch directory returns to where it started first -- not because rm would refuse, since BSD, busybox and GNU rm all remove a directory from inside it and return 0, but because the runner's own bookkeeping still runs in that subshell after the body returns, and would be doing so on an unlinked cwd. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- ...run-kubernetes-talos-diagnostics_test.bats | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/hack/run-kubernetes-talos-diagnostics_test.bats b/hack/run-kubernetes-talos-diagnostics_test.bats index 6ab7a1bc1..451d1560f 100644 --- a/hack/run-kubernetes-talos-diagnostics_test.bats +++ b/hack/run-kubernetes-talos-diagnostics_test.bats @@ -1,8 +1,16 @@ #!/usr/bin/env bats -# EXIT-TRAP DEBT: 8 -- see hack/bats-no-exit-trap.bats; lower it as the traps go, delete it at zero. # Regression coverage for tenant-worker Talos diagnostics in # hack/e2e-chainsaw/_lib/run-kubernetes.sh. cozytest.sh ends an @test block at # the first bare closing brace, so command mocks stay at top level. +# +# Scratch directories are removed at the end of the test body, never from a +# `trap ... EXIT`. Such a trap replaces the one the bats binary installs for its +# own bookkeeping, and a test that then fails prints no TAP line at all -- not +# `not ok`, nothing -- so a reader grepping the output for failures sees a green +# suite while the test silently did not finish. Both runners set -e, so on +# failure the cleanup below is unreachable and the directory survives for +# inspection, which is what a failed test wants anyway. See +# docs/agents/e2e-testing.md and hack/bats-no-exit-trap.bats. kubectl_calls=/dev/null kubectl_manifest=/dev/null @@ -108,7 +116,6 @@ assert_file_lacks_pattern() { @test "reader Certificate is short-lived and never embeds credential data" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" kubectl_manifest="$tmp/certificate.yaml" @@ -121,12 +128,12 @@ assert_file_lacks_pattern() { assert_file_contains 'name: kubernetes-test-latest-version-talos-ca' "$kubectl_manifest" assert_file_contains 'cozystack-e2e.io/tenant-talos-diagnostics: "test-latest-version"' "$kubectl_manifest" assert_file_lacks_pattern '^[[:space:]]*(tls[.]crt|tls[.]key|data):' "$kubectl_manifest" + rm -rf "$tmp" } @test "talosconfig uses the tenant CA and the issued reader key" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" kubectl_manifest="$tmp/certificate.yaml" talosctl_calls="$tmp/talosctl.calls" @@ -144,12 +151,12 @@ assert_file_lacks_pattern() { -rw-------*) ;; *) echo "talosconfig permissions are not 0600" >&2; return 1 ;; esac + rm -rf "$tmp" } @test "diagnostics Pod has no API token or Secret volume" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" kubectl_manifest="$tmp/pod.yaml" @@ -163,6 +170,7 @@ assert_file_lacks_pattern() { assert_file_contains 'image: docker.io/alpine/k8s:1.36.2@sha256:44ef4942e171939b9c665a4a84beb80e2dcdb9a24330d4651cfdfd2e9deecc47' "$kubectl_manifest" assert_file_contains "'docker.io/alpine/k8s:1.36.2@sha256:44ef4942e171939b9c665a4a84beb80e2dcdb9a24330d4651cfdfd2e9deecc47'" hack/e2e-install-cozystack.bats assert_file_lacks_pattern 'secret:|secretName:' "$kubectl_manifest" + rm -rf "$tmp" } @test "Kubernetes Chainsaw operations leave room for failure diagnostics" { @@ -173,7 +181,6 @@ assert_file_lacks_pattern() { @test "orchestrator skips credentials and helper Pod when every VMI lacks an IP" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" kubectl_manifest="$tmp/manifest.yaml" kubectl_vmi_json="$tmp/vmis.json" @@ -192,12 +199,12 @@ assert_file_lacks_pattern() { assert_file_contains 'default VMI interface has no reported IP address' "$COZY_REPORT_DIR/snapshots/$COZY_SNAPSHOT_NAME/tenant-talos/worker-a/capture-error.log" assert_file_contains 'default VMI interface has no reported IP address' "$COZY_REPORT_DIR/snapshots/$COZY_SNAPSHOT_NAME/tenant-talos/worker-b/capture-error.log" assert_file_contains 'no tenant worker VMI has a reported IP for Talos diagnostics' "$COZY_REPORT_DIR/snapshots/$COZY_SNAPSHOT_NAME/tenant-talos/setup-error.log" + rm -rf "$tmp" } @test "node capture uses the VMI IP for endpoint and node with bounded commands" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" timeout_calls="$tmp/timeout.calls" @@ -214,12 +221,12 @@ assert_file_lacks_pattern() { esac assert_file_contains '[capture exit code: 0]' "$tmp/node/dmesg.log" assert_file_contains '[capture exit code: 0]' "$tmp/node/kubelet.log" + rm -rf "$tmp" } @test "a timed-out worker remains recorded and does not block the next capture" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" timeout_calls="$tmp/timeout.calls" timeout_rc=124 @@ -230,12 +237,13 @@ assert_file_lacks_pattern() { [ "$(wc -l < "$timeout_calls")" -eq 2 ] assert_file_contains '[capture exit code: 124]' "$tmp/node/dmesg.log" assert_file_contains '[capture exit code: 124]' "$tmp/node/kubelet.log" + rm -rf "$tmp" } @test "orchestrator continues from a timed-out worker to the next VMI" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT + origin=$PWD kubectl_calls="$tmp/kubectl.calls" kubectl_manifest="$tmp/manifest.yaml" kubectl_vmi_json="$tmp/vmis.json" @@ -257,16 +265,23 @@ assert_file_lacks_pattern() { assert_file_contains '[capture exit code: 124]' "$COZY_REPORT_DIR/snapshots/$COZY_SNAPSHOT_NAME/tenant-talos/worker-a/kubelet.log" assert_file_contains '[capture exit code: 0]' "$COZY_REPORT_DIR/snapshots/$COZY_SNAPSHOT_NAME/tenant-talos/worker-b/dmesg.log" assert_file_contains '[capture exit code: 0]' "$COZY_REPORT_DIR/snapshots/$COZY_SNAPSHOT_NAME/tenant-talos/worker-b/kubelet.log" + # Back out of the scratch directory before removing it: this test runs the + # orchestrator from inside it. Not because rm would fail -- BSD, busybox and + # GNU rm all remove a directory from inside it and return 0 -- but because + # the runner's own bookkeeping still runs in this subshell after the body + # returns, and it would be doing so on an unlinked cwd. + cd "$origin" + rm -rf "$tmp" } @test "cleanup deletes diagnostic Certificate before its Pod and Secret" { . hack/e2e-chainsaw/_lib/run-kubernetes.sh tmp=$(mktemp -d) - trap 'rm -rf "$tmp"' EXIT kubectl_calls="$tmp/kubectl.calls" cozy_cleanup [ "$(sed -n '2p' "$kubectl_calls")" = '-n tenant-test delete certificates.cert-manager.io -l cozystack-e2e.io/tenant-talos-diagnostics --ignore-not-found --wait=true --timeout=30s' ] [ "$(sed -n '3p' "$kubectl_calls")" = '-n tenant-test delete pod,secret -l cozystack-e2e.io/tenant-talos-diagnostics --ignore-not-found --wait=false' ] + rm -rf "$tmp" }