test(hack): run the unit suite under bats and make coverage structural - #3849
test(hack): run the unit suite under bats and make coverage structural#3849myasnikovdaniil wants to merge 2 commits into
Conversation
hack/test_helper.bash restores the `set -u` that hack/cozytest.sh applied to every test body and that bats(1) does not, and it is loaded per file because bats offers no other route. Its default `setup()` is defined in bats-core's own test_functions.bash and sourced before the test file, so nothing outside the file can replace it; `setup_suite` runs once in bats-exec-suite's process and never in the process that runs a test; and SHELLOPTS and BASH_ENV, the two environment routes that do cross a process boundary, are exported by definition and so reach the hack/*.sh scripts under test as well. A per-file line is exactly the kind of requirement that rots. The helper landed covering 32 of 32 unit files. It now has to cover 66, and 33 of the 34 it was missing were added after it -- not because anyone removed a line, but because coverage was something an author had to remember and nothing anywhere held the list of files that were supposed to have one. An uncovered file still runs and still passes; its tests are simply weaker than they read. So the record is hack/bats-strict-setup.bats, and its input is the tree rather than a list. It walks hack/*.bats minus hack/e2e-*.bats and fails on any file that does not load the helper, so a file added next month is in the audit's input the moment it exists and the gap closes at the commit that opens it. It does not keep its own copy of that filter either: a test asks make to print $(BATS_UNIT_FILES) and diffs it against what the audit enumerated, so the set that is audited is provably the set that runs. It also refuses a file that loads the helper and then defines its own `setup()` without calling `strict_setup`, which is the second way the strictness goes missing silently. Mutation-checked rather than assumed, both halves: a canary test reading an unset variable aborts with "unbound variable" with the load in place and passes vacuously without it, and deleting one real file's load line turns the audit red naming that file. hack/cozytest.sh grows a `load`, because the line now sits in files that runner still executes and would otherwise die on `load: not found` under `set -e`. It is compatibility only -- cozytest already runs each body under `set -eu -x` -- and it keeps the choice of runner the Makefile's rather than something each test file has to be written for. Refs: #3453 Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
hack/cozytest.sh serves two populations that want opposite things from a runner: 66 hermetic unit files covering hack/*.sh, the workflows and the packages tree, and 3 live-cluster imperative suites. This moves the unit half to real bats and leaves the live half where it is; the hack/e2e-%.bats filter already in BATS_UNIT_FILES is what keeps them apart. The unit files need no edits beyond the `load test_helper` the layer below added. They pass under vanilla bats unchanged because their authors wrote to the intersection of both runners -- no run, no $status, no skip, repo-root-relative paths. What this buys, none of which cozytest offers: --filter-status failed to rerun only the previous failures, JUnit so CI annotates the failing test instead of requiring a log read, -j, and deletion of a bespoke awk parser whose rewrite rules void helper exit codes, corrupt heredoc fixtures on a column-0 brace, and collide test names after sanitization. WHY THIS COULD NOT LAND FIRST. Under bats an EXIT handler inside a test body replaces the one the binary installs for its own bookkeeping, and a test that then fails prints no TAP line at all -- not `not ok`, nothing. The suite would have gone green and stayed green while real failures vanished. The 67 test-level handlers are gone as of the commits below this one, and that this closed the hole is measured rather than assumed: under Bats 1.14.0 all 66 files report a plan equal to their declared @test count and execute all of it -- 1161 declared, 1161 planned, 1161 executed, 0 failures, and no `bats warning: Executed N instead of expected M` from any file. An exit code would have answered whether anything failed and never whether anything ran. The same 66 files under hack/cozytest.sh execute 1116 of the 1161. The gap is one pre-existing failure, hack/ghcr-mirror_test.bats, byte-identical to origin/main here apart from its load line: it fails at its 20th test under cozytest, which ends that file, and passes 65 of 65 under bats. Not caused by this change and not addressed by it. Timing, measured on this tree on 8 cores: 367s of work, 151s wall under `bats -j`, against 312s serial under cozytest. The wall time is one file rather than the runner -- hack/run-kubernetes-node-join_test.bats spends 138s of it in real sleeps -- which is worth knowing before reading the parallel speedup as the ceiling. Two consequences worth naming, neither covered by the issue: bats is a new dependency. cozytest is pure bash+awk, so no runner image or contributor machine has ever needed anything else. The checks job installs it from a pinned tag rather than the distro package, whose version varies by base image, and the pin is the version the suite was verified against. The make target says so plainly when bats is absent. GNU parallel stays optional -- BATS_JOBS resolves to 1 without it, since `bats -j` exits non-zero rather than degrading. The pre-commit lane is the second consumer and does NOT get bats. That job lints changed YAML and Markdown on a runner with no bats, and the new hook's path filter covers .github/workflows/, so a workflow edit would drag the whole shell suite into a lint lane and duplicate the checks job that already runs `make unit-tests`. It is skipped there by id, keeping the hook a local-developer gate -- one that costs 151s, which the hook's own comment says rather than implies. The tested shell changes. cozytest is #!/bin/sh, which is dash on the CI runner, and bats runs bash. The POSIX scripts that tests source rather than execute therefore lose a runtime dash check they had by accident, while the shell=bash files under e2e-chainsaw/_lib gain correct fidelity for the same reason. The static replacement is the shellcheck gate in item 5 of #3453, which does not exist yet. Refs: #3453 Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM. The migration is well built and I could not fault the mechanism, but it leaves a dozen file headers asserting the opposite of what it just enabled, in the same files it edits.
Business context: move the hack unit suite off the home-grown runner onto bats(1), and replace the remembered set -u helper convention with an audit that walks the tree.
What I verified rather than assumed:
set -uset insetup()really does reach the test body under Bats 1.14.0. Wrote a throwaway file that reads an unset variable in the body and it aborts withunbound variable, sohack/test_helper.bashdoes what it claims.- bats runs a test with the working directory it was invoked from, not the test file's directory. That matters because a large share of these files resolve repo-relative paths against the cwd, and the Makefile invokes bats from the repo root, so they keep working.
- All 66 unit files carry
load test_helperand none defines its ownsetup(), sohack/bats-strict-setup.batshas nothing to catch today. BATS_JOBSdegrades correctly. On a box without GNU parallel it resolves to 1 instead of handing bats a flag it cannot honour.- Full suite under the new target: 1161 tests planned, 1146
ok, 15not ok, zero bats warnings, and all 66 files executed. Every one of those 15 fails identically at the base commit (14 inhack/migration-seaweedfs-db-adopt.bats, plus test 25 ofhack/seaweedfs-naming-audit.bats), so this PR introduces no new failures. Both look platform-specific to macOS.
Worth stating because the description does not: the old target ran hack/cozytest.sh "$$f" || exit 1 inside a single recipe loop, so it stopped at the first red file and everything sorting after it never ran. On this machine that hid 23 files. Handing the whole list to one bats invocation is what fixes that, and it is a bigger win than the runner swap itself.
Blockers
B1: twelve headers now say setup() is never invoked, directly above the line that installs one
hack/build-matrix_test.bats:6 reads "A bats setup() hook would be dead here, cozytest never invokes it, so the repo-root cwd is supplied by the runner rather than a setup() cd." Line 18 of the same file is the load test_helper this PR adds, and that helper exists to define a setup() which the new runner does invoke.
hack/select-e2e_test.bats:8 is the same shape ("setup()/teardown() are not honored"), and so are hack/cozystack-version-stamp.bats, hack/etcd-probe_test.bats, hack/ghcr-mirror_test.bats, hack/helm-unit-tests.bats, hack/pod-label-census_test.bats, hack/remediation-guard.bats, hack/run-kubernetes-drain_test.bats, hack/run-kubernetes-schedulable_test.bats, hack/select-install_test.bats and hack/talos-image-cache_test.bats. Several of the same headers also say "there is no bats run or $status", which stops being true for the runner CI now uses.
This is the failure mode hack/bats-strict-setup.bats was written against, in its own words: a requirement whose only record is the diff that introduced it. A reader who opens one of these files to add a test is told not to use the hook the file three lines later depends on.
B2: cozytest.sh is the CI path stops being true, and the convention doc still argues from it
hack/cozyreport.bats:33 says "cozytest.sh is the CI path." This PR edits that file (the load goes in at line 36) and makes the sentence false.
docs/agents/e2e-testing.md:48 is the bigger one. It reads "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." That sentence is the doc's reason the vanished-TAP-line hazard was survivable: the runner CI used reported the failure. After this PR the unit suite runs under bats in CI, so for those 66 files the hazard is the default rather than the theoretical case, and the paragraph still describes the old arrangement. The PR touches no markdown at all.
Non-blocking follow-ups
- The pre-commit filter does not cover what its own comment says it covers. The comment calls it "the union of what the suite reads" and argues that narrowing it "would let exactly the regressions they exist to catch through", but
^(hack/|Makefile$|\.github/workflows/|packages/|\.claude/hooks/)at.pre-commit-config.yaml:50matches neither.github/labels.yml, whichhack/issue-triage-contract.bats:37asserts over, nordocs/agents/contributing.md, whichhack/downstream-trigger-map.bats:40asserts over. Commits touching only those skip the hook. CI still runs the suite unconditionally so nothing reaches main, which is why this is not a blocker, but the claim is wider than the regex. - The 151s figure in the same comment is the eight-core GNU parallel case. Without GNU parallel
BATS_JOBSresolves to 1, and parallel is not present by default on macOS. Serial here took 9m52s. The Makefile documents the fallback; the hook comment quotes only the fast number, which is the one a developer picking whether to keep the hook will read.
E2E is red, same install-step failure as the PR underneath. Nothing on that path runs a hack/*.bats unit file and the unit job is green, so I do not read it as this diff.
|
Sizing correction for my first blocker. I quoted twelve headers, and the population is bigger than that. On Not every one of those sentences goes false, but every one needs reading, and the twelve I listed are only where the contradiction is immediate. Sized off my list, the fix leaves most of the population behind.
|
Second of two, on top of #3848, and together they replace #3497 and #3498.
hack/cozytest.shis ours and shares nothing with bats but the file format. The intent is that it keeps doing what it was written for, the live-cluster sandbox work, and the script unit tests move to the real thing. After this the only callers left are three, all sandbox install steps:e2e-prepare-cluster.bats,e2e-install-cozystack.batsande2e-test-openapi.bats. The split is drawn by prefix in the Makefile,BATS_UNIT_FILES := $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats)), so a new unit file joins the bats lane by existing rather than by being remembered.The coverage mechanism, first commit
#3498's shim was complete when written, 32 of 32 files. It is now 32 of 63, and the gap is drift rather than a defect: 27 of the 31 uncovered files postdate it. The cause is that coverage needed a hand-added
load test_helperper file, so every new file was uncovered by construction and nobody noticed.hack/bats-strict-setup.batsmakes it structural. It enumerateshack/*.batsminushack/e2e-*.batsfrom the filesystem and fails on any file that does not load the helper, or that defines asetup()skipping strict setup. There is no list to maintain, so it cannot drift the way the shim did, and it does not keep its own copy of the filter either: one test asksmake print-bats-unit-fileswhat actually runs and diffs that against what it enumerated, so the audited set is provably the set that runs.Both halves are mutation proven. A canary aborts on
unbound variablewith the load present and passes vacuously without it; deleting one real load line turns the audit red and names the file.Bats offers no runner-level hook for this. Its default
setup()is sourced before the test file,setup_suiteruns in another process, andSHELLOPTS/BASH_ENVare exported and leak into the scripts under test, so an audited per-file line is the only shape available.hack/cozytest.shgained the same load so this commit stands alone if the flip is dropped.The flip, second commit
66 unit files, 1161 declared tests. Under
bats1.14.0: 1161 planned, 1161 executed, 0 failures. Under cozytest: 1116 executed, 1 failure. Zerobats warningfrom any file, per file and in the full run, which is what verifies #3848 rather than assuming it.That single cozytest failure is
hack/ghcr-mirror_test.bats, which passes 65 of 65 under bats and fails only under cozytest at test 20, ending that file and accounting for the whole 45 test gap. The test doesout=$(ghcr_mirror_diagnose 2>&1)and under cozytest the runner's ownset -xtrace lands in that capture. It is left alone: the flip is what fixes it.Two judgement calls worth naming
The pre-commit hook claimed "~26s on 8 cores". Measured on this tree it is 367s of work, 151s wall under
bats -j 8against 312s serial under cozytest, dominated byhack/run-kubernetes-node-join_test.batsalone at 138s of real sleeps. The hook is kept, the claim is replaced with the measured numbers, andSKIP=bats-unit-testsis named as the escape hatch. Shipping a six times wrong justification seemed worse than either keeping or dropping the hook.The CI
BATS_VERSIONpin moves from v1.13.0 to v1.14.0, the version everything above was verified under.#3498's refresh of 27 file headers is not here. 58 non-e2e files mention cozytest in prose and rewriting them is a separate, purely editorial commit.
Release note