Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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
:)