Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

Jeff King wrote:

On Wed, Jun 09, 2021 at 12:05:20PM -0500, Felipe Contreras wrote:

quoted

The test_atexit unit test relies on the specific location of the
generated files.

When TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets
it to pwd, which is two levels under the pwd of the parent unit test,
and the parent can find the generated files just fine.

But when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,
and even though _run_sub_test_lib_test_common correctly overrides it,
when the child script is run, it sources GIT-BUILD-OPTIONS, and
TEST_OUTPUT_DIRECTORY is overridden.

Effectively both the parent and child scripts output to the same
directory.

  make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&
  make -C t t0000-basic.sh

I agree things are broken when TEST_OUTPUT_DIRECTORY is set. We pollute
/tmp/foobar in that case with trash directories, as well as its
test-results/ directory with subtest results (mostly "counts" files).

quoted

On the other hand we could follow the alternate path suggested in
6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):
pass the --root parameter to the child scripts.

The alternate solution works, so let's do that instead.

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.

Using --root fixes the
trash directories, but we still pollute test-results. No tests in t0000
rely on that, but it's still the wrong thing to be doing.

That's true before your patch, as well, though it does make things
slightly worse when TEST_OUTPUT_DIRECTORY isn't set (before in that case
everything worked perfectly, and now it pollutes test-results/, too).

True.

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.

I think whomever usses that script *and* TEST_OUTPUT_DIRECTORY, can
simply do `TEST_OUTPUT_DIRECTORY=foo valgrind/analyze.sh`.

So maybe:

diff --git a/Makefile b/Makefile
index c3565fc0f8..2e25489569 100644
--- a/Makefile
+++ b/Makefile

@@ -2790,9 +2790,6 @@ GIT-BUILD-OPTIONS: FORCE
        @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
        @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
        @echo X=\'$(X)\' >>$@+
-ifdef TEST_OUTPUT_DIRECTORY
-       @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
-endif
 ifdef GIT_TEST_OPTS
        @echo GIT_TEST_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_OPTS)))'\' >>$@+
 endif

diff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh
index 2ffc80f721..378d0a8daa 100755
--- a/t/valgrind/analyze.sh
+++ b/t/valgrind/analyze.sh

@@ -1,8 +1,5 @@
 #!/bin/sh
 
-# Get TEST_OUTPUT_DIRECTORY from GIT-BUILD-OPTIONS if it's there...
-. "$(dirname "$0")/../../GIT-BUILD-OPTIONS"
-# ... otherwise set it to the default value.
 : ${TEST_OUTPUT_DIRECTORY=$(dirname "$0")/..}
 
 output=

-- 
Felipe Contreras