Repository navigation
tests: Fix checks where we expect a command to fail - #6625
Conversation
40fc7f4 to
20e7b6c
Compare
smcv
left a comment
There was a problem hiding this comment.
Good catch!
I was convinced that the pattern
! commandwith -e aborts whencommandfails. This is not the case (the result offalseis the same as! truebut somehow this doesn't matter).
It doesn't matter because POSIX shells explicitly exempt some patterns from triggering set -e, to make set -e be useful. For example it would be unusable if if thing_that_fails; then ...; else ...; fi crashed out as soon as thing_that_fails fails, without ever getting to the body of the else.
! foo is one of those patterns, as documented in bash(1):
-e Exit immediately if a pipeline (which may consist of a
single simple command), a list, or a compound command
(see SHELL GRAMMAR above), exits with a non-zero status.
The shell does not exit if the command that fails is part
of the command list immediately following a while or un‐
til keyword, part of the test following the if or elif
reserved words, part of any command executed in a && or
|| list except the command following the final && or ||,
any command in a pipeline but the last, or if the com‐
mand's return value is being inverted with !.
(last part of the quote).
Just to troll us, dash(1) (Debian/Ubuntu /bin/sh) doesn't document that ! has this effect, but I don't know whether it's the shell behaviour or just the documentation that actually differs from bash here. But Flatpak's tests explicitly use bash rather than /bin/sh, so we're insulated from that anyway.
dash(1) does have a nicer way to describe what set -e does, which might help you to have an appropriate mental model: it describes it as making the shell exit if an "untested command" fails, and then goes on to say that various things like if and the LHS of && are considered to be "explicitly tested" for the purposes of set -e.
d192ce3 to
2c63a31
Compare
I was convinced that the pattern `! command` with -e aborts when `command` fails. This is not the case (the result of `false` is the same as `! true` but somehow this doesn't matter). Fix the tests and use the newly introduced `assert_not` function. One could also use `command && assert_not_reached "message"` but who has time to write error messages for all the cases.
Fixes: 919d292 ("common: support reinstall option on bundle installations")
Instead of trying to read them into variables, which could fail if there were null bytes in the key. Fixes: 4364233 ("dir: Try to delete the remote if we failed to add it entirely")
2c63a31 to
36f20af
Compare
|
Also added one more commit which makes a test more robust which would randomly fail depending on if a generated key contained a null byte or not. |
|
Seems fine. |
with it fixed, it revealed a real bug: