Re: [PATCHv4 02/15] t4017 (diff-retval): replace manual exit code check with test_expect_code
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 1, 2010, 10:38 UTC
- Message-ID
- <AANLkTinqVTqxiHL5tEv+-SS6YURGUoWaPxCgpccZgjEq@mail.gmail.com>
- In-Reply-To
- <20101001102315.GA6816@burratino>
On Fri, Oct 1, 2010 at 10:23, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 16 quoted lines
> Ævar Arnfjörð Bjarmason wrote: >> On Wed, Sep 29, 2010 at 18:07, Junio C Hamano <gitster@pobox.com> wrote: >>> Elijah Newren <newren@gmail.com> writes: > >>>> -test_expect_success 'git diff-tree HEAD^ HEAD' ' >>>> +test_expect_code 1 'git diff-tree HEAD^ HEAD' ' >>>> git diff-tree --exit-code HEAD^ HEAD >>>> - test $? = 1 >>>> ' >> >> It also looks like this will pass for for all exit codes that *aren't* >> 1, because if $? != 1 +test_expect_code will get the exit code of >> 1. > > You probably missed the - indicating that the "test $? = 1" was being > removed.
Correct. I misread that.
Show 7 quoted lines
> +check_exit_status () {
> + echo "$1" >expect.status
> + shift
> + "$@"
> + echo "$?" >actual.status
> + test_cmp expect.status actual.status
> +}If we add this it should be in the test-lib.sh, it'll probably be useful for other tests.
Show 10 quoted lines
> test_expect_success 'setup' ' > echo "1 " >a && > git add . && > @@ -21,110 +29,81 @@ test_expect_success 'git diff --quiet -w HEAD^^ HEAD^' ' > ' > > test_expect_success 'git diff --quiet HEAD^^ HEAD^' ' > - test_must_fail git diff --quiet HEAD^^ HEAD^ > + check_exit_status 1 git diff --quiet HEAD^^ HEAD^ > '
In most uses of check_exit_status you're using it is the very last command within a test_expect_success. Isn't it redundant to using just "test_expect_code $code ..." there?
Show 14 quoted lines
> test_expect_success 'check detects leftover conflict markers' ' > @@ -133,10 +112,8 @@ test_expect_success 'check detects leftover conflict markers' ' > echo binary >>b && > git commit -m "side" b && > test_must_fail git merge master && > - git add b && ( > - git --no-pager diff --cached --check >test.out > - test $? = 2 > - ) && > + git add b && > + check_exit_status 2 git --no-pager diff --cached --check >test.out && > test 3 = $(grep "conflict marker" test.out | wc -l) && > git reset --hard > '
But of course in cases like these it's needed.
In any case, we only use test_expect_code for two tests in the whole test suite now, and checking the exit status for individual commands is more self-documenting and less prone to break if we add more tests to a given test.
So IMO the best thing to do would be to re-appropriate "test_expect_code" so that it runs inside a test (i.e. does what your check_exit_status does), and not at the top-level.
Then do s/check_exit_status/test_expect_code/g on your patch, and change the tests using test_expect_code in t1504-ceiling-dirs.sh and t6020-merge-df.sh to use test_expect_success + test_expect_code.