On Mon, Jan 6, 2020 at 11:53 PM Denton Liu <liu.denton@xxxxxxxxx> wrote:
> t2018: use test_expect_code for failing git commands
>
> When we are expecting `git diff` to fail, we negate its status with
> `!`. However, if git ever exits unexpectedly, say due to a segfault, we
> would not be able to tell the difference between that and a controlled
> failure. Use `test_expect_code 1 git diff` instead so that if an
> unexpected failure occurs, we can catch it.
>
> We have some instances of `test_must_fail test_dirty_{un,}mergable`.
> However, `test_must_fail` should only be used on git commands. Convert
> these instances to use the `test_dirty_{un,}mergeable_discards_changes`
> helper functions so that we remove the double-negation.
I had to read all of this several times to understand what it was
trying to say. I think what made it difficult was a combination of
talking about using test_expect_code() for "failing git commands",
coupled with the use "we" in "When we are ... we negate..." which (to
me) sounds as if you are describing the _desired_ way of coding this,
not the incorrect way. A possible rewrite:
t2018: be more discerning when checking for expected exit codes
Functions test_dirty_unmergeable() and test_dirty_mergeable()
expect git-diff to exit with the specific code 1. However, rather
than checking for that value explicitly, they instead negate the
exit code. Unfortunately, this negation makes it impossible to
distinguish the expected code from some other unexpected non-zero
code, for instance, from a segmentation fault. Therefore, be more
discerning by checking the exit code explicitly using
test_expect_code().
Furthermore, some callers of those functions want to negate the
result again, and do so with test_must_fail(). However,
test_must_fail() should only be used with git commands. Address
this by introducing a couple new tiny helper functions which test
the exact condition expected (without the unnecessarily confusing
double-negation).
> While we're at it, remove redirections to /dev/null since output is
> silenced when running without `-v` anyway.
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
