[PATCH 0/3] tests: improve misc run-command, hook, submodule tests

[PATCH 0/3] tests: improve misc run-command, hook, submodule tests

[Ævar Arnfjörð Bjarmason]

These are small tests changes left out of[1], which has since
graduated as 6ae1a6eaf26 (Merge branch 'ab/run-hook-api-cleanup',
2022-10-27). The range-diff is to the relevant part of v2 of that
topic.

I updated the commit message of 2/3, and adjusted 3/3 to make the diff
smaller, but functionally the same as far as how the test works.

These changes have already been reviewed, and the only
(understandable) objection was that they were side-tracks in v2 of [1]
that we could do without in the context of that topic.

1. https://lore.kernel.org/git/cover-v3-00.15-00000000000-20221012T205712Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  hook tests: fix redirection logic error in 96e7225b310
  submodule tests: reset "trace.out" between "grep" invocations
  run-command tests: test stdout of run_command_parallel()

 t/t0061-run-command.sh      | 15 ++++++++++-----
 t/t1800-hook.sh             |  2 +-
 t/t5526-fetch-submodules.sh |  8 ++++++++
 3 files changed, 19 insertions(+), 6 deletions(-)

Range-diff:
1:  bc51dfcb1be = 1:  1ba41a5842c hook tests: fix redirection logic error in 96e7225b310
2:  3027f5587a7 ! 2:  708375e3104 submodule tests: reset "trace.out" between "grep" invocations
    @@ Commit message
         not print out a "9 tasks" line, but let's be paranoid and stop
         implicitly assuming that that's the case.
     
    +    This change was originally left out of 51243f9f0f6 (run-command API:
    +    don't fall back on online_cpus(), 2022-10-12), which added the
    +    ">trace.out" seen at the end of the context.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t5526-fetch-submodules.sh ##
    @@ t/t5526-fetch-submodules.sh: test_expect_success "'fetch.recurseSubmodules=on-de
     +		>trace.out &&
     +
      		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
    --		grep "9 tasks" trace.out
    -+		grep "9 tasks" trace.out &&
    -+		>trace.out &&
    - 	)
    - '
    - 
    + 		grep "9 tasks" trace.out &&
    + 		>trace.out &&
3:  c4923358bbd ! 3:  6d6c2241bf2 run-command tests: test stdout of run_command_parallel()
    @@ t/t0061-run-command.sh: World
      
      test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
     -	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs ungrouped in parallel with more jobs avail
      '
      
      test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
     -	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs ungrouped in parallel with as many jobs as
      '
      
      test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
     -	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
     @@ t/t0061-run-command.sh: asking for a quick stop
      EOF
      
      test_expect_success 'run_command is asked to abort gracefully' '
     -	test-tool run-command run-command-abort 3 false 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-abort 3 false >out 2>err &&
    ++	test-tool run-command run-command-abort 3 false >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
     @@ t/t0061-run-command.sh: no further jobs available
      EOF
      
      test_expect_success 'run_command outputs ' '
     -	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command outputs (ungroup) ' '
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310

[Ævar Arnfjörð Bjarmason]

The tests added in 96e7225b310 (hook: add 'run' subcommand,
2021-12-22) were redirecting to "actual" both in the body of the hook
itself and in the testing code below.

The net result was that the "2>>actual" redirection later in the test
wasn't doing anything. Let's have those redirection do what it looks
like they're doing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1800-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 43fcb7c0bfc..2ef3579fa7c 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '
 test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
 	mkdir my-hooks &&
 	write_script my-hooks/test-hook <<-\EOF &&
-	echo Hook ran $1 >>actual
+	echo Hook ran $1
 	EOF
 
 	cat >expect <<-\EOF &&
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 2/3] submodule tests: reset "trace.out" between "grep" invocations

[Ævar Arnfjörð Bjarmason]

Fix test patterns added in 62104ba14af (submodules: allow parallel
fetching, add tests and documentation, 2015-12-15) and
a028a1930c6 (fetching submodules: respect `submodule.fetchJobs` config
option, 2016-02-29).

