Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Jul 14, 2021 at 02:54:03AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> -extract_haves () {
>> -	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
>> -}
>> -
>>  test_expect_success 'with core.alternateRefsCommand' '
>>  	write_script fork/alternate-refs <<-\EOF &&
>>  		git --git-dir="$1" for-each-ref \
>> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
>>  			refs/heads/public/
>>  	EOF
>>  	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
>> -	git rev-parse public/branch >expect &&
>> -	printf "0000" | git receive-pack fork >actual &&
>> -	extract_haves <actual >actual.haves &&
>> -	test_cmp expect actual.haves
>> +
>> +	test-tool pkt-line pack >in <<-\EOF &&
>> +	0000
>> +	EOF
>> +
>> +	cat >expect <<-EOF &&
>> +	$(git rev-parse main) refs/heads/main
>> +	$(git rev-parse base) refs/tags/base
>> +	$(git rev-parse public) .have
>> +	0000
>> +	EOF
>> +
>> +	git receive-pack fork >out <in &&
>> +	test-tool pkt-line unpack <out >actual &&
>
> I don't think extracting the haves themselves (and stripping ".have"
> from the output) adds much verbosity at all. Wouldn't it be:
>
>   test-tool pkt-line unpack <out >actual &&
>   perl -lne '/^(\S+) \.have/ and print $1' <actual >actual.haves
>
> (in fact, it might be quite nice to leave extract_haves as-is changing
> depacketize for 'test-tool pkt-line unpack').

I tend to agree with you ane Peff, after reading the resulting tests
myself.

Specifically I have problem with this line of reasoning:

    The conversion away from extract_haves() in ... isn't strictly
    required for this migration, but in this case it's easy enough
    to test_cmp the whole output, so let's just do that.

as if using test_cmp to compare the whole output is always a better
way to test, which is far from cut-and-dried clear and obvious.  The
default ought to be to keep the original behaviour, unless you can
clearly convince others that either (1) the new way is better, or
(2) keeping the old way is too hard and the cost for doing so
outweighs the benefit.

While there certainly is some value in end-to-end tests, they
inevitably become brittle.  We prefer focused tests that verify
things we _care_ about, while keeping the expected vector unaffected
by future changes in details that do not matter in what is being
tested.  If we are interested in ".have"s, we shouldn't be affected
by irrelevant details like what other branches and tags appear in
the output and in what order, for example.

Of course, if there is a solid reason why we would care not just
".have" but other details, it is perfectly justifiable thing to do
to update the tests, but we'd want to see (1) such an unrelated
change done in a separate step and (2) with its own justification.

So...