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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 31, 2022, 12:50 UTC
Message-ID: <221031.86pme86tcg.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y12JfADN/YRr9IaJ@nand.local>

On Sat, Oct 29 2022, Taylor Blau wrote:

Show 31 quoted lines

> 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...