Re: [PATCH v2 2/9] t5526: use grep to assert on fetches

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

On Thu, Feb 17 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Wed, Feb 16 2022, Glen Choo wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>> On Wed, Feb 16 2022, Glen Choo wrote:
>>>>
>>>>> In a previous commit, we replaced test_cmp invocations with
>>>>> verify_fetch_result(). Finish the process of removing test_cmp by using
>>>>> grep in verify_fetch_result() instead.
>>>>>
>>>>> This makes the tests less sensitive to changes because, instead of
>>>>> checking the whole stderr, we only grep for the lines of the form
>>>>>
>>>>> * "<old-head>..<new-head>\s+branch\s+-> origin/branch"
>>>>> * "Fetching submodule <submodule-path>" (if fetching a submodule)
>>>>>
>>>>> when we expect the repo to have fetched. If we expect the repo to not
>>>>> have fetched, grep to make sure the lines are absent. Also, simplify the
>>>>> assertions by using grep patterns that match only the relevant pieces of
>>>>> information, e.g. <old-head> is irrelevant because we only want to know
>>>>> if the fetch was performed, so we don't need to know where the branch
>>>>> was before the fetch.
>>>>
>>>> I tried ejecting 1/9 and 2/9 out of this series locally, and it passes
>>>> all tests until the new tests you add in 7/9.
>>>>
>>>> As ugly as some of the pre-image is, I wonder if dropping these first
>>>> two and biting the bullet and just continuing with the test_cmp is
>>>> better.
>>>>
>>>> The test_cmp is going to catch issues that even the cleverest grep
>>>> combinations won't, e.g. in the submodule-in-C series I discovered a bug
>>>> where all of our testing & the existing series hadn't spotted that we
>>>> were dropping a \n at the end in one of the messages.
>>>
>>> I think there are two schools of thought on how to test informational
>>> messages:
>>>
>>> - assert an exact match on the exact output that we generate
>>> - assert that the output contains the pieces of information we care
>>>   about
>>>
>>> These two approaches are virtually opposites on two axes - the former
>>> will catch unintentional changes (like you've noted)[...]
>>
>> Yes, and to be fair I'm thoroughly in the "assert an exact match" camp,
>> i.e. "let's just use test_cmp", and not everyone would agree with that.
>>
>> I mean, I don't think we should test_cmp every single instance of a
>> command, but for things that are *the tests* concerning themselves with
>> what the output should be, yes we should do that.
>
> That's a good point I hadn't considered, which is that if we want any
> hope of catching unintentional changes in our test suite, we'd want
> _some_ test to check the output. For "git fetch --recurse-submodules",
> it makes the most sense for that test to live in this file.
>
> By eliminating all instances of test_cmp in this file in particular, we
> lose assurances that we don't introduce accidental changes. It makes
> sense to at least have some tests explicitly for output.
>
>>
>>> [...] and the latter saves on maintenance effort and tends to be less noisy in tests.
>>
>> I also don't think you're right about the other approach "sav[ing] on
>> [future] maintenance effort" in this case.
>>
>> If I was needing to adjust some of this output I'd spend way longer on
>> trying to carefully reason that some series of "grep" invocations were
>> really doing the right thing, and probably end up doing the equivalent
>> of a "test_cmp" for myself out of general paranoia, whereas adjusting
>> the output.
>
> That's fair. I've optimized the tests for readability by putting
> complicated logic in the test helper. But any diligent test reader would
> need to read the test helper to convince themselves of its correctness.
> In this case, I agree that the helper is too complex.
>
>>> Personally, I'm a bit torn between both approaches in general because I
>>> want tests to be maintainable (testing the exact output is a bit of an
>>> antipattern at Google), but I'm not very comfortable with the fact that
>>> unintended changes can sneak through.
>>
>> Yes, anyway whatever one thinks in general what I meant to point out
>> here with "biting the bullet" is that whatever one thinks in general
>> about the right approch for new tests, this series in particular seems
>> to be creating more work for itself than it needs by refactoring the
>> test_cmp in existing tests just to add a few new ones.
>>
>> I.e. even if you'd like to not use test_cmp-alike for the new tests,
>> wouldn't it be simpler to just leave the old ones in place and use your
>> new helper for your new tests?
>
> I'm not sure about this - avoiding changing old tests leads to
> fragmentation in the test suite and even the same file. I find it very
> challenging to read/modify files like this, because there is no longer a
> consistent style for the file, and I have to figure out which is the
> "good" way to write tests.
>
> This suggestion makes sense if there's some qualitative difference
> between the new tests and old ones besides just 'being new'. This isn't
> true for this series, so I'd prefer to keep things consistent.
>
>>> So I don't think there's a correct answer in general. Maybe an
>>> acceptable rule of thumb is that test_cmp is good until it starts
>>> getting in the way of reading and writing understandable tests.
>>>
>>> If we agree on that rule, then for this patch, I think replacing
>>> test_cmp is the way to go, primarily because it lets us ignore the 'old
>>> head' of the branch before the fetch, e.g. in the quoted example..
>>
>> [...]
>>
>>>>>  test_expect_success setup '
>>>>> @@ -274,13 +277,10 @@ test_expect_success "Recursion doesn't happen when no new commits are fetched in
>>>>>  '
>>>>>  
>>>>>  test_expect_success "Recursion stops when no new submodule commits are fetched" '
>>>>> -	head1=$(git rev-parse --short HEAD) &&
>>>>>  	git add submodule &&
>>>>>  	git commit -m "new submodule" &&
>>>>> -	head2=$(git rev-parse --short HEAD) &&
>>>>> -	echo "From $pwd/." > expect.err.super &&
>>>>> -	echo "   $head1..$head2  super      -> origin/super" >>expect.err.super &&
>>>>
>>>> ...as opposed to if we just rolled the generation of this into some
>>>> utility printf function.
>>>
>>> we'd still have to deal with $head1 if we use test_cmp. That's fine for
>>> this test, because it's pretty simple, but it gets pretty janky later
>>> on:
>>>
>>>   @@ -345,20 +339,13 @@ test_expect_success "Recursion picks up all submodules when necessary" '
>>>         git fetch &&
>>>         git checkout -q FETCH_HEAD
>>>       ) &&
>>>   -		head1=$(git rev-parse --short HEAD^) &&
>>>       git add subdir/deepsubmodule &&
>>>       git commit -m "new deepsubmodule" &&
>>>   -		head2=$(git rev-parse --short HEAD) &&
>>>   -		echo "Fetching submodule submodule" > ../expect.err.sub &&
>>>   -		echo "From $pwd/submodule" >> ../expect.err.sub &&
>>>   -		echo "   $head1..$head2  sub        -> origin/sub" >> ../expect.err.sub
>>>   +		git rev-parse --short HEAD >../subhead
>>>     ) &&
>>>   -	head1=$(git rev-parse --short HEAD) &&
>>>     git add submodule &&
>>>     git commit -m "new submodule" &&
>>>   -	head2=$(git rev-parse --short HEAD) &&
>>>   -	echo "From $pwd/." > expect.err.super &&
>>>   -	echo "   $head1..$head2  super      -> origin/super" >> expect.err.super &&
>>>   +	git rev-parse --short HEAD >superhead &&
>>>     (
>>>       cd downstream &&
>>>       git fetch >../actual.out 2>../actual.err
>>>
>>> In this example, we have two $head1 variables in different subshells,
>>> one of which is HEAD, but the other is HEAD^. The reason why we want
>>> HEAD^ isn't obvious (IIRC it's because the submodule upstream is 2
>>> commits ahead because we add the deepsubmodule in a separate commit), in
>>> my opinion, and I got tripped up quite a few times trying to read and
>>> understand the test. That's a lot of effort to spend on irrelevant
>>> information - the test actually cares about what it fetched, not where
>>> the ref used to be.
>>>
>>> So for that reason, I'd prefer to remove test_cmp for this test.
>>
>> I agree that it's pretty irrelevant, but I also think we'd be throwing
>> the baby out with the bath water by entirely doing away with test_cmp
>> here, there's an easier way to do this.
> [...]
>> Instead let's just test once somewhere that when we run submodule
>> fetching that submodules are indeed updated appropriately. Surely other
>> submodule tests will break if the "update" code is made to NOOP, or
>> update to the wrong HEAD>
>>
>> Then for all these test_cmp tests we can just sed-away the
>> $head1..$head2 with something like (untested):
>>
>>     sed -n -e 's/[^.]*\.\.[^.]*/OLD..NEW/g'
>>
>> I.e. let's just skip this entire ceremony with asserting the old/new
>> HEAD unless it's really needed (and then we can probably do it once
>> outside a test_cmp).
>>
>> If you grep through the test suite for "sed" adjacent to "test_cmp"
>> you'll find a lot of such examples of munging the output before
>> test_cmp-ing it.
>
> Makes sense, I hadn't considered this (though I have seen the pattern in
> the test suite, oops). The most compelling argument in favor of this is
> that this could remove a lot of the complexity of verify_fetch_result(),
> which is impeding test readability.
>
>> I.e. none of these tests surely need to test that we updated from
>> $head1..$head2 again and again with the corresponding verbosity in test
>> setup and shelling out to "git rev-parse --short HEAD" or whatever.
>
> I find the converse (we are testing the formatting over and over again)
> less convincing. In these tests, we really are checking for $head2 in
> the stderr to verify that the correct thing was fetched. I'm not
> convinced that we should be relying on _other_ submodule tests to tell
> us that submodule fetch is broken. Which brings me back to the original
> motivation of this patch..
>
>>
>> That's perfectly fine here, since the actual point of the test_cmp is to
>> check the formatting/order etc. of the output itself, not to continually
>> re-assert that submodule updating still works, and that we get the right
>> OIDs.
>
> which is that these tests actually are continually re-asserting the
> submodule updating works correctly in the different circumstances, and
> since we use the stderr to check this, test_cmp adds unwarranted noise.
>
> But you are correct in that the point of test_cmp is to check
> formatting/order etc. There is value in using test_cmp for this purpose,
> and getting rid of it entirely creates a hole in our test coverage.
> (This wouldn't mean that we'd need to use test_cmp _everywhere_ though,
> only that we need to use test_cmp _somewhere_.)
>
> As it stands:
>
> +   test_cmp can improve the readability of the test helpers and
>     debuggability of tests vs grep
> +/- test_cmp can catch formatting changes that are hard to catch
>     otherwise, though at the cost of being sensitive to _any_ formatting
>     changes
> -   test_cmp needs some munging to eliminate unnecessary information
>
> so on the whole, I think it's worth trying to use test_cmp in the test
> helper. We may not _need_ it everywhere, but if it would be nice to use
> it in as many places as possible.

I think whatever you opt to go for here makes sense. I just wanted to
provide the feedback in case it was helpful, i.e. when reading it I
found these conversions a bit odd, wondered if they were strictly needed
etc.

Thanks!