[PR] [tools][test][e2e] Make the bats suite fail on assertions that stop holding [flink-agents]

["weiqingy (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [tools][test][e2e] Make the bats suite fail on assertions that stop holding [flink-agents]

["weiqingy (via GitHub)" <gi...@apache.org>]

weiqingy commented on PR #1043:
URL: https://github.com/apache/flink-agents/pull/1043#issuecomment-5467313535

   @wenjin272 thanks for the review. Agreed on the Bash 3.2 point, and I'll restore that coverage as part of #1045. I've linked your comment on that issue so it doesn't get lost.
   


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [tools][test][e2e] Make the bats suite fail on assertions that stop holding [flink-agents]

["wenjin272 (via GitHub)" <gi...@apache.org>]

wenjin272 merged PR #1043:
URL: https://github.com/apache/flink-agents/pull/1043


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org