test(tests): make negated BATS assertions able to fail - #3790

What this PR does

Seventy-seven assertions across seven BATS suites begin with !. Seventy-two of them pass no matter what they find. POSIX exempts a command whose return value is inverted with ! from set -e, so the failing status is dropped where it is produced, and hack/cozytest.sh closes each test function with return 0, so it never resurfaces as the test's own status either. The other five are followed by || { ... exit 1; }, which reads the inverted status and does work; they are rewritten too, because telling the two apart needs the tail of every statement parsed, and a guard that guesses wrong there is green on a real one.

Switching to the bats binary does not rescue the seventy-two, which is the part worth knowing before reviewing this. Its ERR trap carries the identical exemption: bash -c 'set -eE; trap "echo TRAP" ERR; ! true; echo REACHED' prints REACHED, runs no trap and exits zero. The only negated assertion bats reports is one standing last in the test body, where the inverted status happens to be what the function returns. One of the seventy-two stood last; seventy-one did not.

What was lost is specific rather than diffuse. ! is how you spell "this must not appear", and an absence is what no other assertion in a test covers. The affected suites assert that a migration ran nothing destructive, that no image was copied, that a flag was not passed, that an object does not exist. On that half of their contract they held for any input.

Each site is now written as if cmd; then echo "FAIL: ..."; false; fi, the form already used in the files where this had been noticed one at a time, and each carries a message naming what must not have happened. hack/bats-no-negated-assert.bats scans every hack/**/*.bats and refuses the old form, so a new one cannot be added silently. Inverted conditions (if !, while !, until !) are untouched, because set -e is not what decides a condition. [ ! -f x ] is untouched for a different reason and remains an ordinary failing assertion: the ! there is an operator of test, not negation of a command, so nothing exempts its status.

The guard pins its own premise instead of describing it, on both sides. One test asserts the runner still passes a bare negated assertion; a second, that neither set -e nor the ERR trap sees an inverted status; a third, that the prescribed if cmd; then ...; false; fi does fail, under both runners, and at the form rather than somewhere before it, since a run that died early would satisfy a bare status check and pin nothing.

The third is the one every rewrite here rests on, and its failure mode is the reason it exists rather than being left implied. Were false inside a then body ever to stop propagating, the first test would still pass, because a negated assertion would still be vacuous, while all seventy-seven converted absences went silently green. A ban that prescribes a replacement owes an observer on the replacement, or it is the defect it removes, one level up.

Every claim the guard makes is pinned by removing the thing it claims, because on this file reading was repeatedly not enough. The recursion is exercised through the audit function against a fixture in a subdirectory, not through a second copy of its find expression: a copy keeps passing while the function loses its recursion, and an audit that returns empty reads as a clean tree. hack/e2e-apps/ is what makes recursion matter, since BATS_UNIT_FILES is a non-recursive wildcard and that directory is the one place make unit-tests never reaches. It is not what the test pins against, though, because it is emptying as suites move to Chainsaw and would eventually fail the test for a reason unrelated to recursion.

The rule's surface was checked in the other direction too. The same shape appears nowhere in hack/**/*.sh (including the libraries bats files source) or in the Chainsaw script: steps, so .bats is the whole surface rather than the part that was convenient to guard. Both zeros were taken twice, line-by-line and with continuations folded, because a line-oriented search cannot see a statement split across a continuation. The first attempt at the folded pass disagreed until its if-context filter was restored, which is why the answer is stated as measured twice rather than as measured.

