Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

On Mon, Jun 14 2021, Jeff King wrote:

On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

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.

quoted

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/ (local)

quoted

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

quoted

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