[tools][test][e2e] Make the bats suite fail on assertions that stop holding - #1043
Open
weiqingy wants to merge 5 commits into
Open
[tools][test][e2e] Make the bats suite fail on assertions that stop holding#1043weiqingy wants to merge 5 commits into
weiqingy wants to merge 5 commits into
Conversation
The launcher refuses to run under a bash that cannot fail a `[[ ]]` assertion, but it tested the major version only and so admitted bash 4.0. The `set -e` and ERR trap handling of `[[ ]]` changed in 4.1, not 4.0, so 4.0 has the same defect the check exists to catch. Test the major and minor version together, and correct the two places that stated the old figure. The empty-BASH_VERSION test stays first in the condition. Shells with no BASH_VERSINFO rely on that short-circuit; reversing the operands makes dash fail with `cannot open 41: No such file` instead of printing the intended message. Generated-by: Claude Code 2.1.240 (Claude Opus 5)
On bash before 4.1, errexit does not apply to `[[ ]]`, so a `[[ ]]` assertion that is not the last command of a test body cannot fail the test. It reports ok whether or not the condition holds. 33 of the assertions in this suite were in that position. Append `|| false` to every bare `[[ ]]` assertion, chaining a simple command that errexit does honour on every version. This is the form bats-core documents and the one already used in the checkpoint recovery tests. All 83 are converted, not only the 33 that are currently vacuous. The other 50 work today only because they happen to be the last statement of their block, which a later edit can silently change. Converting them all makes the rule checkable: a bare `[[ ]]` at statement position is now always a defect, with two deliberate exceptions where the construct is a redefined stub's return value rather than an assertion. Generated-by: Claude Code 2.1.240 (Claude Opus 5)
edit_plan_quote escapes embedded single quotes so a value can be sourced back from the dumped state file. It did that with an inline replacement, which 3.2.57 and 5.3.15 do not treat alike: 3.2 emits 'it\'\\'\'s' where the intended output is 'it'\''s'. That is not valid shell, so sourcing fails with an unexpected EOF and the value is lost. install.sh declares macOS support and is fetched and piped to bash, and macOS resolves bash to /bin/bash 3.2.57, so the plan-edit feature corrupts its own state file for those users. Supply the escape through a variable, which both interpreters leave untouched. normalize_path hit the same parameter-expansion quirk and avoids it with sed; a variable is used here instead because sed needs a command substitution, and that strips a trailing newline the round-trip has to preserve. The regression test runs the function under a real bash 3.x and skips when none is present, because on a newer interpreter the unfixed form is already correct and the test would pass either way. Its input carries two adjacent quotes, a later lone quote, and a $x, so an escape that handles only the first occurrence, only adjacent pairs, or wraps in double quotes is caught. The six existing tests miss three of those. setup() skips load_install_sh for that test: sourcing install.sh replaces the EXIT trap bats' skip relies on. Generated-by: Claude Code 2.1.240 (Claude Opus 5)
The bash check in run.sh binds the shell running run.sh. bats evaluates each test body in a separate process started through `#!/usr/bin/env bash`, so the interpreter is re-resolved from PATH at every hop, and on a stock mac that is /bin/bash 3.2. The check passed while the bodies ran under the interpreter it exists to reject, and following its own advice to re-run under a newer bash left PATH untouched and did nothing. Put a `bash` symlink to the accepted interpreter ahead of everything else on PATH, and re-check the version from inside the run through a setup_suite file, so a pin that stops resolving fails the suite instead of quietly reverting to the old behaviour. PATH is the only lever bats offers: there is no flag for it, and BASH in the environment is ignored. The link is built under a temp name and renamed into place. Renaming replaces the name in one step, where `ln -sfn` unlinks first and leaves a window in which a concurrent lookup finds nothing and falls through to the next PATH entry. The pin also re-interprets the scripts under test, which carry the same shebang, so the suite no longer exercises them under the bash a developer happens to have. That is recorded next to the pin. Generated-by: Claude Code 2.1.240 (Claude Opus 5)
remove_submission_pid rebuilds SUBMISSION_PIDS from a local array. When
it removes the last pid that array is empty, and expanding "${arr[@]}"
on an empty array under `set -u` is an unbound-variable error on bash
4.3 and older, so the function dies along with the test driving it. The
loop just above already guards its own expansion; this one was missed.
The bug predates this branch and behaves the same on the merge base. CI
does not see it, since both legs run bash 5. It is fixed here because
the branch states a supported floor of bash 4.1, and leaving it would
name a floor that one of the suite's own tests does not pass on for
anyone whose bash is in [4.1, 4.4).
Rebuild with the alternate-value form rather than `:-`, which would
yield a single empty-string element instead of an empty array and breaks
the same test on every version, including those that pass today.
Verified on 3.2.57, 4.0.44, 4.1.17, 4.2.53, 4.3.48, 4.4.23, 5.0.18 and
5.3.15: the test fails below 4.4 before the change and passes on all
eight after it.
Generated-by: Claude Code 2.1.240 (Claude Opus 5)
github-actions
Bot
added
doc-not-needed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue: #1035
Also fixes #1044, a user-facing installer bug found while doing this. Follow-up work is tracked in #1045.
Purpose of change
tools/test/run.shrequires bash 4+ becauseerrexitdoes not apply to[[ ]]on older bash, so an assertion that stops holding still reportsok. But the check binds the shell runningrun.sh, while bats starts every test body through#!/usr/bin/env bash, which re-resolves the interpreter fromPATH. On a stock mac that is/bin/bash3.2. The check passed while the bodies ran under the interpreter it exists to reject.[[ ]]came underset -ein 4.1, not 4.0, so a major-version test admits a defective interpreter.|| falseto all 83 bare[[ ]]assertions, across 9 files. This is the form bats-core documents and the repo already uses.edit_plan_quoteintools/install.sh, which emits invalid shell on bash 3.2 and corrupts the plan state file for macOS users. Also fixes #1044.setup_suiteguard as a backstop if the pin ever stops binding.test_submit_examples_to_flink.shthat errors below bash 4.4. Change 1 declares a 4.1 floor, so without this the branch would name a floor its own suite fails on.Change 5 is the only file outside
tools/and.github/.Three things worth knowing before reading the diff.
The count in the issue is wrong. It says 42 vacuous assertions across 5 files; it is 33 across 6. The original count never subtracted the 22 that are the last statement of their block, where a bare
[[ ]]does supply the exit status and works fine. The issue also says the bulk of the cleanup is mine; it is 11 of 33.Nothing was silently failing. No vacuous assertion is currently false, and CI was never exposed, since
ci.ymlrunsbash tools/test/run.shunqualified so a singlePATHlookup feeds both the gate and the bodies. This closes a way for the suite to go quietly green on developer machines in future.All 83 are converted, not just the 33 vacuous today. The other 50 work only because they happen to be last in their block, which a later edit can change with no signal. Converting all of them makes the rule checkable: a bare
[[ ]]at statement position is now always a defect, with two documented exceptions.One accepted cost: the pin re-interprets the scripts under test too, so the suite no longer exercises
install.shunder the bash its macOS users have. That is how theedit_plan_quotebug surfaced. It is recorded in a comment at the pin and tracked in #1045.Tests
A passing suite proves nothing here, because it passed before this PR too. So each change is verified by making it fail on purpose, on real interpreters rather than by reasoning about versions.
|| falseconversion|| falseit reportsnot okon 3.2.57 and 5.3.15. Without it, the same mutation reportsokon 3.2.57.edit_plan_quotefixPATHwithout the pin gives 3.2.57.Also checked:
PATHshell.grep -rnE '^[[:space:]]*\[\[' tools/test --include='*.bats' --exclude-dir=.bats-cache | grep -vE '\|\||&&'returns exactly the two intended exceptions.Three things worth calling out.
The
edit_plan_quotetest is built to catch wrong fixes, not just the original bug. Its input carries two adjacent quotes, a later lone quote and a$x, so an escape handling only the first occurrence, only adjacent pairs, or wrapping in double quotes all fail it. The six existing tests for that function miss three such cases.The installer fix has independent evidence. On a 3.2 body the suite used to report
Executed 313 instead of expected 314, with that test missing from the TAP stream entirely. It now emits every test with no count warning.The obvious form of the empty-array fix is wrong, and the code carries a comment saying so.
("${arr[@]:-}")yields a one-element array holding the empty string rather than an empty array, which breaks the same test on every version including those that pass today. The alternate-value form is used instead.API
No public API change. The changes are to the test harness and to an internal function in the installer script.
Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.240 (Claude Opus 5)