From 2a8eab6e73d61ce3186534f38234c795941775d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcos=20M=C3=A9ndez?= Date: Mon, 28 Sep 2026 08:03:53 +0000 Subject: [PATCH] test: a negated command only asserts in final position `! cmd` on its own line asserts nothing in a bats body unless it happens to be the last command of that body: bash does not apply errexit to a negated command, so the verdict is the exit status of the final command and every earlier `! cmd` is inert. shellcheck names the class SC2314 and grades the two cases apart, error for the inert ones and note for the rest. Six negations here were inert, and all six are about trusting a proxy: - nginx.bats, the shipped conf.d default: it trusts nobody and it never matches geo on `$remote_addr`. Two of the four assertions ran. - nodebb.bats, `nodebb_valid_proxy`: `"2001:db8::13; }"`, the value that would close the geo block and append directives of its own, was the inert one. Only `hello` decided anything. - nodebb.bats, the rendered file: that geo is on `$realip_remote_addr` and never on `$remote_addr`, which is the redirect loop the file's own comment warns about, and that no address means no trust. All of them become `run !`, together with the eight that were in final position and did assert, because a line whose meaning depends on its position is the trap itself. Turning one on found a second bug in the same test. "proxy_conf without an address trusts nobody" asserted `! grep -q set_real_ip_from`, unanchored, against a file whose own comment explains the directive: # that is not a detail. This same file sets set_real_ip_from with So that assertion could never have held, for any input, and nobody could know because it never ran. It is anchored now, `^set_real_ip_from`, which is the property meant and the spelling the positive case at line 167 already used. The rendered file is right; only the assertion was wrong. Two tests also needed the rendered file kept in a local before the refutations: `run` replaces `$output`, so a second `run` reading `<<< "$output"` reads the first refutation's empty output and passes vacuously, which is the same defect in a new shape. Suite green, 134 tests. Coverage unchanged, threshold 95: zz-project-packages 100.00, nodebb.sh 100.00, 40nodebb 96.97, boot-test-lib.sh 100.00, keel-archive-check 100.00. --- tests/hook.bats | 6 ++++-- tests/nginx.bats | 12 +++++++----- tests/nodebb.bats | 26 +++++++++++++++++--------- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/tests/hook.bats b/tests/hook.bats index d54e570..e5576b4 100644 --- a/tests/hook.bats +++ b/tests/hook.bats @@ -4,6 +4,8 @@ # the system (systemctl, redis-cli, nginx, runuser, chown) is a PATH stub # that records its arguments. Nothing here needs root or a network. +bats_require_minimum_version 1.5.0 + setup() { ROOT="$BATS_TEST_DIRNAME/.." HOOK="$ROOT/overlay/usr/lib/inithooks/firstboot.d/40nodebb" @@ -115,7 +117,7 @@ assert c["admin:username"] == "admin", c' "$NODEBB_DIR/config.json" write_conf run "$HOOK" [ "$status" -eq 0 ] - ! grep -q -- '--skip-build' "$CALLS" + run ! grep -q -- '--skip-build' "$CALLS" } @test "the hook is idempotent: a second run changes nothing" { @@ -157,7 +159,7 @@ assert c["admin:username"] == "admin", c' "$NODEBB_DIR/config.json" run "$HOOK" [ "$status" -eq 0 ] grep -q '^geo \$realip_remote_addr \$nodebb_trusted_proxy {$' "$NGINX_PROXY_CONF" - ! grep -q '^set_real_ip_from' "$NGINX_PROXY_CONF" + run ! grep -q '^set_real_ip_from' "$NGINX_PROXY_CONF" } @test "the hook falls back to the hostname when no domain is declared" { diff --git a/tests/nginx.bats b/tests/nginx.bats index 4b1c62e..6c3c996 100644 --- a/tests/nginx.bats +++ b/tests/nginx.bats @@ -4,6 +4,8 @@ # default and the rendered file must not drift apart, since the rendered one # replaces the other and only the rendered one is exercised in production). +bats_require_minimum_version 1.5.0 + setup() { ROOT="$BATS_TEST_DIRNAME/.." LIB="$ROOT/overlay/usr/lib/inithooks/lib/nodebb.sh" @@ -16,8 +18,8 @@ setup() { @test "the shipped conf.d default trusts nobody" { grep -q '^ default 0;$' "$DEFAULT_CONF" - ! grep -q ' 1;$' "$DEFAULT_CONF" - ! grep -q '^set_real_ip_from' "$DEFAULT_CONF" + run ! grep -q ' 1;$' "$DEFAULT_CONF" + run ! grep -q '^set_real_ip_from' "$DEFAULT_CONF" } @test "the shipped conf.d default uses the same geo variable as the rendered file" { @@ -26,8 +28,8 @@ setup() { } @test "the shipped conf.d default never matches geo on remote_addr" { - ! grep -q '^geo \$remote_addr' "$DEFAULT_CONF" - ! grep -q '^geo \$nodebb_trusted_proxy' "$DEFAULT_CONF" + run ! grep -q '^geo \$remote_addr' "$DEFAULT_CONF" + run ! grep -q '^geo \$nodebb_trusted_proxy' "$DEFAULT_CONF" } @test "the shipped conf.d default carries the same map as the rendered file" { @@ -61,7 +63,7 @@ setup() { @test "the shared proxy directives forward the trusted scheme, not the raw one" { grep -q '^proxy_set_header X-Forwarded-Proto \$nodebb_scheme;$' "$INCLUDE" - ! grep -q 'X-Forwarded-Proto \$scheme;' "$INCLUDE" + run ! grep -q 'X-Forwarded-Proto \$scheme;' "$INCLUDE" } @test "the shared proxy directives carry the websocket upgrade headers" { diff --git a/tests/nodebb.bats b/tests/nodebb.bats index 356a7b0..50ef83b 100644 --- a/tests/nodebb.bats +++ b/tests/nodebb.bats @@ -3,6 +3,8 @@ # scratch directories for every path, a PATH stub for redis-cli, python3 # real (it only encodes and decodes JSON here). +bats_require_minimum_version 1.5.0 + setup() { LIB="$BATS_TEST_DIRNAME/../overlay/usr/lib/inithooks/lib/nodebb.sh" # shellcheck source=../overlay/usr/lib/inithooks/lib/nodebb.sh @@ -14,7 +16,7 @@ setup() { @test "needs_setup is true without config.json and false with it" { nodebb_needs_setup "$scratch" touch "$scratch/config.json" - ! nodebb_needs_setup "$scratch" + run ! nodebb_needs_setup "$scratch" } @test "first_value skips empty values and the DEFAULT placeholder" { @@ -114,8 +116,8 @@ assert c == {"url": "https://forum.example.org", "secret": "s", nodebb_valid_proxy 2001:db8::13 nodebb_valid_proxy 2001:db8::/64 nodebb_valid_proxy 192.0.2.7 - ! nodebb_valid_proxy "2001:db8::13; }" - ! nodebb_valid_proxy "hello" + run ! nodebb_valid_proxy "2001:db8::13; }" + run ! nodebb_valid_proxy "hello" } @test "proxy_conf lists the trusted proxy from APP_TRUSTED_PROXY in geo" { @@ -128,12 +130,14 @@ assert c == {"url": "https://forum.example.org", "secret": "s", @test "proxy_conf matches geo on realip_remote_addr, never on remote_addr" { run nodebb_proxy_conf 2001:db8::13 [ "$status" -eq 0 ] - grep -q '^geo \$realip_remote_addr \$nodebb_trusted_proxy {$' <<< "$output" + # every `run` below replaces $output, so keep the rendered file first + local conf=$output + grep -q '^geo \$realip_remote_addr \$nodebb_trusted_proxy {$' <<< "$conf" # the regression this guards: set_real_ip_from below has already # rewritten $remote_addr by the time geo is evaluated, so a geo block on # $remote_addr never matches the proxy and port 80 answers 307 forever - ! grep -q '^geo \$remote_addr' <<< "$output" - ! grep -q '^geo \$nodebb_trusted_proxy' <<< "$output" + run ! grep -q '^geo \$remote_addr' <<< "$conf" + run ! grep -q '^geo \$nodebb_trusted_proxy' <<< "$conf" } @test "proxy_conf keeps the geo variable the same when nobody is trusted" { @@ -174,9 +178,13 @@ assert c == {"url": "https://forum.example.org", "secret": "s", @test "proxy_conf without an address trusts nobody" { run nodebb_proxy_conf "" [ "$status" -eq 0 ] - ! grep -q ' 1;$' <<< "$output" - ! grep -q set_real_ip_from <<< "$output" - grep -q 'default 0;' <<< "$output" + # every `run` below replaces $output, so keep the rendered file first + local conf=$output + run ! grep -q ' 1;$' <<< "$conf" + # anchored: the comment this file carries names the directive, so an + # unanchored match finds the explanation and never the directive + run ! grep -q '^set_real_ip_from' <<< "$conf" + grep -q '^ default 0;$' <<< "$conf" } @test "proxy_conf rejects an address nginx would not accept" {