The scan is lexical, so its header names the statement boundaries it owns rather than implying it understands shell: ; & && || { (, the ) closing a case pattern, and the words then do else, each measured against the runner before being added. A keyword may legally be preceded only by whitespace or by ;, since every other separator in front of then, do or else is a shell syntax error, checked with sh -n rather than recalled. The scan therefore admits exactly those two, and that part of the class is closed rather than patched one spelling at a time. All three keywords are vacuous after either spelling, so missing one is a silent miss, the single direction this scan must not err in, given that its whole design trades noise for safety.

Four over-counts remain and are deliberate, each named in the header: ( ! cmd ) does propagate its inverted status and is reported anyway; so would a ! opening a statement after a redirection &; x=$(true || ! false) is reported although the || reads the inverted status; and a condition continued by a trailing && instead of a backslash is reported, because the strip only recognises a condition whose end it can see. Each costs one rewritten line, where the opposite error costs a silent pass. They are enumerated rather than engineered away on purpose, because a removed error leaves no trace while a listed one tells the next reader what they are looking at. For the same reason, the one place the scan errs quiet is named too: a file cut off midway through a line continuation loses its last buffered statement. That input is not a runnable test, since sh -n rejects it with unexpected end of file, so nothing executable can hide there. The header still claims the scan errs loud or not at all, and this is the exception to that claim rather than to its coverage. The test naming the shapes claims only what its fixture holds. A lexical scan over shell cannot promise it has enumerated the language, and a name that promised it would be false whether or not a gap existed today.

The branch name is narrower than the subject and I am leaving it as it is rather than churn it. This is not a defect in hack/cozytest.sh that moving to the bats binary would fix. It is a POSIX rule both runners inherit, and moving to bats would close one of the seventy-two sites.

One suite went red for real once its assertions could fail. seaweedfs-guard-parity forbids four mutable discriminators by grepping each chart, and both charts carry a template comment block arguing why those signals were rejected, so the grep matched the argument and would have reported a chart for documenting the rule it follows. The charts are correct; the assertion was not. It now strips {{/* ... */}} before matching. Two details there are load-bearing and neither is obvious. The strip removes the comment span, not the line that opens it: dropping the line is shorter and silently loses a discriminator sharing a line with a comment, which is a chart classifying on a forbidden signal and a test reporting that it does not. And the delimiters are checked to balance first, because every {{/* is read as an opener, so one inside a quoted string (these charts carry long fail (printf ...) messages) would swallow the rest of the file and leave the checks reading nothing.

A last commit widens six comments that explained this trap as applying to a "negated pipeline". It is not about pipelines, and the narrower wording invites the conclusion that the other shapes are safe. Comment text only; each of those files already uses the working form.

Verification

make bats-unit-tests passes 556 tests and then stops at migration-seaweedfs-db-adopt, so the suites ordered after it were run in a second pass: 387 more, 943 green in total, measured in a worktree separate from the one being edited.

Three suites are red on this macOS host and each one reproduces at the merge base, so none is this branch's. migration-seaweedfs-db-adopt: its container cannot write into the bind-mounted temp directory. seaweedfs-naming-audit: fails the same test with the same counts at the merge base. release-changelog-behaviour: gated on command -v act and docker info, it skipped silently while this host had no Docker running, and began failing once Docker came up, on an act incompatibility with the Colima socket path. That last red is a property of the host having Docker at all, which is worth knowing before reading a local run as a regression.

Each suite whose assertions changed and that runs on this host was measured before the edit and matches that baseline now. The six suites that received only a reworded comment were run after the change and not before, which is the weaker claim and the true one, since nothing executable in them was touched.

migration-seaweedfs-db-adopt was then run on Linux, where it passes 15 of 15. Its nine rewritten sites also carry a two-sided check, because a suite that goes green proves nothing on its own: with the pattern swapped for one the same test proves is present, the rewritten form fails the suite and the original ! form passes it. Same file, same test, same pattern, same runner, only the spelling differs.

Two sites are still unverified and I would rather say so than imply otherwise: both are in e2e-apps/monitoring-oidc-customconfig, which needs a live cluster. It will not get one soon either, because the directory holding it is wired into no workflow, no Makefile target and no suite selector, a leftover of the move to Chainsaw that is filed as #3786. Those two assertions were vacuous before this change and are real now, so whichever way that issue is resolved, they stop lying in the meantime.

Screenshots

Not a UI change.

Downstream repositories

Walked the trigger map against the diff file by file. The change touches seven BATS suites, adds one, and adds a convention to docs/agents/e2e-testing.md. It does not touch hack/e2e-prepare-cluster.bats, which is the node contract ansible-cozystack and talm restate; it moves and renames nothing under hack/ and changes no make target's behaviour, which is what ccp gates on; and it touches no package, schema, enum, default, CRD, label, annotation or metric.

• No downstream repository is affected by this change
• cozystack/website - follow-up:
• cozystack/terraform-provider-cozystack - follow-up:
• cozystack/ansible-cozystack - follow-up:
• cozystack/ccp - follow-up:
• cozystack/talm - follow-up:
• cozystack/cozyhr - follow-up:
• cozystack/cozy-proxy - follow-up:
• cozystack/cozystack-telemetry-server - follow-up:
• cozystack/external-apps-example - follow-up:
• cozystack/examples - follow-up:

Release note

NONE

Summary by CodeRabbit

• Documentation

  • Added guidance prohibiting negated Bats assertions that can mask test failures.
  • Documented the required explicit conditional failure pattern and updated the reviewer checklist.

• Tests

  • Added automated checks to detect unsafe negated assertions in end-to-end tests.
  • Updated existing tests with clearer failure messages and more reliable conditional checks.
  • Expanded Helm template comment handling coverage in SeaweedFS guard tests.

Closes #3565