Re: [PATCH 2/3] t: directly test parse_pathspec_file()

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 30, 2019, 18:52 UTC
Message-ID: <xmqq8smthcib.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <27383a5b084b5e68152b08eb96fb4ddaf6d87f82.1577727747.git.gitgitgadget@gmail.com>

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 17 quoted lines

> diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
> new file mode 100644
> index 0000000000..e7f525feb9
> --- /dev/null
> +++ b/t/helper/test-parse-pathspec-file.c
> @@ -0,0 +1,34 @@
> +#include "test-tool.h"
> +#include "parse-options.h"
> +#include "pathspec.h"
> +#include "gettext.h"
> + ...
> +	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
> +			    pathspec_file_nul);
> +
> +	for (i = 0; i < pathspec.nr; i++) {
> +		printf("%s\n", pathspec.items[i].original);
> +	}

No need for {} around a single statement block.

Show 20 quoted lines

> diff --git a/t/t0067-parse_pathspec_file.sh b/t/t0067-parse_pathspec_file.sh
> new file mode 100755
> index 0000000000..df7b319713
> --- /dev/null
> +++ b/t/t0067-parse_pathspec_file.sh
> @@ -0,0 +1,89 @@
> +#!/bin/sh
> +
> +test_description='Test parse_pathspec_file()'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'one item from stdin' '
> +	echo fileA.t | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	fileA.t
> +	EOF
> +	test_cmp expect actual
> +'

The use of the blank lines are somewhat inconsistent here.

> + ...
> +test_expect_success 'NUL delimiters' '
> +	printf "fileA.t\0fileB.t\0" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&

Fold line immediately after the pipe (same for the earlier and later ones).

Show 6 quoted lines

> +	cat >expect <<-\EOF &&
> +	fileA.t
> +	fileB.t
> +	EOF
> +	test_cmp expect actual
> +'

If you want to have a gap between the steps, i.e. "capturing the actual output", "creating the ideal output", and "seeing how they differ", using blank like this is OK:

	printf "fileA.t\0fileB.t\0" |
	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&

	cat >expect <<-\EOF &&
	fileA.t
	fileB.t
	EOF

	test_cmp expect actual

I thought we typically prepare the ideal output sample before capturing the actual output, so if we follow that convention, the above becomes

	cat >expect <<-\EOF &&
	fileA.t
	fileB.t
	EOF

	printf "fileA.t\0fileB.t\0" |
	test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&

	test_cmp expect actual

Show 22 quoted lines

> +test_expect_success 'quotes' '
> +	# shell  takes \\\\101 and spits \\101
> +	# printf takes   \\101 and spits  \101
> +	# git    takes    \101 and spits     A
> +	printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	fileA.t
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--pathspec-file-nul takes quotes literally' '
> +	# shell  takes \\\\101 and spits \\101
> +	# printf takes   \\101 and spits  \101
> +	printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	"file\101.t"
> +	EOF
> +	test_cmp expect actual
> +'

Testing low level machinery like this is of course a good idea, in addition to the end-to-end tests that make sure that the machinery is called correctly from the higher layer.

Thanks.