search

rcaloras / bash-preexec

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

Fix false-negative test case for __bp_install #151

Closed akinomyoga closed 7 months ago

akinomyoga commented 7 months ago

This is an oversight of changing trap DEBUG to trap - DEBUG in

https://github.com/rcaloras/bash-preexec/pull/106

The oversight did not cause a test error because the test always succeeds. The problem is that even if __bp_install is broken and fails to remove trap - DEBUG, the current test case failed to detect the failure and produce a false negative. This PR fixes it.

This fix was first discussed in PR #129 and included as a part of PR #129. However, a reviewer doesn't seem to appear for PR #129, and PR #129 doesn't seem to be going to be merged currently. Because this fix is so trivial that I don't think there is a reason to block further, I separate the fix as an independent PR here.

dimo414 commented 7 months ago

Thanks for spotting this!

the test always succeeds. The problem is that even if __bp_install is broken and fails to remove trap - DEBUG, the current test case failed to detect the failure

Is it possible to update the test to detect this?

akinomyoga commented 7 months ago

Is it possible to update the test to detect this?

Maybe I'm confused or I confused you with my poor explanation, but this PR does that, i.e., to make the test case properly detect that (when it is broken in the future, i.e., the main code is not broken currently). This PR fixes a test case in bash-preexec.bats but not in the main code of bash-preexec.bash. Or do you actually request a test code for the test code?

dimo414 commented 7 months ago

Yeah sorry if that wasn't clear. What I'm suggesting is that since the test passes both prior to this change and with it, the test is still brittle (!= assertions often are). It would be nice to adjust the test further so that if this behavior changed again this assertion would catch the change rather than continue to silently pass. It might be sufficient to check for != *trap* for example, since the goal of these assertions (IIRC) is simply to confirm that PROMPT_COMMAND has been updated.

Alternatively, we could pull the two strings trap - DEBUG and __bp_install out into variables so they can't become out of sync, that way we're always asserting "present before, not present after".

akinomyoga commented 7 months ago

Ah, I see. Thank you for your clarification!

Alternatively, we could pull the two strings trap - DEBUG and __bp_install out into variables so they can't become out of sync, that way we're always asserting "present before, not present after".

It is actually another part of #129. We already have the variable __bp_install_string introduced by PR #110 (bash-preexec.sh:54@8a9f00c), so we can use that variable to simplify the test as in test/bash-preexec.bats:77@9452363.

akinomyoga commented 7 months ago

I added a commit 8913ea5fafdde261bb5ea64eef3ad477be9f589e. Thank you!

akinomyoga commented 7 months ago

Thanks! I've applied the suggestion with a line break added.