Re: [PATCH 6/7] object tests: add test for unexpected objects in tags

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Mar 28 2021, Jeff King wrote:

> On Sun, Mar 28, 2021 at 03:35:46AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> Fix a blind spot in the tests added in 0616617c7e1 (t: introduce tests
>> >> for unexpected object types, 2019-04-09), there were no meaningful
>> >> tests for checking how we reported on finding the incorrect object
>> >> type in a tag, i.e. one that broke the "type" promise in the tag
>> >> header.
>> >
>> > Isn't this covered by tests 16 and 17 ("traverse unexpected non-commit
>> > tag", both "lone" and "seen")? And likewise the matching "non-tree" and
>> > "non-blob" variants afterwards?
>> 
>> Barely, those tests are mainly testing that rev-list doesn't die, and
>> only do a very fuzzy match on the output. E.g. checking `grep "not a
>> commit" err`, not a full test_cmp that'll check what OID is reported
>> etc.
>
> I thought the "blind spot" you meant was not testing these cases. But I
> guess you mean that we are not checking stderr.
>
> So OK, but...

Ah yes. I'll clarify that. I thought it was clear since the series is
about the output we emit on errors, not the rev-list traversal itself.

>> > The only thing we don't seem to cover is an unexpected non-tag. I don't
>> > mind adding that, but why wouldn't we just follow the template of the
>> > existing tests?
>> 
>> I am following that template to some extent, e.g. using
>> ${commit,tree,blob}. It just didn't seem worth it to refactor an earlier
>> test in the file just to re-use a single hash-object invocation, those
>> tests e.g. clobber the $tag variable, so bending over backwards to
>> re-use anything set up in them would mean some refactoring.
>> 
>> I think it's much clearer just do do all the different kinds of setup in
>> the new setup function.
>
> It does not seem to make the resulting test script more clear at all to
> create the same situation twice, but test stderr only in the second
> case. I.e., why is the change to the test script not just:
>
> diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
> index 52cde097dd..4cdc87c913 100755
> --- a/t/t6102-rev-list-unexpected-objects.sh
> +++ b/t/t6102-rev-list-unexpected-objects.sh
> @@ -82,12 +82,13 @@ test_expect_success 'setup unexpected non-commit tag' '
>  '
>  
>  test_expect_success 'traverse unexpected non-commit tag (lone)' '
> -	test_must_fail git rev-list --objects $tag
> +	test_must_fail git rev-list --objects $tag >output 2>&1 &&
> +	test_i18ngrep "is a blob, not a commit" output
>  '
>  
>  test_expect_success 'traverse unexpected non-commit tag (seen)' '
>  	test_must_fail git rev-list --objects $blob $tag >output 2>&1 &&
> -	test_i18ngrep "not a commit" output
> +	test_i18ngrep "is a blob, not a commit" output
>  '
>  
>  test_expect_success 'setup unexpected non-tree tag' '
>
> and so forth (or you can replace it with a full test_cmp if you really
> prefer). That does not seem like bending over backwards to me, but
> rather keeping the test script tidy and readable.

Yeah, it needs to be a test_cmp (or equivalent) since the point of the
test is *which* thing we're reporting as the "is X not Y".

But yes, these can be combined. But I still think it's clearer to have
minimal tests for the traversal and then later (much more verbose) tests
for the output.

So if you the traversal fails you'd be looking at a fairly isolated test
with just two lines, v.s. 10-15 lines of later test_cmp etc.

> [...]

Will reply to the rest, but that discussion seems split, so reading the
thread to see what the best place to continue that chat is...