weiqingy opened a new issue, #1045: URL: https://github.com/apache/flink-agents/issues/1045
### Description Two defects at the same seam, where the bats suite meets the scripts it tests. They are separate problems but they touch the same sourcing path and the first has to land before the second is legible, so they are tracked together. **1. install.sh's traps replace bats' own, so failures stop reporting.** `tools/install.sh:382` sets an unconditional `ERR` trap whose handler exits, and `:41` sets `trap cleanup_tmpfiles EXIT`. Both are at top level, so they run on source. `install.sh:2034` already gates the main run behind `FLINK_AGENTS_INSTALL_SH_NO_RUN`, but the traps are not gated. Every `.bats` file whose `setup()` calls `load_install_sh` inherits them. That is 21 files. In those, a failing assertion emits **no `not ok` line at all**: the test disappears from the TAP stream, leaving only `# bats warning: Executed N instead of expected M`. bats' `skip` breaks the same way, because it works by exiting 0 and depends on bats' `EXIT` trap still being installed. The run still exits non-zero, so CI is not fooled. Anything scanning output for `not ok` is, and a vanished test reads as a count discrepancy rather than a failure with a file and line. To reproduce: mutate any assertion to be false in a file that sources `install.sh`, for example `tools/test/unit/ui_helpers.bats`, and run the suite. No `not ok` appears. A control in a file that does not source it, such as `tools/test/integration/build_help.bats`, reports normally. This reproduces on a tree without any of #1035's changes, using a plain `[ ]` assertion, so it is not related to the `|| false` conversion. **2. The suite no longer exercises the scripts under test on the bash their users run.** #1035 pins the interpreter bats resolves, because bats starts every test body through `#!/usr/bin/env bash` and would otherwise run bodies on macOS's `/bin/bash` 3.2, where `errexit` does not apply to `[[ ]]`. The scripts under test carry the same shebang, so the pin re-interprets them too. `tools/install.sh` is the end-user installer: it declares macOS support, it is fetched and piped to bash, and macOS resolves `bash` to `/bin/bash` 3.2.57. So 3.2 is its production environment and the suite no longer runs it there. That is not hypothetical. It is how the `edit_plan_quote` bug fixed in #1035 was found: the suite happened to be running on 3.2 and caught a parameter-expansion difference that produced invalid shell in the dumped state file. Two other places in `install.sh` already work around bash 3.2 differences, so the class recurs. ### Suggested approach Order matters. Fix 1 first: while failures in those 21 files vanish rather than report, any new failure that fix 2 surfaces would be swallowed instead of shown. For 1, gating the traps on the same `FLINK_AGENTS_INSTALL_SH_NO_RUN` flag that already gates the main run is the obvious candidate, though it changes what sourcing `install.sh` does and some tests may rely on `cleanup_tmpfiles`. Saving and restoring bats' traps around `load_install_sh` is the alternative. For 2, invoking the scripts under test through an explicit interpreter keeps the harness requirement (bash 4.1+, so assertions work) separate from the subject's (whatever the user has). A CI leg on stock `/bin/bash` would also help. The pin exists to make assertions honest, which is a property of the harness rather than of the subject. Both fixes exist to make hidden failures visible, so either may surface work that is currently invisible. Expect the scope to grow with whatever turns up. ### Are you willing to submit a PR? Yes, one PR covering both, with the trap fix first. -- 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]
