Skip to content

tests: Fix checks where we expect a command to fail - #6625

Merged
swick merged 3 commits into
flatpak:mainfrom
swick:wip/test-fail-fix
Apr 28, 2026
Merged

swick merged 3 commits into
flatpak:mainfrom
swick:wip/test-fail-fix

Conversation

@swick

@swick swick commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator
tests: Fix checks where we expect a command to fail

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 `command && false`. One could also use `command &&
assert_not_reached "message"` but who has time to write error messages
for all the cases.

with it fixed, it revealed a real bug:

system-helper: Fix checking if the reinstall flag was passed in

@swick
swick force-pushed the wip/test-fail-fix branch from 40fc7f4 to 20e7b6c Compare April 14, 2026 19:22

@smcv smcv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

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).

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.

Comment thread tests/test-bundle.sh Outdated
Comment thread tests/test-preinstall.sh
@swick
swick force-pushed the wip/test-fail-fix branch 2 times, most recently from d192ce3 to 2c63a31 Compare April 16, 2026 11:58
Comment thread tests/libtest.sh Outdated
@swick swick added this to the 1.18 milestone Apr 16, 2026
swick added 3 commits April 28, 2026 12:09
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")
@swick
swick force-pushed the wip/test-fail-fix branch from 2c63a31 to 36f20af Compare April 28, 2026 10:27
@swick

swick commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@bbhtt

bbhtt commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Seems fine.

@swick
swick added this pull request to the merge queue Apr 28, 2026
Merged via the queue into flatpak:main with commit bd75302 Apr 28, 2026
11 checks passed
@swick
swick deleted the wip/test-fail-fix branch April 28, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants