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 9be41a4a2..c27c3a4bc 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 @@ -148,7 +148,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 9c7c9d940..c5a7d3418 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..451d1560f 100644 --- a/hack/run-kubernetes-talos-diagnostics_test.bats +++ b/hack/run-kubernetes-talos-diagnostics_test.bats @@ -2,6 +2,15 @@ # 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 @@ -107,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" @@ -120,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" @@ -143,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" @@ -162,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" { @@ -172,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" @@ -191,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" @@ -213,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 @@ -229,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" @@ -256,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" } diff --git a/hack/select-e2e_test.bats b/hack/select-e2e_test.bats index cb896b526..dca7b2847 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 #