weiqingy opened a new pull request, #1081: URL: https://github.com/apache/flink-agents/pull/1081
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.sh` installed 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 no `not ok` line 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.sh` is an end-user installer supporting macOS, where `bash` is 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_BASH` selects the interpreter for the script under test and defaults to `bash`, 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 `skip` still report from a file that sources `install.sh`. The other asserts that spawned scripts really do run on the selected interpreter, and that no spawn site drifts back to a bare `bash`. 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.sh` rejects a `FLINK_AGENTS_SUT_BASH` that 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_BASH` is read only by the test harness. ### 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.251 (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]
