weiqingy opened a new pull request, #1043:
URL: https://github.com/apache/flink-agents/pull/1043
Linked issue: #1035
### Purpose of change
`tools/test/run.sh` requires bash 4+ because `errexit` does not apply to `[[
]]` on older bash, so a substring assertion that stops holding still reports
`ok`. The check 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.
Four changes:
1. The threshold was wrong. `[[ ]]` became subject to `set -e` and the ERR
trap in bash **4.1**, not 4.0, so the major-version test admitted an
interpreter with the defect. bats-core states the same in its own
`docs/source/gotchas.rst`.
2. Every bare `[[ ]]` assertion at statement position now carries `||
false`, the form bats-core documents and the one already used in the checkpoint
recovery tests. 83 lines across 9 files.
3. `edit_plan_quote` in `tools/install.sh` is fixed. It is broken on bash
3.2 today, and the suite only caught it by accident of running on 3.2.
4. `run.sh` pins the interpreter bats resolves, and a `setup_suite` file
re-checks the version from inside the run.
5. `test_submit_examples_to_flink.sh` no longer dies on an empty array under
`set -u`. Expanding `${arr[@]}` when the array is empty is an error below bash
4.4, so `remove_submission_pid` failed when it removed the last pid. This
predates the branch and fails the same way on the merge base; CI never saw it,
because both legs run bash 5. It is fixed here rather than separately because
change 1 states a supported floor of 4.1, and without this the branch would
name a floor that one of its own tests does not pass on.
Two things worth stating plainly.
**The count in the issue is wrong.** It says 42 vacuous assertions across 5
files. The real figure is 33 across 6: the original count never subtracted the
22 `[[ ]]` in `checkpoint_recovery_harness.bats` that are the final statement
of their block, where a bare `[[ ]]` does supply the body's exit status and
works correctly. Two files that post-date the count add 15. 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, because `ci.yml` invokes `bash tools/test/run.sh`
unqualified, so one `PATH` lookup feeds both the gate and the bodies. This
removes a way for the suite to go quietly green in future, on developer
machines.
All 83 are converted rather than only the 33 that are vacuous today. The
other 50 work only because they happen to be last in their block, which a later
edit can change without any signal. 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.
The pin also re-interprets the scripts under test, which carry the same
shebang. The suite therefore stops exercising `install.sh` under the bash its
macOS users actually have. That cost is real, it is how the `edit_plan_quote`
bug surfaced, and it is recorded in a comment next to the pin. Restoring that
coverage properly needs its own design and is filed separately.
### Tests
The conversion is verified by mutation rather than by the suite passing,
since the suite passed before. Converted assertions were mutated false in a
scratch copy: with `|| false` they report `not ok` on both 3.2.57 and 5.3.15;
without it the same mutation reports `ok` on 3.2.57. Completeness is checked
with `grep -rnE '^[[:space:]]*\[\[' tools/test --include='*.bats'
--exclude-dir=.bats-cache | grep -vE '\|\||&&'`, which returns exactly the two
intended exceptions.
`edit_plan_quote` gets a regression test that runs the function under a real
bash 3.x and skips when none is present. On a newer interpreter the unfixed
code is already correct, so a test written there would pass with or without the
fix. 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 for that function miss three
of those cases.
Independent evidence the installer fix is real: on a 3.2 body the suite
previously reported `Executed 313 instead of expected 314` with that test
absent from the TAP stream entirely. It now emits all tests with no count
warning.
The pin is verified from inside a test body rather than from the launcher.
The guard was verified by forcing it two ways, a dangling shim and an
impossible threshold, each aborting with zero bodies run and exit 1. Every shim
failure mode was tested: dangling link, the shim path existing as a directory,
leftover temp files, read-only shim and cache, cold cache, unset interpreter.
All are loud, self-healing, or caught by the guard.
Suite is 315/315 exit 0 from a stock-`PATH` shell. Both CI legs were
simulated: with no bash 3.x present the new test skips cleanly, and with one
present it executes.
The empty-array fix is verified with the same two-direction method, on real
interpreters rather than by reasoning about versions. The test that drives
`remove_submission_pid` fails on 3.2.57, 4.0.44, 4.1.17, 4.2.53 and 4.3.48
before the change and passes on 4.4.23, 5.0.18 and 5.3.15; after the change it
passes on all eight.
The obvious form of that fix is wrong and the code carries a comment saying
so. `("${arr[@]:-}")` yields a one-element array containing the empty string
rather than an empty array, which breaks the same test on every version
including the ones that pass today. The alternate-value form is used instead.
Note one commit touches `e2e-test/test-scripts/` rather than `tools/`. That
is change 5 above, and it is the only file outside `tools/` and `.github/`.
### API
No public API change. The changes are to the test harness and to an internal
function in the installer script.
### Documentation
- [ ] `doc-needed`
- [x] `doc-not-needed`
- [ ] `doc-included`
### Was this patch authored or co-authored using generative AI tooling?
- [x] Yes
- [ ] No
Generated-by: Claude Code 2.1.240 (Claude Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
