Negations that assert, and a gate so the class cannot come back #14

Merged

marcos-mendez merged 3 commits from test/negations-that-assert into main

2026-09-29 01:08:41 -03:00

marcos-mendez commented

2026-09-28 05:33:22 -03:00

(Migrated from github.com)

Nine inert negations here, and the gate that stops the class coming back anywhere.

Closes #13. Part of Keel-Linux/tracker#19, which has the organization-wide sweep.

Merge this last. It is @main, so the moment it lands every repository that still has an inert negation goes red. The eight repository pull requests in Keel-Linux/tracker#19 come first.

Why a bare `!` is inert

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, so ! cmd decides the test in final position and is inert everywhere else. shellcheck grades the two apart, error: for the inert ones and note: for the rest:

shellcheck -f gcc --shell=bash tests/*.bats | grep SC2314

Measured over every default branch in the organization that carries a bats file, 19 of them: 57 inert negations in 8 repositories, beside 45 in final position that do assert.

What was not being checked here

tests/boot-test-nodes.bats, over lib/boot-test-nodes.sh — the node topology this repository lends to every appliance gate:

lines 20–23: four of the five refused role names (Primary, data_node, node.one, 1node). Only "" 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, fc42::1/64. Only "" ran.

A role name becomes part of a container name and part of an environment variable name; a node base is what LXC is handed as --name. All nine are run ! now, with the three that were in final position and did assert, because a line whose meaning depends on its position is the trap itself.

Every predicate was run against every one of its cases by hand before the conversion, and each answers as its test believed. The assertions were dead, not wrong.

The gate

A new step in test-shell.yml, Negations that assert (SC2314):

git ls-files -z '*.bats' \
  | xargs -0 -r shellcheck -f gcc --shell=bash \
      --severity=error --include=SC2314 \
  | tee sc2314.txt || status=$?

--include=SC2314 with --severity=error reports exactly the inert set: the final-position negations come back as note and are filtered out, so a suite that is merely fragile is not a failure. git ls-files also means an untracked second clone inside a working tree is not scanned. Findings go to the step summary and the job fails.

It runs straight after checkout, before the kcov build, and it is not gated on the coverage script existing: it needs only a checkout, and a suite that is not asserting is not worth measuring. It installs shellcheck only if the runner has none.

The existing advisory shellcheck step is left exactly as it is.

Why here rather than 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. One step covers fifteen repositories and cannot drift out of step with itself. The alternative is the same lines in fifteen workflows, and "copying a test library instead of sharing it" is already in docs/traps.md as a mistake this project made and then had to pay for twice. lib/boot-test-nodes.sh lives here for the same reason.

The gap it does not close: keel has a bats suite that it runs from pytest through test-python.yml, so this step does not reach it. It is clean today. (keel-lamp, keel-lapp and tkldev do call test-shell.yml, so they are covered: eighteen repositories with a bats suite, not fifteen.)

Review addendum (6372ac2). The step as first written saw only what shellcheck's SC2314 sees. It now also includes SC2315 (! [[ ... ]]), and a short awk program reports the negations shellcheck cannot see, one level down in a test body: a && ! b, ! cmd in a loop, a branch, a case arm, a group or a substitution, on any line but the body's last. Each of those forms was written into a bats file and run: bats passed all of them and shellcheck reported none. Over every default branch and every open pull request head in the organization, the awk program reports exactly the lines shellcheck reports (57 on the default branches) and nothing more. The step is renamed Negations that assert. Note that this repository's own tests / coverage calls test-shell.yml@main, so the green check on this pull request does not run the new step; it was run locally, extracted from the workflow, against every repository.

profile/WORKFLOWS.md records the step, and docs/traps.md in the handbook gets the entry (separate pull request).

Test plan

The step's script extracted and run against keel-wordpress before its fix — reports its ten findings and exits non-zero — and against this repository after the fix, which exits 0.
The script is shellcheck-clean at -S warning, which is what actionlint runs over a run block.
test-shell.yml parses as YAML.
shellcheck -f gcc --shell=bash --severity=error --include=SC2314 tests/*.bats reports nothing.
bats tests/ green, 55 tests.

tests/coverage.sh: 100.00 percent, threshold 100. Unchanged.

100.00  57/57    bin/require-changelog
100.00  113/113  lib/boot-test-nodes.sh

actionlint on this pull request, which is this repository's own gate.
.github/ ships nothing, so no changelog entry.

Nine inert negations here, and the gate that stops the class coming back anywhere. Closes #13. Part of Keel-Linux/tracker#19, which has the organization-wide sweep. > **Merge this last.** It is `@main`, so the moment it lands every repository that still has an inert negation goes red. The eight repository pull requests in Keel-Linux/tracker#19 come first. ## Why a bare `!` is inert 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, so `! cmd` decides the test in final position and is inert everywhere else. shellcheck grades the two apart, `error:` for the inert ones and `note:` for the rest: shellcheck -f gcc --shell=bash tests/*.bats | grep SC2314 Measured over every default branch in the organization that carries a bats file, 19 of them: **57 inert negations in 8 repositories**, beside 45 in final position that do assert. ## What was not being checked here `tests/boot-test-nodes.bats`, over `lib/boot-test-nodes.sh` — the node topology this repository lends to every appliance gate: - lines 20–23: four of the five refused role names (`Primary`, `data_node`, `node.one`, `1node`). Only `""` 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`, `fc42::1/64`. Only `""` ran. A role name becomes part of a container name **and** part of an environment variable name; a node base is what LXC is handed as `--name`. All nine are `run !` now, with the three that were in final position and did assert, because a line whose meaning depends on its position is the trap itself. Every predicate was run against every one of its cases by hand before the conversion, and each answers as its test believed. The assertions were dead, not wrong. ## The gate A new step in `test-shell.yml`, `Negations that assert (SC2314)`: git ls-files -z '*.bats' \ | xargs -0 -r shellcheck -f gcc --shell=bash \ --severity=error --include=SC2314 \ | tee sc2314.txt || status=$? `--include=SC2314` with `--severity=error` reports **exactly** the inert set: the final-position negations come back as `note` and are filtered out, so a suite that is merely fragile is not a failure. `git ls-files` also means an untracked second clone inside a working tree is not scanned. Findings go to the step summary and the job fails. It runs straight after checkout, before the kcov build, and it is **not** gated on the coverage script existing: it needs only a checkout, and a suite that is not asserting is not worth measuring. It installs shellcheck only if the runner has none. The existing advisory shellcheck step is left exactly as it is. ### Why here rather than 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. One step covers fifteen repositories and cannot drift out of step with itself. The alternative is the same lines in fifteen workflows, and **"copying a test library instead of sharing it"** is already in `docs/traps.md` as a mistake this project made and then had to pay for twice. `lib/boot-test-nodes.sh` lives here for the same reason. The gap it does not close: `keel` has a bats suite that it runs from pytest through `test-python.yml`, so this step does not reach it. It is clean today. (`keel-lamp`, `keel-lapp` and `tkldev` do call `test-shell.yml`, so they are covered: eighteen repositories with a bats suite, not fifteen.) **Review addendum (6372ac2).** The step as first written saw only what shellcheck's SC2314 sees. It now also includes SC2315 (`! [[ ... ]]`), and a short awk program reports the negations shellcheck cannot see, one level down in a test body: `a && ! b`, `! cmd` in a loop, a branch, a case arm, a group or a substitution, on any line but the body's last. Each of those forms was written into a bats file and run: bats passed all of them and shellcheck reported none. Over every default branch and every open pull request head in the organization, the awk program reports exactly the lines shellcheck reports (57 on the default branches) and nothing more. The step is renamed `Negations that assert`. Note that this repository's own `tests / coverage` calls `test-shell.yml@main`, so the green check on this pull request does not run the new step; it was run locally, extracted from the workflow, against every repository. `profile/WORKFLOWS.md` records the step, and `docs/traps.md` in the handbook gets the entry (separate pull request). ## Test plan - [x] The step's script extracted and run against `keel-wordpress` before its fix — reports its ten findings and exits non-zero — and against this repository after the fix, which exits 0. - [x] The script is shellcheck-clean at `-S warning`, which is what `actionlint` runs over a `run` block. - [x] `test-shell.yml` parses as YAML. - [x] `shellcheck -f gcc --shell=bash --severity=error --include=SC2314 tests/*.bats` reports nothing. - [x] `bats tests/` green, 55 tests. - [x] `tests/coverage.sh`: **100.00 percent**, threshold 100. Unchanged. 100.00 57/57 bin/require-changelog 100.00 113/113 lib/boot-test-nodes.sh - [ ] `actionlint` on this pull request, which is this repository's own gate. - [x] `.github/` ships nothing, so no changelog entry.

marcos-mendez commented

2026-09-29 00:45:59 -03:00

(Migrated from github.com)

Review: the gate missed ! [[ ]] (SC2315) and every negation nested in a list, loop, branch or group, which shellcheck does not see and bats passes. 6372ac2 adds both. Across every default branch and open PR head it reports exactly the 57 and nothing else. Also merged main (#12). This repository's own check calls test-shell.yml@main, so it did not run the new step; I ran it locally against all 19 repositories. Merge after the eight repository PRs.

Review: the gate missed `! [[ ]]` (SC2315) and every negation nested in a list, loop, branch or group, which shellcheck does not see and bats passes. 6372ac2 adds both. Across every default branch and open PR head it reports exactly the 57 and nothing else. Also merged main (#12). This repository's own check calls test-shell.yml@main, so it did not run the new step; I ran it locally against all 19 repositories. Merge after the eight repository PRs.