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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2021, 16:22 UTC
Message-ID: <xmqqpmvkyie6.fsf@gitster.g>
In-Reply-To: <YO5iOqWx46UxdhKX@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 39 quoted lines

> 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...