In the former case we were leaving a trace.out file at the top-level
for any subsequent tests (there are none, currently). Let's clean the
file up instead.

In the latter case we were testing that a given configuration would
result in "N tasks" in the log, but we were grepping through the log
for all previous such tests, when we really meant to clear the logs
between the "grep" invocations.

In practice this resulted in no logic error, as e.g. "--fetch 7" would
not print out a "9 tasks" line, but let's be paranoid and stop
implicitly assuming that that's the case.

This change was originally left out of 51243f9f0f6 (run-command API:
don't fall back on online_cpus(), 2022-10-12), which added the
">trace.out" seen at the end of the context.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5526-fetch-submodules.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 75da8acf8f4..b9546ef8e5e 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -178,6 +178,7 @@ test_expect_success "submodule.recurse option triggers recursive fetch" '
 '
 
 test_expect_success "fetch --recurse-submodules -j2 has the same output behaviour" '
+	test_when_finished "rm -f trace.out" &&
 	add_submodule_commits &&
 	(
 		cd downstream &&
@@ -705,15 +706,22 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .git
 
 test_expect_success 'fetching submodules respects parallel settings' '
 	git config fetch.recurseSubmodules true &&
+	test_when_finished "rm -f downstream/trace.out" &&
 	(
 		cd downstream &&
 		GIT_TRACE=$(pwd)/trace.out git fetch &&
 		grep "1 tasks" trace.out &&
+		>trace.out &&
+
 		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
 		grep "7 tasks" trace.out &&
+		>trace.out &&
+
 		git config submodule.fetchJobs 8 &&
 		GIT_TRACE=$(pwd)/trace.out git fetch &&
 		grep "8 tasks" trace.out &&
+		>trace.out &&
+
 		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
 		grep "9 tasks" trace.out &&
 		>trace.out &&
-- 
2.38.0.1280.g8136eb6fab2

[PATCH 3/3] run-command tests: test stdout of run_command_parallel()

[Ævar Arnfjörð Bjarmason]

Extend the tests added in c553c72eed6 (run-command: add an
asynchronous parallel child processor, 2015-12-15) to test stdout in
addition to stderr.

When the "ungroup" feature was added in fd3aaf53f71 (run-command: add
an "ungroup" option to run_process_parallel(), 2022-06-07) its tests
were made to test both the stdout and stderr, but these existing tests
were left alone. Let's also exhaustively test our expected output
here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0061-run-command.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 7b5423eebda..e2411f6a9bd 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -130,7 +130,8 @@ World
 EOF
 
 test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
-	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+	test_must_be_empty out &&
 	test_cmp expect actual
 '
 
@@ -141,7 +142,8 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail
 '
 
 test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
-	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+	test_must_be_empty out &&
 	test_cmp expect actual
 '
 
@@ -152,7 +154,8 @@ test_expect_success 'run_command runs ungrouped in parallel with as many jobs as
 '
 
 test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
-	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+	test_must_be_empty out &&
 	test_cmp expect actual
 '
 
@@ -172,7 +175,8 @@ asking for a quick stop
 EOF
 
 test_expect_success 'run_command is asked to abort gracefully' '
-	test-tool run-command run-command-abort 3 false 2>actual &&
+	test-tool run-command run-command-abort 3 false >out 2>actual &&
+	test_must_be_empty out &&
 	test_cmp expect actual
 '
 
@@ -187,7 +191,8 @@ no further jobs available
 EOF
 
 test_expect_success 'run_command outputs ' '
-	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
+	test_must_be_empty out &&
 	test_cmp expect actual
 '
 
-- 
2.38.0.1280.g8136eb6fab2

Re: [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310

[Taylor Blau]

[+cc Emily]

On Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 43fcb7c0bfc..2ef3579fa7c 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '
>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
>  	mkdir my-hooks &&
>  	write_script my-hooks/test-hook <<-\EOF &&
> -	echo Hook ran $1 >>actual
> +	echo Hook ran $1
>  	EOF

Looking reasonable to me. Though let's see what Emily thinks, too...

Thanks,
Taylor

Re: [PATCH 2/3] submodule tests: reset "trace.out" between "grep" invocations

[Taylor Blau]

On Sat, Oct 29, 2022 at 04:59:46AM +0200, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 75da8acf8f4..b9546ef8e5e 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -178,6 +178,7 @@ test_expect_success "submodule.recurse option triggers recursive fetch" '
>  '
>
>  test_expect_success "fetch --recurse-submodules -j2 has the same output behaviour" '
> +	test_when_finished "rm -f trace.out" &&
>  	add_submodule_commits &&
>  	(
>  		cd downstream &&
> @@ -705,15 +706,22 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .git
>
>  test_expect_success 'fetching submodules respects parallel settings' '
>  	git config fetch.recurseSubmodules true &&
> +	test_when_finished "rm -f downstream/trace.out" &&

These two seem OK to me, but...

>  	(
>  		cd downstream &&
>  		GIT_TRACE=$(pwd)/trace.out git fetch &&
>  		grep "1 tasks" trace.out &&
> +		>trace.out &&
> +

I fail to see why these hunks are necessary. If we specify GIT_TRACE,
and don't have a test_must_fail around the execution, then why should we
feel obligated to clean up the trace.out after every execution?

If we really are concerned about not cleaning up after ourselves, how
about writing to a separate file each time?

Thanks,
Taylor

Re: [PATCH 3/3] run-command tests: test stdout of run_command_parallel()

[Taylor Blau]

On Sat, Oct 29, 2022 at 04:59:47AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Extend the tests added in c553c72eed6 (run-command: add an
> asynchronous parallel child processor, 2015-12-15) to test stdout in
> addition to stderr.
>
> When the "ungroup" feature was added in fd3aaf53f71 (run-command: add
> an "ungroup" option to run_process_parallel(), 2022-06-07) its tests
> were made to test both the stdout and stderr, but these existing tests
> were left alone. Let's also exhaustively test our expected output
> here.

Makes sense, this patch looks good. Thanks.

Thanks,
Taylor

Re: [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310

[Ævar Arnfjörð Bjarmason]

On Sat, Oct 29 2022, Taylor Blau wrote:

> [+cc Emily]
>
> On Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
>> index 43fcb7c0bfc..2ef3579fa7c 100755
>> --- a/t/t1800-hook.sh
>> +++ b/t/t1800-hook.sh
>> @@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '
>>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
>>  	mkdir my-hooks &&
>>  	write_script my-hooks/test-hook <<-\EOF &&
>> -	echo Hook ran $1 >>actual
>> +	echo Hook ran $1
>>  	EOF
>
> Looking reasonable to me. Though let's see what Emily thinks, too...

FWIW Junio looked at it back in
https://lore.kernel.org/git/xmqqczh84fpy.fsf@gitster.g/

Re: [PATCH 2/3] submodule tests: reset "trace.out" between "grep" invocations

[Ævar Arnfjörð Bjarmason]

On Sat, Oct 29 2022, Taylor Blau wrote:

> On Sat, Oct 29, 2022 at 04:59:46AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
>> index 75da8acf8f4..b9546ef8e5e 100755
>> --- a/t/t5526-fetch-submodules.sh
>> +++ b/t/t5526-fetch-submodules.sh
>> @@ -178,6 +178,7 @@ test_expect_success "submodule.recurse option triggers recursive fetch" '
>>  '
>>
>>  test_expect_success "fetch --recurse-submodules -j2 has the same output behaviour" '
>> +	test_when_finished "rm -f trace.out" &&
>>  	add_submodule_commits &&
>>  	(
>>  		cd downstream &&
>> @@ -705,15 +706,22 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .git
>>
>>  test_expect_success 'fetching submodules respects parallel settings' '
>>  	git config fetch.recurseSubmodules true &&
>> +	test_when_finished "rm -f downstream/trace.out" &&
>
> These two seem OK to me, but...
>
>>  	(
>>  		cd downstream &&
>>  		GIT_TRACE=$(pwd)/trace.out git fetch &&
>>  		grep "1 tasks" trace.out &&
>> +		>trace.out &&
>> +
>
> I fail to see why these hunks are necessary. If we specify GIT_TRACE,
> and don't have a test_must_fail around the execution, then why should we
> feel obligated to clean up the trace.out after every execution?

Because the trace file isn't clobbered by each git command that
specifies GIT_TRACE, so these tests are basically doing:

	(echo foo; echo bar) >>trace &&
	grep foo trace &&

        (echo bar) >>trace &&
	grep bar trace

Now, it just so happens that the earlier command isn't echoing "bar" to
the file, so this is currently working out.

But it's a bad pattern to be pretending as though you care about the
last output (which was the intent of the test), when really what you're
testing is the combined output of all preceding commands.

This would also be a potenital landmine with "test_must_fail", just
because the command failed we're not guaranteed to have written nothing
to the log (and usually we'd get as far as to write something).

>
> If we really are concerned about not cleaning up after ourselves, how
> about writing to a separate file each time?

Better yet would be to refactor this into a function, set that up and
"test_when_finished" nuke it every time.

But I'm just going for the most minimal change here to not leave this
landmine in place. My 51243f9f0f6 (run-command API: don't fall back on
online_cpus(), 2022-10-12) already started using this pattern, and is on
"master". I think a good incremental step is to do the bare minimum to
do the same for the rest, and guard any subsequent test from being
affected by the "trace.out".

So, unless you insist I'd rather just change nothing about this series,
and not get stuck in various re-rolls/commentary about what's the ideal
refactoring, once we're starting to do that anyway...

Re: [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310

[Taylor Blau]

On Mon, Oct 31, 2022 at 01:44:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Sat, Oct 29 2022, Taylor Blau wrote:
>
> > [+cc Emily]
> >
> > On Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> >> index 43fcb7c0bfc..2ef3579fa7c 100755
> >> --- a/t/t1800-hook.sh
> >> +++ b/t/t1800-hook.sh
> >> @@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '
> >>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
> >>  	mkdir my-hooks &&
> >>  	write_script my-hooks/test-hook <<-\EOF &&
> >> -	echo Hook ran $1 >>actual
> >> +	echo Hook ran $1
> >>  	EOF
> >
> > Looking reasonable to me. Though let's see what Emily thinks, too...
>
> FWIW Junio looked at it back in
> https://lore.kernel.org/git/xmqqczh84fpy.fsf@gitster.g/

Good, though I would like some feedback from Emily, too, seeing as most
of this code blames back to her.

Thanks,
Taylor

Re: [PATCH 2/3] submodule tests: reset "trace.out" between "grep" invocations

[Taylor Blau]

On Mon, Oct 31, 2022 at 01:50:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >>  	(
> >>  		cd downstream &&
> >>  		GIT_TRACE=$(pwd)/trace.out git fetch &&
> >>  		grep "1 tasks" trace.out &&
> >> +		>trace.out &&
> >> +
> >
> > I fail to see why these hunks are necessary. If we specify GIT_TRACE,
> > and don't have a test_must_fail around the execution, then why should we
> > feel obligated to clean up the trace.out after every execution?
>
> Because the trace file isn't clobbered by each git command that
> specifies GIT_TRACE, so these tests are basically doing:
>
> 	(echo foo; echo bar) >>trace &&
> 	grep foo trace &&
>
>         (echo bar) >>trace &&
> 	grep bar trace
>
> Now, it just so happens that the earlier command isn't echoing "bar" to
> the file, so this is currently working out.

Ah, nicely explained. This is new to me, since I thought the behavior of
both GIT_TRACE and GIT_TRACE2 was to clobber the file.

> But it's a bad pattern to be pretending as though you care about the
> last output (which was the intent of the test), when really what you're
> testing is the combined output of all preceding commands.
>
> This would also be a potenital landmine with "test_must_fail", just
> because the command failed we're not guaranteed to have written nothing
> to the log (and usually we'd get as far as to write something).

Yeah, both make sense, thanks.

Thanks,
Taylor