test(hack): restore set -u and refresh the cozytest-era headers - #3498
test(hack): restore set -u and refresh the cozytest-era headers#3498myasnikovdaniil wants to merge 1 commit into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
The layer below moved the unit suite to bats(1), which enforces `set -e` but not `set -u`. cozytest.sh ran every test body under `set -eu -x`, so without this a test that reads an unset variable silently sees an empty string and passes. There is no runner-level way to inject it, which is why this needs a per-file load rather than a Makefile flag. Two mechanisms were measured and rejected: a setup() defined in --setup-suite-file does not reach test files at all, and SHELLOPTS=nounset does reach them but is exported, so it also applies to the hack/*.sh scripts the tests exercise as subprocesses -- changing the behaviour of the code under test rather than the tests, and dropping two tests outright. `set -u` inside setup() is a shell option rather than an exported one, so it stays scoped to the test body. hack/test_helper.bash therefore defines the shared setup(), and all 32 unit files load it. The restoration is mutation-checked, not assumed: a canary test reading an unset variable aborts with "unbound variable" with the load in place and passes vacuously without it. Suite is 328/328 green either way, so nothing in the tree was relying on the laxer behaviour. The headers are the other half. 27 of the 32 files documented the cozytest contract in prose -- "there is no bats `run` or `$status`", "setup()/teardown() are not honored", "Run with: hack/cozytest.sh ...", title-sanitization rules from the awk parser, and instructions to keep `}` off column 0 so the parser would not truncate a test. Every one of those statements is now false or vacuous, and a false comment about the harness is worse than none: the next author reads "setup() is not honored" and hand-rolls per-test cleanup that the runner would have done. Where a constraint merely stopped binding rather than reversing, the comment says so instead of deleting the line -- the column-0 heredoc indentation and the manual status capture in migration-seaweedfs-db-adopt are both kept, now marked belt-and-braces. Claims that survived the move are left alone: `! cmd` really is still vacuous under `set -e`, since bash suppresses errexit for a negated command exactly as dash did. hack/seaweedfs-naming-audit.bats gains a note that its subject is POSIX sh executed by /bin/sh but sourced into bats' bash, so shellcheck's shell=sh directive is now the only thing guarding that gap. Refs: #3453 Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Important Review skippedDraft detected. 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 |
6720f96 to
9f875e7
Compare
|
Replaced by #3849, which carries the shim as its first commit. Worth recording why it needed reworking rather than rebasing: the shim was complete when written, 32 files of 32, and is now 32 of 63. The gap is drift rather than a defect, since 27 of the 31 uncovered files postdate this pull request. The cause is that coverage needed a hand-added #3849 makes it structural instead: an audit enumerates the unit files from the filesystem and fails on any that does not load the helper, and cross-checks its own idea of the file set against Closing in favour of it. |
Part 2 of a stack implementing #3453. Stacked on top of the runner switch — review that one first; this diff only makes sense against it.
What
Restores the
set -uthathack/cozytest.shapplied to every test body, and rewrites the 27 file headers that documented the cozytest contract in prose.Why
set -uneeds a per-file loadbats enforces
set -ebut notset -u, so without this a test that reads an unset variable silently sees an empty string and passes — the one strictness property the move to bats would otherwise drop. There is no runner-level way to inject it. Two mechanisms were measured and rejected:setup()in--setup-suite-fileSHELLOPTS=nounsetin the environmenthack/*.shscripts under test — changing the behaviour of the code being tested rather than the tests, and dropping 2 tests outrightset -uinsidesetup()(chosen)hack/test_helper.bashdefines the sharedsetup(); all 32 unit files load it.Verification
The restoration is mutation-checked, not assumed. A canary test reading an unset variable aborts with
unbound variablewith the load in place, and passes vacuously without it:The suite is 328/328 green either way, so nothing in the tree was relying on the laxer behaviour.
make bats-unit-testsand the pre-commit hook both pass.The headers
27 of the 32 files stated things that are now false: "there is no bats
runor$status", "setup()/teardown() are not honored", "Run with: hack/cozytest.sh ...", title-sanitization rules inherited from the awk parser, and instructions to keep}off column 0 so the parser would not truncate a test. A false comment about the harness is worse than no comment — the next author reads "setup() is not honored" and hand-rolls per-test cleanup the runner would have done for them.Where a constraint merely stopped binding rather than reversing, the comment says so instead of vanishing: the column-0 heredoc indentation and the manual status capture in
migration-seaweedfs-db-adopt.batsare both kept and marked belt-and-braces. Claims that survived the move are left alone —! cmdreally is still vacuous underset -e, because bash suppresses errexit for a negated command exactly as dash did.hack/seaweedfs-naming-audit.batsgains a note that its subject is POSIX sh executed by/bin/shbut sourced into bats' bash, so shellcheck'sshell=shdirective is now the only thing guarding that gap.Refs: #3453