Re: [PATCH v2 1/8] progress.c tests: make start/stop verbs on stdin

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Sep 21, 2021 at 01:09:22AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> Change the usage of the "test-tool progress" introduced in
> 2bb74b53a49 (Test the progress display, 2019-09-16) to take command
> like "start" and "stop" on stdin, instead of running them implicitly.
> 
> This makes for tests that are easier to read, since the recipe will
> mirror the API usage, and allows for easily testing invalid usage that
> would yield (or should yield) a BUG(), e.g. providing two "start"
> calls in a row. A subsequent commit will add such stress tests.

Ok. So this is just a readability change, and not a functional change to
the helper, for now.

Or so I thought, but I was surprised to see the usage changing and the
total count moving to stdin. I don't think that's a bad change but the
commit message doesn't mention it in a way that I expected to see it in
the diff.

> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
> index 5d05cbe7894..685c0a7c49a 100644
> --- a/t/helper/test-progress.c
> +++ b/t/helper/test-progress.c
> @@ -22,31 +26,41 @@
>  
>  int cmd__progress(int argc, const char **argv)
>  {
> -	int total = 0;
> -	const char *title;
> +	const char *default_title = "Working hard";
> +	char *detached_title = NULL;
>  	struct strbuf line = STRBUF_INIT;
> -	struct progress *progress;
> +	struct progress *progress = NULL;
>  
>  	const char *usage[] = {
> -		"test-tool progress [--total=<n>] <progress-title>",
> +		"test-tool progress <stdin",
>  		NULL
>  	};
>  	struct option options[] = {
> -		OPT_INTEGER(0, "total", &total, "total number of items"),
>  		OPT_END(),
>  	};
>  
>  	argc = parse_options(argc, argv, NULL, options, usage, 0);
> -	if (argc != 1)
> -		die("need a title for the progress output");
> -	title = argv[0];
> +	if (argc)
> +		usage_with_options(usage, options);

Ok. We lose the args entirely, moving them to stdin lines, and that
cleans up the usage() check. Nice.

>  	progress_testing = 1;
> -	progress = start_progress(title, total);

Getting rid of the implied start. Ok.

>  	while (strbuf_getline(&line, stdin) != EOF) {
>  		char *end;
>  
> -		if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
> +		if (!strcmp(line.buf, "start")) {
> +			progress = start_progress(default_title, 0);
'start' with no args...
> +		} else if (skip_prefix(line.buf, "start ", (const char **) &end)) {
'start 1234'...

Would it be more readable to use strbuf_split_buf() here instead? Maybe
it doesn't fix the need for strtoull() but it could make the parsing
clearer. I did have to think about this one for a bit.

> +			uint64_t total = strtoull(end, &end, 10);
> +			if (*end == '\0') {
> +				progress = start_progress(default_title, total);
> +			} else if (*end == ' ') {
'start 1234 lorem ipsum dolor'. Ok.
> +				free(detached_title);
> +				detached_title = strbuf_detach(&line, NULL);
> +				progress = start_progress(end + 1, total);
> +			} else {
> +				die("invalid input: '%s'\n", line.buf);
> +			}

I wondered why we had to do all this title parsing from scratch now when
we didn't before, but I guess it's because we don't get a nicely
allocated argv[0]. Ok.

> +		} else if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
>  			uint64_t item_count = strtoull(end, &end, 10);
>  			if (*end != '\0')
>  				die("invalid input: '%s'\n", line.buf);
> @@ -63,12 +77,15 @@ int cmd__progress(int argc, const char **argv)
>  				die("invalid input: '%s'\n", line.buf);
>  			progress_test_ns = test_ms * 1000 * 1000;
>  			display_throughput(progress, byte_count);
> -		} else if (!strcmp(line.buf, "update"))
> +		} else if (!strcmp(line.buf, "update")) {
>  			progress_test_force_update();
> -		else
> +		} else if (!strcmp(line.buf, "stop")) {
> +			stop_progress(&progress);
> +		} else {

And 'stop' doesn't take any args. Ok. Do you need the {}?

>  			die("invalid input: '%s'\n", line.buf);
> +		}
>  	}
> -	stop_progress(&progress);
> +	free(detached_title);
>  
>  	return 0;
>  }
> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
> index 22058b503ac..ca96ac1fa55 100755
> --- a/t/t0500-progress-display.sh
> +++ b/t/t0500-progress-display.sh
> @@ -17,6 +17,7 @@ test_expect_success 'simple progress display' '
>  	EOF
>  
>  	cat >in <<-\EOF &&
> +	start 0

Does it need the total arg?

>  	update
>  	progress 1
>  	update
> @@ -88,16 +92,15 @@ Working hard.......2.........3.........4.........5.........6:
>  EOF
>  
>  	cat >in <<-\EOF &&
> -	update
Was it intended to drop the 'update' line here? Does this not change the
content of the test?
> +	start 100000 Working hard.......2.........3.........4.........5.........6
>  	progress 1
>  	update
>  	progress 2
>  	progress 10000
>  	progress 100000
> +	stop
>  	EOF
> -	test-tool progress --total=100000 \
> -		"Working hard.......2.........3.........4.........5.........6" \
> -		<in 2>stderr &&
> +	test-tool progress <in 2>stderr &&
>  
>  	show_cr <stderr >out &&
>  	test_cmp expect out

With whichever nits seem appropriate,

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>