A bare `!` in a bats body asserts nothing unless it is last: nine here, and no gate anywhere #13

New issue

Closed

opened 2026-09-28 05:32:43 -03:00 by marcos-mendez · 0 comments

marcos-mendez commented

2026-09-28 05:32:43 -03:00

(Migrated from github.com)

Measured 2026-09-28 over every default branch in the organization that carries a bats file (19 of them):

shellcheck -f gcc --shell=bash <file>.bats | grep SC2314

57 inert negations in 8 repositories, beside 45 that are in final position and do assert. shellcheck grades the two apart, error: for the inert ones and note: for the rest. Keel-Linux/tracker#19 has the full table.

Two things are wanted here.

1. Nine inert negations in tests/boot-test-nodes.bats

lib/boot-test-nodes.sh is the node topology this repository lends to every appliance gate, which is why it is tested here rather than copied. Its refusal cases mostly did not run:

lines 20–23: four of the five refused role names (Primary, data_node, node.one, 1node). Only the empty string ran.
lines 30, 31: two of the three refused container name bases (Keel, -leading-dash).
lines 246–248: three of the four things that are not an IPv6 literal — localhost, 127.0.0.1 and fc42::1/64. Only the empty string ran.

A role name becomes part of a container name and part of an environment variable name, and a node base is what LXC is handed as --name, so these are the predicates that keep a malformed topology out of the runner.

2. A gate, so the class cannot come back

test-shell.yml already runs shellcheck, but advisory (-S warning || true) and over a file list that excludes *.bats, so nothing has ever looked at this.

Wanted: a step that runs shellcheck over *.bats with SC2314 at error severity only, and fails the job. Severity error is exactly the inert set; the final-position ones come back as note and must not be failures.

Why here and not in each repository

Every repository with a bats suite already calls test-shell.yml@main: apt, common, inithooks, keel-apache-php, keel-core, keel-mariadb, keel-nodebb, keel-postgresql, keel-redis, keel-transition, keel-wordpress, unit-mariadb, unit-postgresql, unit-redis and this repository itself. One step covers all of them, at once, and cannot drift. The alternative is the same few lines in fourteen workflows, and "copying a test library instead of sharing it" is already written down in docs/traps.md as a mistake this project made and paid for twice.

It should also run before the coverage work and without needing a coverage script: it needs nothing but a checkout, and a suite that is not asserting is not worth measuring.

keel-lamp and keel-lapp have bats suites and no workflows at all; both are clean today, but they will not be covered until they call the gate. Out of scope here.

Merge order

This is @main. The moment it lands, every repository that still has an inert negation goes red. It should merge after the eight repository pull requests in Keel-Linux/tracker#19.

Test plan

The step's script exercised against a repository that still has inert negations and against one that does not.
shellcheck --severity=error --include=SC2314 over tests/*.bats here reports nothing.
actionlint green, including the shellcheck it runs over the new run block.
bats tests/ green and tests/coverage.sh at the committed threshold of 100.

A bats test body has no assertions of its own: its verdict is the exit status of the last command it ran. Bash does not apply errexit to a negated command — that exemption is in the shell by design, so a script can test a command without dying — so `! cmd` on its own line decides the test when it is the final command of the body and is **inert everywhere else**. The line reads as a refutation either way. Measured 2026-09-28 over every default branch in the organization that carries a bats file (19 of them): shellcheck -f gcc --shell=bash <file>.bats | grep SC2314 **57 inert negations in 8 repositories**, beside 45 that are in final position and do assert. shellcheck grades the two apart, `error:` for the inert ones and `note:` for the rest. Keel-Linux/tracker#19 has the full table. Two things are wanted here. ## 1. Nine inert negations in tests/boot-test-nodes.bats `lib/boot-test-nodes.sh` is the node topology this repository lends to every appliance gate, which is why it is tested here rather than copied. Its refusal cases mostly did not run: - lines 20–23: four of the five refused role names (`Primary`, `data_node`, `node.one`, `1node`). Only the empty string ran. - lines 30, 31: two of the three refused container name bases (`Keel`, `-leading-dash`). - lines 246–248: three of the four things that are not an IPv6 literal — `localhost`, `127.0.0.1` and `fc42::1/64`. Only the empty string ran. A role name becomes part of a container name **and** part of an environment variable name, and a node base is what LXC is handed as `--name`, so these are the predicates that keep a malformed topology out of the runner. ## 2. A gate, so the class cannot come back `test-shell.yml` already runs shellcheck, but advisory (`-S warning || true`) and over a file list that excludes `*.bats`, so nothing has ever looked at this. Wanted: a step that runs shellcheck over `*.bats` with **SC2314 at error severity only**, and fails the job. Severity `error` is exactly the inert set; the final-position ones come back as `note` and must not be failures. ### Why here and not in each repository Every repository with a bats suite already calls `test-shell.yml@main`: `apt`, `common`, `inithooks`, `keel-apache-php`, `keel-core`, `keel-mariadb`, `keel-nodebb`, `keel-postgresql`, `keel-redis`, `keel-transition`, `keel-wordpress`, `unit-mariadb`, `unit-postgresql`, `unit-redis` and this repository itself. One step covers all of them, at once, and cannot drift. The alternative is the same few lines in fourteen workflows, and **"copying a test library instead of sharing it"** is already written down in `docs/traps.md` as a mistake this project made and paid for twice. It should also run before the coverage work and without needing a coverage script: it needs nothing but a checkout, and a suite that is not asserting is not worth measuring. `keel-lamp` and `keel-lapp` have bats suites and no workflows at all; both are clean today, but they will not be covered until they call the gate. Out of scope here. ### Merge order This is `@main`. The moment it lands, every repository that still has an inert negation goes red. It should merge **after** the eight repository pull requests in Keel-Linux/tracker#19. ## Test plan - The step's script exercised against a repository that still has inert negations and against one that does not. - `shellcheck --severity=error --include=SC2314` over `tests/*.bats` here reports nothing. - `actionlint` green, including the shellcheck it runs over the new `run` block. - `bats tests/` green and `tests/coverage.sh` at the committed threshold of 100.