Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: "Jeff King" <peff@peff.net>,
	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 11:33:12 +0200	[thread overview]
Message-ID: <8735tk22if.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <60c715dd9939e_436208f3@natae.notmuch>


On Mon, Jun 14 2021, Felipe Contreras wrote:

> Jeff King wrote:
>> On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:
>> 
>> > > Unfortunately, this isn't a complete solution.
>> > 
>> > Software will never be perfect.
>> > 
>> > We don't need to wait for a perfect solution, all we need is something
>> > better than the current siuation.
>> 
>> Sure, but if you don't fully understand the situation (e.g., that --root
>> and TEST_OUTPUT_DIRECTORY are not equivalent), then you may end up
>> revisiting the incomplete fix later,
>
> The fact that you may end up revisiting a solution is a fact for *all*
> changes (including 2d14e13c56 (test output: respect
> $TEST_OUTPUT_DIRECTORY, 2013-04-29)).
>
>> or even making things worse (as this patch did).
>
> 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".

>> > > I think solving the whole issue would require a mechanism for passing
>> > > TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an
>> > > environment variable or the command-line).
>> > 
>> > Why do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking
>> > for a reason there's 2d14e13c56 (test output: respect
>> > $TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for
>> > valgrind/analyze.sh.
>> > 
>> > I don't know who uses that script, or how. There's no documentaion,
>> > nothing on the mailing list, and nothing found on Google.
>> 
>> Perhaps 268fac6919 (Add a script to coalesce the valgrind outputs,
>> 2009-02-04) is enlightening.
>
> That makes it clearer.
>
>> I don't know if anybody still uses it these days, though. I suspect it's
>> outlived its usefulness, in that we would typically not have any
>> valgrind errors at all (so coalescing them is not that interesting).
>> 
>> Possibly folks investigating leak-checking via valgrind could find it
>> useful, but even there I think LSan is a much better path forward.
>
> Yeah, but even if they do run this tool, they can set
> TEST_OUTPUT_DIRECTORY manually.
>
> The needs of the few should not otweight needs of the many.

Do we need to bring Spock into this?:)

I think the following alternate/on-top patch (which you should feel free
to squash in with my SOB if you agree) solves the issue both of you are
noting.

It will barf if you run the tests under e.g. --tee with that environment
variable, but that's intentional. It's better to loudly error if we
don't have the expected test-results than to silently write in the wrong
place.

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 &&
+
 		cd "$name" &&
 		write_script "$name.sh" "$TEST_SHELL_PATH" <<-EOF &&
 		test_description='$descr (run in sub test-lib)
@@ -89,14 +95,12 @@ _run_sub_test_lib_test_common () {
 		EOF
 		cat >>"$name.sh" &&
 		export TEST_DIRECTORY &&
-		TEST_OUTPUT_DIRECTORY=$(pwd) &&
-		export TEST_OUTPUT_DIRECTORY &&
 		sane_unset GIT_TEST_FAIL_PREREQS &&
 		if test -z "$neg"
 		then
-			./"$name.sh" "$@" >out 2>err
+			./"$name.sh" --root="$(pwd)" "$@" >out 2>err
 		else
-			! ./"$name.sh" "$@" >out 2>err
+			! ./"$name.sh" --root="$(pwd)" "$@" >out 2>err
 		fi
 	)
 }
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
 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"
 
