Re: [PATCH 1/7] progress: introduce GIT_TEST_CHECK_PROGRESS to verify progress counters
flat view
From: Taylor Blau <hidden>
Date: 2021-06-22 15:55:25
On Sun, Jun 20, 2021 at 10:02:57PM +0200, SZEDER Gábor wrote:
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+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