Re: [PATCH 6/7] object tests: add test for unexpected objects in tags
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-03-28 15:40:39
On Sun, Mar 28 2021, Jeff King wrote:
On Sun, Mar 28, 2021 at 03:35:46AM +0200, Ævar Arnfjörð Bjarmason wrote:quoted
quoted
quoted
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.
quoted hunk ↗ jump to hunk
quoted
quoted
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...