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

[Victoria Dye <vdye@github.com>]

Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> As documented in the bundle URI design doc in 2da14fad8fe (docs:
> document bundle URI standard, 2022-08-09), the 'creationToken' member of
> a bundle URI allows a bundle provider to specify a total order on the
> bundles.
> 
> Future changes will allow the Git client to understand these members and
> modify its behavior around downloading the bundles in that order. In the
> meantime, create tests that add creation tokens to the bundle list. For
> now, the Git client correctly ignores these unknown keys.
> 
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  t/t5558-clone-bundle-uri.sh | 52 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 9155f31fa2c..328caeeae9a 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -284,7 +284,17 @@ test_expect_success 'clone HTTP bundle' '
>  	test_config -C clone-http log.excludedecoration refs/bundle/
>  '
>  
> +# usage: test_bundle_downloaded <bundle-name> <trace-file>
> +test_bundle_downloaded () {
> +	cat >pattern <<-EOF &&
> +	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/$1"\]
> +	EOF
> +	grep -f pattern "$2"
> +}
> +
>  test_expect_success 'clone bundle list (HTTP, no heuristic)' '
> +	test_when_finished rm -f trace*.txt &&
> +
>  	cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
>  	cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
>  	[bundle]
> @@ -304,12 +314,19 @@ test_expect_success 'clone bundle list (HTTP, no heuristic)' '
>  		uri = $HTTPD_URL/bundle-4.bundle
>  	EOF
>  
> -	git clone --bundle-uri="$HTTPD_URL/bundle-list" \
> +	GIT_TRACE2_EVENT="$(pwd)/trace-clone.txt" \
> +		git clone --bundle-uri="$HTTPD_URL/bundle-list" \
>  		clone-from clone-list-http  2>err &&
>  	! grep "Repository lacks these prerequisite commits" err &&
>  
>  	git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> -	git -C clone-list-http cat-file --batch-check <oids
> +	git -C clone-list-http cat-file --batch-check <oids &&
> +
> +	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?

-------- 8< --------

diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index 0604d721f1..b2f55dd983 100755
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -292,6 +292,16 @@ test_bundle_downloaded () {
 	grep -f pattern "$2"
 }
 
+test_download_bundle_count () {
+	cat >exclude <<-EOF &&
+	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/bundle-list"\]
+	EOF
+	cat >pattern <<-EOF &&
+	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/.*"\]
+	EOF
+	test $(grep -f pattern "$2" | grep -v -f exclude | wc -l) -eq "$1"
+}
+
 test_expect_success 'clone bundle list (HTTP, no heuristic)' '
 	test_when_finished rm -f trace*.txt &&
 
@@ -322,6 +332,7 @@ test_expect_success 'clone bundle list (HTTP, no heuristic)' '
 	git -C clone-from for-each-ref --format="%(objectname)" >oids &&
 	git -C clone-list-http cat-file --batch-check <oids &&
 
+	test_download_bundle_count 4 trace-clone.txt &&
 	for b in 1 2 3 4
 	do
 		test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||

-------- >8 --------

[1] https://lore.kernel.org/git/51f210ddeb46fb06e885dc384a486c4bb16ad8cd.1673037405.git.gitgitgadget@gmail.com/

>  '
>  
>  test_expect_success 'clone bundle list (HTTP, any mode)' '
> @@ -350,6 +367,37 @@ test_expect_success 'clone bundle list (HTTP, any mode)' '
>  	test_cmp expect actual
>  '
>  
> +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). 

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.

> +'
> +
>  # Do not add tests here unless they use the HTTP server, as they will
>  # not run unless the HTTP dependencies exist.
>