[tools][ci] Fix the bats suite's boundary with the scripts it tests - #1081
Merged
Conversation
…n flag install.sh installs its EXIT, INT and ERR traps at top level, so a `source` installs them too. The 21 bats files whose setup() calls load_install_sh inherit them, which is 154 of the suite's 316 tests. bats reports a failing or skipped result from inside its own EXIT trap, so a handler laid over that one makes such a test vanish from the stream rather than fail: the run still exits non-zero, but the output carries only a count mismatch, and under the pretty formatter it summarises as "0 failures". The main run was already gated behind FLINK_AGENTS_INSTALL_SH_NO_RUN. Put the traps behind the same condition, so sourcing the file to call individual functions leaves the caller's handlers as it found them. A real run, including the piped-to-bash install path, is unchanged. Gate all three rather than EXIT alone. Only EXIT hides a result, but an ERR handler left armed rewrites the trace of whatever survives, printing an installer failure banner for a failure that belongs to the test, and INT displaces bats' own interrupt handler. edit_plan_quote.bats carried a conditional setup() working around this; it is removed, restoring the skip path that documented the defect. install_sh_trap_isolation.bats drives a nested bats run over a fixture written at run time, since a test cannot observe its own missing result. Reverting each gate individually gives a distinct verdict, and the trace assertion keys on the on_error frame displacing bats' own rather than on a line number, which differs between bash 3.2 and 5.x. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
…d cover bash 3.2 run.sh puts a `bash` symlink to its own interpreter at the front of PATH so every `#!/usr/bin/env bash` hop inside the run resolves to a shell new enough for a failing `[[ ]]` to fail the test. That pin also reaches the scripts under test, which is more than it was meant to do: those scripts run on whatever interpreter their own users have. install.sh is fetched and piped to bash and declares macOS support, and ut.sh is invoked by path by two CI jobs that matrix on macos-latest. On macOS `bash` is 3.2, so the suite stopped exercising either where it runs in production, and three of the four bash 3.2 workarounds in those scripts have no test. Name the subject's interpreter explicitly. FLINK_AGENTS_SUT_BASH is exported by run.sh and used at the 47 sites that spawn a script under test; an absolute path is not a PATH lookup, so it is not claimed by the pin. The harness keeps its own floor, which is a property of the harness rather than of the subject. Three sites nest `bash -c "bash ..."`, where only the inner token is the subject; there the variable is expanded by the outer shell and single-quoted into the string. run.sh rejects a set value that does not report a bash version, probing for the version rather than testing executability, since an executable non-shell yields passing tests that assert nothing. An unset value still falls back to `bash`. Without this a misspelled key in the new job would leave a green leg covering nothing. The new CI job runs the suite with the subject on /bin/bash and asserts the major version rather than skipping, so a runner without it fails rather than passing silently. It is a separate job: an `include:` entry whose original matrix keys match an existing combination adds keys to that combination, which would have converted the existing macOS leg instead of adding a third. spawn_site_interpreter.bats drives a nested bats run over a converted file and asserts the subject reached the named interpreter, then checks statically that no file spawns a subject through a bare `bash` or through its shebang alone. The static half is what catches the next site added the old way. Both patterns read a single line, so a spawn split across a continuation is outside what they can see. Generated-by: Claude Code 2.1.251 (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
2 participants
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: #1045
Purpose of change
Two problems where the bats suite meets the scripts it tests. Neither is visible from a passing run, which is what makes them worth fixing together.
The suite could not report failures in nearly half its tests.
tools/install.shinstalled EXIT, INT and ERR traps as soon as it was sourced, and 21 test files source it to call individual functions. Bash replaces an EXIT trap rather than stacking it, and bats prints a failing or skipped result from inside its own EXIT trap. So in those files a failing assertion produced nonot okline at all. That covered 154 of the 316 tests on main. The suite reported healthy counts either way, so this only shows up when a test is supposed to fail and quietly does not.The traps now arm only when the script actually runs, reusing the flag that already gates the main run. A real install is unaffected. One test had worked around the bug with a conditional
setup(), and that workaround is gone.The suite stopped testing the scripts on the bash their users run.
Pinning the bash that bats itself runs on also changed the bash that the scripts under test run on.
install.shis an end-user installer supporting macOS, wherebashis 3.2.57, so the suite stopped covering the version its users are most likely to hit.The harness bash and the subject bash are now separate.
FLINK_AGENTS_SUT_BASHselects the interpreter for the script under test and defaults tobash, so a local run behaves exactly as before. A new CI job runs the suite on macOS with the subject on/bin/bash.Tests
321 tests pass in both configurations, the default and with the subject on bash 3.2, with zero bats warnings in each. The warning count is worth stating on this PR in particular: a swallowed test emits a warning and no
not ok, so a pass count on its own is exactly what hid the first bug.Two new files guard the fixes. One asserts that a failing assertion and a
skipstill report from a file that sourcesinstall.sh. The other asserts that spawned scripts really do run on the selected interpreter, and that no spawn site drifts back to a barebash.Both were checked by deliberately reverting the fix they guard, to confirm they can actually fail. Undoing the interpreter change fails 2 of the 321.
Two smaller safeguards:
tools/test/run.shrejects aFLINK_AGENTS_SUT_BASHthat does not report a bash version, exiting before any test runs, and the new CI job fails loudly rather than skipping if it does not get bash 3. Both exist so the job cannot report success while testing the wrong thing.API
No public API change.
FLINK_AGENTS_SUT_BASHis read only by the test harness.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.251 (Claude Opus 5)