From: "Ævar Arnfjörð Bjarmason" <firstname.lastname@example.org>
To: Taylor Blau <email@example.com>
Cc: firstname.lastname@example.org, Junio C Hamano <email@example.com>,
Phillip Wood <firstname.lastname@example.org>,
Calvin Wan <email@example.com>
Subject: Re: [PATCH 2/3] submodule tests: reset "trace.out" between "grep" invocations
Date: Mon, 31 Oct 2022 13:50:07 +0100 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
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...
next prev parent reply other threads:[~2022-10-31 13:00 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-29 2:59 [PATCH 0/3] tests: improve misc run-command, hook, submodule tests Ævar Arnfjörð Bjarmason
2022-10-29 2:59 ` [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310 Ævar Arnfjörð Bjarmason
2022-10-29 20:11 ` Taylor Blau
2022-10-31 12:44 ` Ævar Arnfjörð Bjarmason
2022-10-31 23:56 ` Taylor Blau
2022-10-29 2:59 ` [PATCH 2/3] submodule tests: reset "trace.out" between "grep" invocations Ævar Arnfjörð Bjarmason
2022-10-29 20:13 ` Taylor Blau
2022-10-31 12:50 ` Ævar Arnfjörð Bjarmason [this message]
2022-10-31 23:58 ` Taylor Blau
2022-10-29 2:59 ` [PATCH 3/3] run-command tests: test stdout of run_command_parallel() Ævar Arnfjörð Bjarmason
2022-10-29 20:15 ` Taylor Blau
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).