fix: Test failures on Bash 5.3 macOS (nix-shell) - #913
Conversation
Users with diff.external configured (e.g. difftastic) get broken diff output because git delegates to the external tool, which ignores --word-diff and produces a different format.
Bash 5.3 on macOS segfaults (exit 139) when LC_ALL is set inside a command substitution that reads the magic EPOCHREALTIME variable. Besides, force the console_results_test to use the perl clock path on Bash 5+.
Bash 5.3 on macOS segfaults when LC_ALL=C is used as a temporary
env prefix inside $() command substitutions. The LC_ALL=C LANG=C
was unnecessary here: the asserted strings ('failed', 'Error')
come from bashunit's own output, not locale-dependent messages.
/tmp is not writable inside the nix sandbox on darwin, causing all init tests to fail with 'Permission denied'.
The JUnit time reads still used a bare LC_ALL=C awk prefix, the same form that segfaults inside $() on Bash 5.3.9 macOS. C is required here (it keeps awk's radix a dot in the XML), so pass it through env instead of removing it.
Nothing guarded either fix: no CI job runs Bash 5.3 macOS or sets diff.external, so both regressions could return unnoticed.
There was a problem hiding this comment.
Nice catch, and thanks for the clear reproducer in #912. That made both bugs easy to confirm. You got here first, so I'm merging yours and closing my #914.
I pushed a few small things on top so you don't have to round-trip:
src/reports.shhad the sameLC_ALL=Cprefix in its two JUnit time reads, so--report-junitwould still crash on Bash 5.3. There theClocale is actually doing something (it keeps awk printing1.234and not1,234), so I usedenv LC_ALL=C awkinstead of removing it.- A test for each fix. CI can't catch these on its own: no runner has
diff.externalset, and none runs Bash 5.3 on macOS. Both tests fail without your changes. - A short comment next to each removal so nobody puts the prefix back, plus the CHANGELOG entries moved to the top of Fixed with the
(#912)link.
Dropping LC_ALL=C LANG=C from the acceptance test was the right call. Those assertions only look at bashunit's own output, and the Spanish/Brazilian/Japanese jobs run the acceptance tests too, so they'd have told us otherwise.
All green here on macOS Bash 3.2 (make test, --parallel, make sa, make lint). Thanks again!
|
@Chemaclass Wow, you are fast. Thank you. Let me know when you have cut a new release with tag, then I start bumping and simplifying the nix derivation. |
|
@tricktron new release out! https://github.com/TypedDevs/bashunit/releases/tag/0.44.0 |
2 participants
Background
Closes #912.
I first stumbled on this issue in NixOS/nixpkgs#543029.
If merged we can update and simplify the bashunit derivation in nixpkgs and all tests will pass even in the sandbox on darwin.
Changes
render_diffdidn't bypass external diff tools. Anyone with e.g. difftastic got broken output (2 failures)shell_time()and one acceptance test usedLC_ALL=Cinside$(), which segfaults on Bash 5.3 macOS. I removed since it was unnecessary in both places (5 failures).Checklist
CHANGELOG.mdto reflect the new feature or fix