Re: [PATCH v2 04/16] t2018: use test_expect_code for failing git commands

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 12, 2020, 10:50 UTC
Message-ID: <CAPig+cSoDFVonhn=xoWasHcN_hJHVoTiMT7=aU9ujiW93qk8kg@mail.gmail.com>
In-Reply-To: <587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com>

On Mon, Jan 6, 2020 at 11:53 PM Denton Liu <liu.denton@gmail.com> wrote:

Show 12 quoted lines

> 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@gmail.com>