Subject: Re: [PATCH 1/7] progress: introduce GIT_TEST_CHECK_PROGRESS to verify progress counters

From: Taylor Blau <[email-masked]>
Date: Tue, 22 Jun 2021 11:55:20 -0400

On Sun, Jun 20, 2021 at 10:02:57PM +0200, SZEDER Gábor wrote:
> +	progress->last_value = n;
> +
>  	if (progress->delay && (!progress_update || --progress->delay))
>  		return;
>
> -	progress->last_value = n;

Makes sense, and thanks for explaining it explicitly in the patch
message.

>  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
>  	if (progress->total) {
>  		unsigned percent = n * 100 / progress->total;
> @@ -252,7 +255,11 @@ void display_progress(struct progress *progress, uint64_t n)
>  static struct progress *start_progress_delay(const char *title, uint64_t total,
>  					     unsigned delay, unsigned sparse)
>  {
> -	struct progress *progress = xmalloc(sizeof(*progress));
> +	struct progress *progress;
> +
> +	test_check_progress = git_env_bool("GIT_TEST_CHECK_PROGRESS", 0);
> +
> +	progress = xmalloc(sizeof(*progress));

Ævar noted below, I think, but this cleanup to move the xmalloc() call
to after reading $GIT_TEST_CHECK_PROGRESS is unnecessary.

> +test_expect_success 'GIT_TEST_CHECK_PROGRESS catches non-matching total' '
> +	cat >in <<-\EOF &&
> +	progress 1
> +	progress 2
> +	progress 4
> +	EOF
> +
> +	test_must_fail env GIT_TEST_CHECK_PROGRESS=1 \
> +		test-tool progress --total=3 "Not enough" <in 2>stderr &&
> +	grep "BUG:.*total progress does not match" stderr &&
> +
> +	test_must_fail env GIT_TEST_CHECK_PROGRESS=1 \
> +		test-tool progress --total=5 "Too much" <in 2>stderr &&
> +	grep "BUG:.*total progress does not match" stderr
> +'

This and the below test are both good to see. I wondered briefly whether
or not it would be worth adding a test to check that the "progress does
not match" triggers even when we have a non-zero delay, like:

    test_must_fail env GIT_PROGRESS_DELAY=100 GIT_TEST_CHECK_PROGRESS=1 \
      test-tool progress --total=5 "Too much" <in 2>stderr &&
    grep "BUG:.*total progress does not match" stderr

But it's not helpful, because GIT_PROGRESS_DELAY is already 2 by
default, and we unset GIT_* environment variables (including
GIT_PROGRESS_DELAY) except a few which are left alone.

So we are already testing this case implicitly. It may be worth making
it explicit, and/or testing the case where GIT_PROGRESS_DELAY=0, but I
do not feel strongly about it. Besides, I would much rather err on the
side of testing cases we feel are legitimately interesting, rather than
filling in a grid of all possible combinations, including uninteresting
ones.

Thanks,
Taylor