Re: [PATCH 1/8] t5558: add tests for creationToken heuristic

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 17, 2023, 21:00 UTC
Message-ID: <be447e59-26be-75b1-65b4-4e976a2e28d4@github.com>
In-Reply-To: <4d13cea1-e9f6-381f-14ef-8a5a645e2a8f@github.com>

On 1/17/2023 1:17 PM, Victoria Dye wrote:

> Derrick Stolee via GitGitGadget wrote:

Show 18 quoted lines

>> +	for b in 1 2 3 4
>> +	do
>> +		test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||
>> +			return 1
>> +	done
> 
> Because the current state of bundle list handling is equivalent to "no
> heuristic", this pre-existing test is just updated to verify all bundles are
> downloaded. This isn't new behavior, but it'll be relevant to compare with
> the behavior of the 'creationToken' heuristic. 
> 
> I was going to ask how the tests verify that *only* the expected bundles are
> downloaded, and it looks like later patches [1] handle that with
> '! test_bundle_downloaded' checks. That approach seems a bit fragile (if a
> bundle's name doesn't match the '! test_bundle_downloaded' check for some
> reason, the bundle can be either downloaded or not with no effect on the
> test result). Would something like a 'test_downloaded_bundle_count' work
> instead?

Or, perhaps we could check the exact list _and order_ using this slightly more generic helper?

# Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs
# sent to git-remote-https child processes.
test_remote_https_urls() {
	grep -e '"event":"child_start".*"argv":\["git-remote-https",".*"\]' |
		sed -e 's/{"event":"child_start".*"argv":\["git-remote-https","//g' \
		    -e 's/"\]}//g'
}

With a test example looking a lot like this:

	cat >expect <<-EOF &&
	$HTTPD_URL/newest.bundle
	$HTTPD_URL/new.bundle
	$HTTPD_URL/everything.bundle
	EOF

	test_remote_https_urls <trace-clone.txt >actual &&
	test_cmp expect actual

Thanks for the inspiration.

Show 43 quoted lines

>> +test_expect_success 'clone bundle list (http, creationToken)' '
>> +	cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
>> +	cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
>> +	[bundle]
>> +		version = 1
>> +		mode = all
>> +		heuristic = creationToken
>> +
>> +	[bundle "bundle-1"]
>> +		uri = bundle-1.bundle
>> +		creationToken = 1
>> +
>> +	[bundle "bundle-2"]
>> +		uri = bundle-2.bundle
>> +		creationToken = 2
>> +
>> +	[bundle "bundle-3"]
>> +		uri = bundle-3.bundle
>> +		creationToken = 3
>> +
>> +	[bundle "bundle-4"]
>> +		uri = bundle-4.bundle
>> +		creationToken = 4
>> +	EOF
>> +
>> +	git clone --bundle-uri="$HTTPD_URL/bundle-list" . clone-list-http-2 &&
>> +
>> +	git -C clone-from for-each-ref --format="%(objectname)" >oids &&
>> +	git -C clone-list-http-2 cat-file --batch-check <oids
> 
> This test looks like the one that was updated above, but adds the
> 'creationToken' heuristic key. However, the 'test_bundle_downloaded' check
> isn't included - if it were, it would need to verify that all bundles were
> downloaded, with the heuristic being ignored, all bundles will be downloaded
> (which isn't consistent with what the 'creationToken' heuristic will
> *eventually* do).
> 
> As a matter of personal preference (so no pressure to change if you
> disagree), I find this test in its current state a bit misleading; because
> it's a 'test_expect_success' and there's no "NEEDSWORK" or "TODO", I could
> easily assume that cloning from a bundle list with the 'creationToken'
> heuristic is working as-intended at this point (that is, there's no
> indication that it's not implemented).

It's true that it's not implemented right now, and it is a bit misleading because of that. At clone time, the only thing that will change with the implementation is possibly the order of the files being downloaded (and the order is not predictable before that implementation).

The restriction of _not_ downloading some files comes only for the 'git fetch' implementation, so these test changes are only foundations for those future tests.

The only benefit of having these tests right now is that we get some demonstration that the existing implementation ignores unknown properties in the bundle list.

Show 5 quoted lines

> If you did want to change it, adding a 'NEEDSWORK' comment, changing to
> 'test_expect_failure' & including the appropriate 'test_bundle_downloaded'
> check, or moving this test to the patch where the heuristic is implemented
> would mitigate any confusion. That said, this "issue" is resolved by the
> end of the series anyway, so it's really a low priority fix.

I think if we wanted to go this route, we could do it with the "download the bundles in this order" check, or possibly by adding the 'git fetch' behavior into the test at this point. I'll consider these options for v2.

Thanks, -Stolee