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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 22, 2021, 15:55 UTC
Message-ID: <YNIH6FD9NKLc9zYl@nand.local>
In-Reply-To: <20210620200303.2328957-2-szeder.dev@gmail.com>

On Sun, Jun 20, 2021 at 10:02:57PM +0200, SZEDER Gábor wrote:

Show 6 quoted lines

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

Show 13 quoted lines

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

Show 15 quoted lines

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