On Fri, Oct 1, 2010 at 10:23, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Ãvar ArnfjÃrà Bjarmason wrote:
>> On Wed, Sep 29, 2010 at 18:07, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> Elijah Newren <newren@xxxxxxxxx> 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.
> +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.
> Â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?
> Â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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html