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@xxxxxxxxx/
>> 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
:)
