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
