Fix false-negative test case for __bp_install

[akinomyoga]

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

#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]

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]

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]

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]

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]

I added a commit 8913ea5. Thank you!

[dimo414]

Thanks! Suggested a comment but otherwise LGTM.

[akinomyoga]

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