rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
862 stars 94 forks source link

Tests fail with bats 1.5.0 #121

Closed rycee closed 1 year ago

rycee commented 2 years ago

See below for a test run with Bats 1.5.0 (the same run succeeds with Bats 1.4.1). Originally found in a NixOS CI job. This is with bash-preexec 0.4.1 but master fails in the same way.

I'm not completely certain what the cause is but the line

Error: Test file "/home/rycee/devel/nixpkgs/source/merge" does not exist

seems problematic. I wonder if not the "merge" in the error above comes from the "merge" in bats_merge_stdout_and_stderr?

$ bats test
 ✓ __bp_install_after_session_init should exit with 1 if we're not using bash
 ✓ __bp_install should exit if it's already installed
 ✓ __bp_install should remove trap logic and itself from PROMPT_COMMAND
 ✓ __bp_install should preserve an existing DEBUG trap
 ✓ __bp_sanitize_string should remove semicolons and trim space
 ✓ Appending to PROMPT_COMMAND should work after bp_install
 ✓ Appending or prepending to PROMPT_COMMAND should work after bp_install_after_session_init
 ✓ Adding to PROMPT_COMMAND before and after initiating install
 ✓ Adding to PROMPT_COMMAND after with semicolon
 ✓ during install PROMPT_COMMAND and precmd functions should be executed each once
 ✓ No functions defined for preexec should simply return
 ✓ precmd should execute a function once
 ✓ precmd should set $? to be the previous exit code
 ✓ precmd should set $BP_PIPESTATUS to the previous $PIPESTATUS
 ✓ precmd should set $_ to be the previous last arg
 ✓ preexec should execute a function with the last command in our history
 ✓ preexec should execute multiple functions in the order added to their arrays
 ✓ preecmd should execute multiple functions in the order added to their arrays
 ✗ preexec should execute a function with IFS defined to local scope
   (in test file test/bash-preexec.bats, line 260)
     `[ $status -eq 0 ]' failed
   Error: Test file "/home/rycee/devel/nixpkgs/source/merge" does not exist
 ✗ precmd should execute a function with IFS defined to local scope
   (in test file test/bash-preexec.bats, line 270)
     `[ $status -eq 0 ]' failed
   Error: Test file "/home/rycee/devel/nixpkgs/source/merge" does not exist
 ✓ preexec should set $? to be the exit code of preexec_functions
 ✓ in_prompt_command should detect if a command is part of PROMPT_COMMAND
 ✓ __bp_adjust_histcontrol should remove ignorespace and ignoreboth
 ✓ preexec should respect HISTTIMEFORMAT
 ✓ preexec should not strip whitespace from commands
 ✓ preexec should preserve multi-line strings in commands
 ✓ preexec should work on options to 'echo' commands
 ✓ should not import if it's already defined
 ✓ should import if not defined
 ✓ bp should stop installation if HISTTIMEFORMAT is readonly

30 tests, 2 failures
benjamb commented 1 year ago

@rycee This should be fixed in bats 1.8.0. The relevant fix is: https://github.com/bats-core/bats-core/commit/c8b5bbade301cecb9e8039615a4125269ec24891

akinomyoga commented 1 year ago

Ah, this is reported here. I actually submitted the fix to bats-core for the test failure in bash-preexec but haven't recognized that the issue was already reported here. Thank you for the comment. @rcaloras Now I think this issue can be closed.

(And, now I noticed my typo in the commit message s/sepcial/special/...)

rcaloras commented 1 year ago

Great. Thanks as always @akinomyoga!

rycee commented 1 year ago

Thanks, I tester just now with bats 1.8.2 and curiously I get

ok 19 preexec should execute a function with IFS defined to local scope
not ok 20 precmd should execute a function with IFS defined to local scope
# (in test file test/bash-preexec.bats, line 272)
#   `[ $status -eq 0 ]' failed
ok 21 preexec should set $? to be the exit code of preexec_function
akinomyoga commented 1 year ago

I'm not sure why I overlooked this, but this seems to be an oversight of updating tests in #131. Lines 212 and 248 in test/bash-preexec.bats seem to have been correctly updated, but line 280 was not updated at that time. I guess this is because this test failure had been hidden by the bug of bats-core, but it is revealed now that bats-core is fixed by https://github.com/bats-core/bats-core/pull/650.

akinomyoga commented 1 year ago

@rycee I realized that this is actually already fixed in the master by @rcaloras 17 hours ago! You can pull the latest master and try it!

rycee commented 1 year ago

@akinomyoga Indeed, with latest master all tests pass. Thanks!