Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren <newren@gmail.com> wrote:
> On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > [...] Therefore,
> > replace the manual exit code management with test_must_fail() and a
> > normal &&-chain.
> >
> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> > ---
> > diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
> > @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not segfault' '> > -               git push .. master:master
> > -               test $? = 1
> > +               test_must_fail git push .. master:master
>
> test_must_fail or test_expect_code 1?

A legitimate question, and the answer is that it's a judgment call
based upon the spirit of the test.

Although test_expect_code() would make for a faithful literal
conversion, I don't think it agrees with the spirit of this test,
which wants to verify that the command correctly exits with an error
(in the general sense), as opposed to outright crashing (which it did
at one time). test_must_fail() is tailor-made for this use case.

Contrast this with patch 09/29 "t7810: use test_expect_code() instead
of hand-rolled comparison". Different exit codes from git-grep have
genuine different meanings, and that test is checking for a very
specific exit code, for which test_expect_code() is tailor-made.

> > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> > @@ -12,20 +12,8 @@ test_expect_success 'start p4d' '
> >  test_expect_success 'p4 help unknown returns 1' '
> >         (
> >                 cd "$cli" &&
> > -               (
> > -                       p4 help client >errs 2>&1
> > -                       echo $? >retval
> > -               )
> > -               echo 0 >expected &&
> > -               test_cmp expected retval &&
> > -               rm retval &&
> > -               (
> > -                       p4 help nosuchcommand >errs 2>&1
> > -                       echo $? >retval
> > -               )
> > -               echo 1 >expected &&
> > -               test_cmp expected retval &&
> > -               rm retval
> > +               p4 help client &&
> > +               test_must_fail p4 help nosuchcommand
>
> same question?

Same answer. Not shown in this patch, but just above the context lines
you will find this comment in the file:

    # We rely on this behavior to detect for p4 move availability.

which means that the test is really interested in being able to
reliably detect if a sub-command is or is not available. So, despite
the (somewhat) misleading test title, this test doesn't care about the
exact error code but rather cares only that "p4 help nosuchcommand"
errors out, period. Hence, test_must_fail() again agrees with the
spirit of the test.