From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Felipe Contreras" <felipe.contreras@gmail.com>,
git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
"John Keeping" <john@keeping.me.uk>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY
Date: Mon, 14 Jun 2021 18:55:03 +0200 [thread overview]
Message-ID: <87r1h4z8k0.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YMdm5XayUfp4/atY@coredump.intra.peff.net>
On Mon, Jun 14 2021, Jeff King wrote:
> On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > I think breaking the test suite is objectively worse than having a few
>> > extra files in the output directory, but to each his own.
>>
>> We've got both in-tree and out-tree things that rely on e.g. the
>> *.counts in that directory to have a 1=1 mapping with "real"
>> tests. E.g. "make aggregate-results".
>
> Indeed. With Felipe's original patch, the "test" target (but not
> "prove") in t/Makefile will report, whether you set
> TEST_OUTPUT_DIRECTORY or not:
>
> failed test(s): t1234 t2345
>
> fixed 0
> success 23243
> failed 2
> broken 221
> total 23647
>
> though curiously it doesn't exit non-zero back to make (usually we'd
> also see the failures from the individual make targets, and barf there).
Odd.
>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> index 2c6e34b9478..29bf67d49bf 100755
>> --- a/t/t0000-basic.sh
>> +++ b/t/t0000-basic.sh
>> @@ -76,6 +76,12 @@ _run_sub_test_lib_test_common () {
>> # this variable, so we need a stable setting under which to run
>> # the sub-test.
>> sane_unset HARNESS_ACTIVE &&
>> +
>> + # These tests should emit no metrics or output that
>> + # would normally go in the "test-results" directory.
>> + TEST_NO_RESULTS_OUTPUT=1 &&
>> + export TEST_NO_RESULTS_OUTPUT &&
>
> I'm OK with this general approach. I do think it would be nice if we let
> the environment supersede the on-disk GIT-BUILD-OPTIONS, which IMHO is
> the real root of the problem (and possibly others), but that may be more
> challenging to get right (I posted a patch earlier, but it does rely on
> stuffing all of "set" into a variable, which makes me concerned some
> less-able shells may complain).
Yeah I don't know and haven't dug into who wants all this combination of
GIT-BUILD-OPTIONS, passing things in the env, or passing things as
paramaters to make (sometimes under the same names).
> It also means that t0000 can't test the results output (since we don't
> write it), but I assume we don't do that now (I didn't actually try
> running with your patch).
Yeah, but only in the trivial wrapper function, you can still write the
test script and check the output yourself.
That's much easier on top of a series to move that into a lib-subtest.sh
that I submitted today:
https://lore.kernel.org/git/cover-0.8-00000000000-20210614T104351Z-avarab@gmail.com/
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 54938c64279..9e9696a3185 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -252,8 +252,14 @@ TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
>> TEST_NAME="$(basename "$0" .sh)"
>> TEST_NUMBER="${TEST_NAME%%-*}"
>> TEST_NUMBER="${TEST_NUMBER#t}"
>> -TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
>> -TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
>> +if test -n "$TEST_NO_RESULTS_OUTPUT"
>> +then
>> + TEST_RESULTS_DIR=/dev/null
>> + TEST_RESULTS_BASE=/dev/null
>> +else
>> + TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
>> + TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
>> +fi
>
> I wondered about this use of /dev/null, since we'd generally use this as
> a directory, and writing to "/dev/null/foo" is going to throw an error.
>
> But...
>
>> TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
>> test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
>> case "$TRASH_DIRECTORY" in
>> @@ -1124,7 +1130,7 @@ test_done () {
>>
>> finalize_junit_xml
>>
>> - if test -z "$HARNESS_ACTIVE"
>> + if test -z "$HARNESS_ACTIVE$TEST_NO_RESULTS_OUTPUT"
>> then
>> mkdir -p "$TEST_RESULTS_DIR"
>
> ...here we would never look at those variables at all, so it is just a
> sentinel that would let us know the assumption has been violated.
>
> We do look at them elsewhere, though (in --tee as you noted, and I think
> for --stress). I'd prefer to notice the "no results" flag explicitly
> there and report something sensible, rather than getting:
If we edit every single current callsite instead of setting it to
something you can't write to then we're setting ourselves up for subtle
bugss when someone uses $TEST_RESULTS_DIR for something else.
> mkdir: cannot create directory ‘/dev/null’: Not a directory
>
> or similar.
Yeah that error sucks, but nobody will see it unless they're hacking on
the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.
In any case, I'll let Felipe decide what, if anything, to do with this
:)
next prev parent reply other threads:[~2021-06-14 17:00 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 17:05 [PATCH] test: fix for TEST_OUTPUT_DIRECTORY Felipe Contreras
2021-06-13 4:42 ` Jeff King
2021-06-13 15:44 ` Felipe Contreras
2021-06-14 7:43 ` Jeff King
2021-06-14 8:39 ` Felipe Contreras
2021-06-14 9:33 ` Ævar Arnfjörð Bjarmason
2021-06-14 14:25 ` Jeff King
2021-06-14 16:55 ` Ævar Arnfjörð Bjarmason [this message]
2021-06-15 11:10 ` Jeff King
2021-06-15 11:21 ` Bagas Sanjaya
2021-06-15 11:23 ` Jeff King
2021-06-15 18:09 ` Felipe Contreras
2021-06-15 17:45 ` Felipe Contreras
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r1h4z8k0.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=carenas@gmail.com \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=john@keeping.me.uk \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).