{"thread":{"id":"55889","subject":"[PATCH] test: fix for TEST_OUTPUT_DIRECTORY","startedAt":"2021-06-09T17:05:39Z","lastAt":"2021-06-15T18:09:54Z","messageCount":13,"participants":["Felipe Contreras","Jeff King","Ævar Arnfjörð Bjarmason","Bagas Sanjaya"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"426904","messageId":"20210609170520.67014-1-felipe.contreras@gmail.com","threadId":"55889","inReplyTo":null,"subject":"[PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T17:05:20Z","receivedAt":"2021-06-09T17:05:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The test_atexit unit test relies on the specific location of the\ngenerated files.\n\nWhen TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets\nit to pwd, which is two levels under the pwd of the parent unit test,\nand the parent can find the generated files just fine.\n\nBut when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,\nand even though _run_sub_test_lib_test_common correctly overrides it,\nwhen the child script is run, it sources GIT-BUILD-OPTIONS, and\nTEST_OUTPUT_DIRECTORY is overridden.\n\nEffectively both the parent and child scripts output to the same\ndirectory.\n\n  make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&\n  make -C t t0000-basic.sh\n\nWe could simply revert 2d14e13c56 (test output: respect\n$TEST_OUTPUT_DIRECTORY, 2013-04-29), but presumably it was done for some\nreason.\n\nOn the other hand we could follow the alternate path suggested in\n6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):\npass the --root parameter to the child scripts.\n\nThe alternate solution works, so let's do that instead.\n\nPresumably this was broken since 900721e15c (test-lib: introduce\n'test_atexit', 2019-03-13).\n\nCc: John Keeping <john@keeping.me.uk>\nCc: Jeff King <peff@peff.net>\nCc: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t0000-basic.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 705d62cc27..16b70ef940 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -93,14 +93,12 @@ _run_sub_test_lib_test_common () {\n \t\tEOF\n \t\tcat >>\"$name.sh\" &&\n \t\texport TEST_DIRECTORY &&\n-\t\tTEST_OUTPUT_DIRECTORY=$(pwd) &&\n-\t\texport TEST_OUTPUT_DIRECTORY &&\n \t\tsane_unset GIT_TEST_FAIL_PREREQS &&\n \t\tif test -z \"$neg\"\n \t\tthen\n-\t\t\t./\"$name.sh\" \"$@\" >out 2>err\n+\t\t\t./\"$name.sh\" --root=\"$(pwd)\" \"$@\" >out 2>err\n \t\telse\n-\t\t\t! ./\"$name.sh\" \"$@\" >out 2>err\n+\t\t\t! ./\"$name.sh\" --root=\"$(pwd)\" \"$@\" >out 2>err\n \t\tfi\n \t)\n }\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"427210","messageId":"YMWMuDbctae7tF6J@coredump.intra.peff.net","threadId":"55889","inReplyTo":"20210609170520.67014-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-13T04:42:32Z","receivedAt":"2021-06-13T04:42:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 09, 2021 at 12:05:20PM -0500, Felipe Contreras wrote:\n\n> The test_atexit unit test relies on the specific location of the\n> generated files.\n> \n> When TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets\n> it to pwd, which is two levels under the pwd of the parent unit test,\n> and the parent can find the generated files just fine.\n> \n> But when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,\n> and even though _run_sub_test_lib_test_common correctly overrides it,\n> when the child script is run, it sources GIT-BUILD-OPTIONS, and\n> TEST_OUTPUT_DIRECTORY is overridden.\n> \n> Effectively both the parent and child scripts output to the same\n> directory.\n> \n>   make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&\n>   make -C t t0000-basic.sh\n\nI agree things are broken when TEST_OUTPUT_DIRECTORY is set. We pollute\n/tmp/foobar in that case with trash directories, as well as its\ntest-results/ directory with subtest results (mostly \"counts\" files).\n\n> On the other hand we could follow the alternate path suggested in\n> 6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):\n> pass the --root parameter to the child scripts.\n> \n> The alternate solution works, so let's do that instead.\n\nUnfortunately, this isn't a complete solution. Using --root fixes the\ntrash directories, but we still pollute test-results. No tests in t0000\nrely on that, but it's still the wrong thing to be doing.\n\nThat's true before your patch, as well, though it does make things\nslightly worse when TEST_OUTPUT_DIRECTORY isn't set (before in that case\neverything worked perfectly, and now it pollutes test-results/, too).\n\nI think solving the whole issue would require a mechanism for passing\nTEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an\nenvironment variable or the command-line).\n\nAlternatively, it would be nice if GIT-BUILD-OPTIONS didn't override the\nenvironment. That would fix this problem, prevent similar ones in the\nfuture, and I think would do the right thing if a caller decided to do\nsomething like:\n\n  PERL_PATH=/some/other/perl ./t1234-foo.sh\n\nWe might be able to write GIT-BUILD-OPTIONS to use ${foo:=}\ndefault-value expansion, but I'm not sure if there are any non-shell\nreaders of the file (I don't see any, but I feel like we've run afoul of\nthis before). Something like this might work:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex adaf03543e..731dd41d9c 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -54,7 +54,10 @@ then\n \techo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'\n \texit 1\n fi\n+# let our env take precedence over build options\n+current_env=$(set)\n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n+eval \"$current_env\"\n export PERL_PATH SHELL_PATH\n \n # Disallow the use of abbreviated options in the test suite by default\n\nbut I think more fixes are required for $TEST_OUTPUT_DIRECTORY, since we\nset it so early in the script (before we read GIT-BUILD-OPTIONS, so at\nthat point we have no idea if it came from the environment or our\ndefault settings).\n\n-Peff\n"},{"id":"427238","messageId":"60c627cac29b3_41f45208a7@natae.notmuch","threadId":"55889","inReplyTo":"YMWMuDbctae7tF6J@coredump.intra.peff.net","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T15:44:10Z","receivedAt":"2021-06-13T15:45:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Wed, Jun 09, 2021 at 12:05:20PM -0500, Felipe Contreras wrote:\n> \n> > The test_atexit unit test relies on the specific location of the\n> > generated files.\n> > \n> > When TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets\n> > it to pwd, which is two levels under the pwd of the parent unit test,\n> > and the parent can find the generated files just fine.\n> > \n> > But when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,\n> > and even though _run_sub_test_lib_test_common correctly overrides it,\n> > when the child script is run, it sources GIT-BUILD-OPTIONS, and\n> > TEST_OUTPUT_DIRECTORY is overridden.\n> > \n> > Effectively both the parent and child scripts output to the same\n> > directory.\n> > \n> >   make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&\n> >   make -C t t0000-basic.sh\n> \n> I agree things are broken when TEST_OUTPUT_DIRECTORY is set. We pollute\n> /tmp/foobar in that case with trash directories, as well as its\n> test-results/ directory with subtest results (mostly \"counts\" files).\n> \n> > On the other hand we could follow the alternate path suggested in\n> > 6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):\n> > pass the --root parameter to the child scripts.\n> > \n> > The alternate solution works, so let's do that instead.\n> \n> Unfortunately, this isn't a complete solution.\n\nSoftware will never be perfect.\n\nWe don't need to wait for a perfect solution, all we need is something\nbetter than the current siuation.\n\n> Using --root fixes the\n> trash directories, but we still pollute test-results. No tests in t0000\n> rely on that, but it's still the wrong thing to be doing.\n> \n> That's true before your patch, as well, though it does make things\n> slightly worse when TEST_OUTPUT_DIRECTORY isn't set (before in that case\n> everything worked perfectly, and now it pollutes test-results/, too).\n\nTrue.\n\n> I think solving the whole issue would require a mechanism for passing\n> TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an\n> environment variable or the command-line).\n\nWhy do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking\nfor a reason there's 2d14e13c56 (test output: respect\n$TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for\nvalgrind/analyze.sh.\n\nI don't know who uses that script, or how. There's no documentaion,\nnothing on the mailing list, and nothing found on Google.\n\nI think whomever usses that script *and* TEST_OUTPUT_DIRECTORY, can\nsimply do `TEST_OUTPUT_DIRECTORY=foo valgrind/analyze.sh`.\n\nSo maybe:\n\ndiff --git a/Makefile b/Makefile\nindex c3565fc0f8..2e25489569 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2790,9 +2790,6 @@ GIT-BUILD-OPTIONS: FORCE\n        @echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n        @echo DC_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_SHA1)))'\\' >>$@+\n        @echo X=\\'$(X)\\' >>$@+\n-ifdef TEST_OUTPUT_DIRECTORY\n-       @echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n-endif\n ifdef GIT_TEST_OPTS\n        @echo GIT_TEST_OPTS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_OPTS)))'\\' >>$@+\n endif\ndiff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh\nindex 2ffc80f721..378d0a8daa 100755\n--- a/t/valgrind/analyze.sh\n+++ b/t/valgrind/analyze.sh\n@@ -1,8 +1,5 @@\n #!/bin/sh\n \n-# Get TEST_OUTPUT_DIRECTORY from GIT-BUILD-OPTIONS if it's there...\n-. \"$(dirname \"$0\")/../../GIT-BUILD-OPTIONS\"\n-# ... otherwise set it to the default value.\n : ${TEST_OUTPUT_DIRECTORY=$(dirname \"$0\")/..}\n \n output=\n\n-- \nFelipe Contreras\n"},{"id":"427277","messageId":"YMcIv7q1ctyJ0EZn@coredump.intra.peff.net","threadId":"55889","inReplyTo":"60c627cac29b3_41f45208a7@natae.notmuch","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-14T07:43:59Z","receivedAt":"2021-06-14T07:44:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:\n\n> > Unfortunately, this isn't a complete solution.\n> \n> Software will never be perfect.\n> \n> We don't need to wait for a perfect solution, all we need is something\n> better than the current siuation.\n\nSure, but if you don't fully understand the situation (e.g., that --root\nand TEST_OUTPUT_DIRECTORY are not equivalent), then you may end up\nrevisiting the incomplete fix later, or even making things worse (as\nthis patch did).\n\n> > I think solving the whole issue would require a mechanism for passing\n> > TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an\n> > environment variable or the command-line).\n> \n> Why do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking\n> for a reason there's 2d14e13c56 (test output: respect\n> $TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for\n> valgrind/analyze.sh.\n> \n> I don't know who uses that script, or how. There's no documentaion,\n> nothing on the mailing list, and nothing found on Google.\n\nPerhaps 268fac6919 (Add a script to coalesce the valgrind outputs,\n2009-02-04) is enlightening.\n\nI don't know if anybody still uses it these days, though. I suspect it's\noutlived its usefulness, in that we would typically not have any\nvalgrind errors at all (so coalescing them is not that interesting).\n\nPossibly folks investigating leak-checking via valgrind could find it\nuseful, but even there I think LSan is a much better path forward.\n\n> So maybe:\n> \n> diff --git a/Makefile b/Makefile\n> index c3565fc0f8..2e25489569 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2790,9 +2790,6 @@ GIT-BUILD-OPTIONS: FORCE\n>         @echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n>         @echo DC_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_SHA1)))'\\' >>$@+\n>         @echo X=\\'$(X)\\' >>$@+\n> -ifdef TEST_OUTPUT_DIRECTORY\n> -       @echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n> -endif\n\nI don't personally have any problem with that. It does mean that \"make\nt1234-foo.sh\" will behave differently than \"./t1234-foo.sh\", but that is\nalready true if you set GIT_TEST_OPTS.\n\n-Peff\n"},{"id":"427279","messageId":"60c715dd9939e_436208f3@natae.notmuch","threadId":"55889","inReplyTo":"YMcIv7q1ctyJ0EZn@coredump.intra.peff.net","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-14T08:39:57Z","receivedAt":"2021-06-14T08:40:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:\n> \n> > > Unfortunately, this isn't a complete solution.\n> > \n> > Software will never be perfect.\n> > \n> > We don't need to wait for a perfect solution, all we need is something\n> > better than the current siuation.\n> \n> Sure, but if you don't fully understand the situation (e.g., that --root\n> and TEST_OUTPUT_DIRECTORY are not equivalent), then you may end up\n> revisiting the incomplete fix later,\n\nThe fact that you may end up revisiting a solution is a fact for *all*\nchanges (including 2d14e13c56 (test output: respect\n$TEST_OUTPUT_DIRECTORY, 2013-04-29)).\n\n> or even making things worse (as this patch did).\n\nI think breaking the test suite is objectively worse than having a few\nextra files in the output directory, but to each his own.\n\n> > > I think solving the whole issue would require a mechanism for passing\n> > > TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an\n> > > environment variable or the command-line).\n> > \n> > Why do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking\n> > for a reason there's 2d14e13c56 (test output: respect\n> > $TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for\n> > valgrind/analyze.sh.\n> > \n> > I don't know who uses that script, or how. There's no documentaion,\n> > nothing on the mailing list, and nothing found on Google.\n> \n> Perhaps 268fac6919 (Add a script to coalesce the valgrind outputs,\n> 2009-02-04) is enlightening.\n\nThat makes it clearer.\n\n> I don't know if anybody still uses it these days, though. I suspect it's\n> outlived its usefulness, in that we would typically not have any\n> valgrind errors at all (so coalescing them is not that interesting).\n> \n> Possibly folks investigating leak-checking via valgrind could find it\n> useful, but even there I think LSan is a much better path forward.\n\nYeah, but even if they do run this tool, they can set\nTEST_OUTPUT_DIRECTORY manually.\n\nThe needs of the few should not otweight needs of the many.\n\n> > So maybe:\n> > \n> > diff --git a/Makefile b/Makefile\n> > index c3565fc0f8..2e25489569 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2790,9 +2790,6 @@ GIT-BUILD-OPTIONS: FORCE\n> >         @echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n> >         @echo DC_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_SHA1)))'\\' >>$@+\n> >         @echo X=\\'$(X)\\' >>$@+\n> > -ifdef TEST_OUTPUT_DIRECTORY\n> > -       @echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n> > -endif\n> \n> I don't personally have any problem with that. It does mean that \"make\n> t1234-foo.sh\" will behave differently than \"./t1234-foo.sh\", but that is\n> already true if you set GIT_TEST_OPTS.\n\nOnly if you haven't changed TEST_OUTPUT_DIRECTORY since the last time\nyou ran 'make' on the top level directory.\n\nAnd of course if somebody really wants their environment to be honored,\nthat's what \"make -e t1234-foo.sh\" is for.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427282","messageId":"8735tk22if.fsf@evledraar.gmail.com","threadId":"55889","inReplyTo":"60c715dd9939e_436208f3@natae.notmuch","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-14T09:33:12Z","receivedAt":"2021-06-14T09:56:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 14 2021, Felipe Contreras wrote:\n\n> Jeff King wrote:\n>> On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:\n>> \n>> > > Unfortunately, this isn't a complete solution.\n>> > \n>> > Software will never be perfect.\n>> > \n>> > We don't need to wait for a perfect solution, all we need is something\n>> > better than the current siuation.\n>> \n>> Sure, but if you don't fully understand the situation (e.g., that --root\n>> and TEST_OUTPUT_DIRECTORY are not equivalent), then you may end up\n>> revisiting the incomplete fix later,\n>\n> The fact that you may end up revisiting a solution is a fact for *all*\n> changes (including 2d14e13c56 (test output: respect\n> $TEST_OUTPUT_DIRECTORY, 2013-04-29)).\n>\n>> or even making things worse (as this patch did).\n>\n> I think breaking the test suite is objectively worse than having a few\n> extra files in the output directory, but to each his own.\n\nWe've got both in-tree and out-tree things that rely on e.g. the\n*.counts in that directory to have a 1=1 mapping with \"real\"\ntests. E.g. \"make aggregate-results\".\n\n>> > > I think solving the whole issue would require a mechanism for passing\n>> > > TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an\n>> > > environment variable or the command-line).\n>> > \n>> > Why do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking\n>> > for a reason there's 2d14e13c56 (test output: respect\n>> > $TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for\n>> > valgrind/analyze.sh.\n>> > \n>> > I don't know who uses that script, or how. There's no documentaion,\n>> > nothing on the mailing list, and nothing found on Google.\n>> \n>> Perhaps 268fac6919 (Add a script to coalesce the valgrind outputs,\n>> 2009-02-04) is enlightening.\n>\n> That makes it clearer.\n>\n>> I don't know if anybody still uses it these days, though. I suspect it's\n>> outlived its usefulness, in that we would typically not have any\n>> valgrind errors at all (so coalescing them is not that interesting).\n>> \n>> Possibly folks investigating leak-checking via valgrind could find it\n>> useful, but even there I think LSan is a much better path forward.\n>\n> Yeah, but even if they do run this tool, they can set\n> TEST_OUTPUT_DIRECTORY manually.\n>\n> The needs of the few should not otweight needs of the many.\n\nDo we need to bring Spock into this?:)\n\nI think the following alternate/on-top patch (which you should feel free\nto squash in with my SOB if you agree) solves the issue both of you are\nnoting.\n\nIt will barf if you run the tests under e.g. --tee with that environment\nvariable, but that's intentional. It's better to loudly error if we\ndon't have the expected test-results than to silently write in the wrong\nplace.\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 2c6e34b9478..29bf67d49bf 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -76,6 +76,12 @@ _run_sub_test_lib_test_common () {\n \t\t# this variable, so we need a stable setting under which to run\n \t\t# the sub-test.\n \t\tsane_unset HARNESS_ACTIVE &&\n+\n+\t\t# These tests should emit no metrics or output that\n+\t\t# would normally go in the \"test-results\" directory.\n+\t\tTEST_NO_RESULTS_OUTPUT=1 &&\n+\t\texport TEST_NO_RESULTS_OUTPUT &&\n+\n \t\tcd \"$name\" &&\n \t\twrite_script \"$name.sh\" \"$TEST_SHELL_PATH\" <<-EOF &&\n \t\ttest_description='$descr (run in sub test-lib)\n@@ -89,14 +95,12 @@ _run_sub_test_lib_test_common () {\n \t\tEOF\n \t\tcat >>\"$name.sh\" &&\n \t\texport TEST_DIRECTORY &&\n-\t\tTEST_OUTPUT_DIRECTORY=$(pwd) &&\n-\t\texport TEST_OUTPUT_DIRECTORY &&\n \t\tsane_unset GIT_TEST_FAIL_PREREQS &&\n \t\tif test -z \"$neg\"\n \t\tthen\n-\t\t\t./\"$name.sh\" \"$@\" >out 2>err\n+\t\t\t./\"$name.sh\" --root=\"$(pwd)\" \"$@\" >out 2>err\n \t\telse\n-\t\t\t! ./\"$name.sh\" \"$@\" >out 2>err\n+\t\t\t! ./\"$name.sh\" --root=\"$(pwd)\" \"$@\" >out 2>err\n \t\tfi\n \t)\n }\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54938c64279..9e9696a3185 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -252,8 +252,14 @@ TEST_STRESS_JOB_SFX=\"${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}\"\n TEST_NAME=\"$(basename \"$0\" .sh)\"\n TEST_NUMBER=\"${TEST_NAME%%-*}\"\n TEST_NUMBER=\"${TEST_NUMBER#t}\"\n-TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n-TEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n+if test -n \"$TEST_NO_RESULTS_OUTPUT\"\n+then\n+\tTEST_RESULTS_DIR=/dev/null\n+\tTEST_RESULTS_BASE=/dev/null\n+else\n+\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n+\tTEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n+fi\n TRASH_DIRECTORY=\"trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX\"\n test -n \"$root\" && TRASH_DIRECTORY=\"$root/$TRASH_DIRECTORY\"\n case \"$TRASH_DIRECTORY\" in\n@@ -1124,7 +1130,7 @@ test_done () {\n \n \tfinalize_junit_xml\n \n-\tif test -z \"$HARNESS_ACTIVE\"\n+\tif test -z \"$HARNESS_ACTIVE$TEST_NO_RESULTS_OUTPUT\"\n \tthen\n \t\tmkdir -p \"$TEST_RESULTS_DIR\"\n \n"},{"id":"427352","messageId":"YMdm5XayUfp4/atY@coredump.intra.peff.net","threadId":"55889","inReplyTo":"8735tk22if.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-14T14:25:41Z","receivedAt":"2021-06-14T14:25:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I think breaking the test suite is objectively worse than having a few\n> > extra files in the output directory, but to each his own.\n> \n> We've got both in-tree and out-tree things that rely on e.g. the\n> *.counts in that directory to have a 1=1 mapping with \"real\"\n> tests. E.g. \"make aggregate-results\".\n\nIndeed. With Felipe's original patch, the \"test\" target (but not\n\"prove\") in t/Makefile will report, whether you set\nTEST_OUTPUT_DIRECTORY or not:\n\n  failed test(s): t1234 t2345\n\n  fixed   0\n  success 23243\n  failed  2\n  broken  221\n  total   23647\n\nthough curiously it doesn't exit non-zero back to make (usually we'd\nalso see the failures from the individual make targets, and barf there).\n\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 2c6e34b9478..29bf67d49bf 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -76,6 +76,12 @@ _run_sub_test_lib_test_common () {\n>  \t\t# this variable, so we need a stable setting under which to run\n>  \t\t# the sub-test.\n>  \t\tsane_unset HARNESS_ACTIVE &&\n> +\n> +\t\t# These tests should emit no metrics or output that\n> +\t\t# would normally go in the \"test-results\" directory.\n> +\t\tTEST_NO_RESULTS_OUTPUT=1 &&\n> +\t\texport TEST_NO_RESULTS_OUTPUT &&\n\nI'm OK with this general approach. I do think it would be nice if we let\nthe environment supersede the on-disk GIT-BUILD-OPTIONS, which IMHO is\nthe real root of the problem (and possibly others), but that may be more\nchallenging to get right (I posted a patch earlier, but it does rely on\nstuffing all of \"set\" into a variable, which makes me concerned some\nless-able shells may complain).\n\nIt also means that t0000 can't test the results output (since we don't\nwrite it), but I assume we don't do that now (I didn't actually try\nrunning with your patch).\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 54938c64279..9e9696a3185 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -252,8 +252,14 @@ TEST_STRESS_JOB_SFX=\"${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}\"\n>  TEST_NAME=\"$(basename \"$0\" .sh)\"\n>  TEST_NUMBER=\"${TEST_NAME%%-*}\"\n>  TEST_NUMBER=\"${TEST_NUMBER#t}\"\n> -TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n> -TEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n> +if test -n \"$TEST_NO_RESULTS_OUTPUT\"\n> +then\n> +\tTEST_RESULTS_DIR=/dev/null\n> +\tTEST_RESULTS_BASE=/dev/null\n> +else\n> +\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n> +\tTEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n> +fi\n\nI wondered about this use of /dev/null, since we'd generally use this as\na directory, and writing to \"/dev/null/foo\" is going to throw an error.\n\nBut...\n\n>  TRASH_DIRECTORY=\"trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX\"\n>  test -n \"$root\" && TRASH_DIRECTORY=\"$root/$TRASH_DIRECTORY\"\n>  case \"$TRASH_DIRECTORY\" in\n> @@ -1124,7 +1130,7 @@ test_done () {\n>  \n>  \tfinalize_junit_xml\n>  \n> -\tif test -z \"$HARNESS_ACTIVE\"\n> +\tif test -z \"$HARNESS_ACTIVE$TEST_NO_RESULTS_OUTPUT\"\n>  \tthen\n>  \t\tmkdir -p \"$TEST_RESULTS_DIR\"\n\n...here we would never look at those variables at all, so it is just a\nsentinel that would let us know the assumption has been violated.\n\nWe do look at them elsewhere, though (in --tee as you noted, and I think\nfor --stress). I'd prefer to notice the \"no results\" flag explicitly\nthere and report something sensible, rather than getting:\n\n  mkdir: cannot create directory ‘/dev/null’: Not a directory\n\nor similar.\n\n-Peff\n"},{"id":"427364","messageId":"87r1h4z8k0.fsf@evledraar.gmail.com","threadId":"55889","inReplyTo":"YMdm5XayUfp4/atY@coredump.intra.peff.net","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-14T16:55:03Z","receivedAt":"2021-06-14T17:00:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 14 2021, Jeff King wrote:\n\n> On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I think breaking the test suite is objectively worse than having a few\n>> > extra files in the output directory, but to each his own.\n>> \n>> We've got both in-tree and out-tree things that rely on e.g. the\n>> *.counts in that directory to have a 1=1 mapping with \"real\"\n>> tests. E.g. \"make aggregate-results\".\n>\n> Indeed. With Felipe's original patch, the \"test\" target (but not\n> \"prove\") in t/Makefile will report, whether you set\n> TEST_OUTPUT_DIRECTORY or not:\n>\n>   failed test(s): t1234 t2345\n>\n>   fixed   0\n>   success 23243\n>   failed  2\n>   broken  221\n>   total   23647\n>\n> though curiously it doesn't exit non-zero back to make (usually we'd\n> also see the failures from the individual make targets, and barf there).\n\nOdd.\n\n>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n>> index 2c6e34b9478..29bf67d49bf 100755\n>> --- a/t/t0000-basic.sh\n>> +++ b/t/t0000-basic.sh\n>> @@ -76,6 +76,12 @@ _run_sub_test_lib_test_common () {\n>>  \t\t# this variable, so we need a stable setting under which to run\n>>  \t\t# the sub-test.\n>>  \t\tsane_unset HARNESS_ACTIVE &&\n>> +\n>> +\t\t# These tests should emit no metrics or output that\n>> +\t\t# would normally go in the \"test-results\" directory.\n>> +\t\tTEST_NO_RESULTS_OUTPUT=1 &&\n>> +\t\texport TEST_NO_RESULTS_OUTPUT &&\n>\n> I'm OK with this general approach. I do think it would be nice if we let\n> the environment supersede the on-disk GIT-BUILD-OPTIONS, which IMHO is\n> the real root of the problem (and possibly others), but that may be more\n> challenging to get right (I posted a patch earlier, but it does rely on\n> stuffing all of \"set\" into a variable, which makes me concerned some\n> less-able shells may complain).\n\nYeah I don't know and haven't dug into who wants all this combination of\nGIT-BUILD-OPTIONS, passing things in the env, or passing things as\nparamaters to make (sometimes under the same names).\n\n> It also means that t0000 can't test the results output (since we don't\n> write it), but I assume we don't do that now (I didn't actually try\n> running with your patch).\n\nYeah, but only in the trivial wrapper function, you can still write the\ntest script and check the output yourself.\n\nThat's much easier on top of a series to move that into a lib-subtest.sh\nthat I submitted today:\nhttps://lore.kernel.org/git/cover-0.8-00000000000-20210614T104351Z-avarab@gmail.com/\n\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 54938c64279..9e9696a3185 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -252,8 +252,14 @@ TEST_STRESS_JOB_SFX=\"${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}\"\n>>  TEST_NAME=\"$(basename \"$0\" .sh)\"\n>>  TEST_NUMBER=\"${TEST_NAME%%-*}\"\n>>  TEST_NUMBER=\"${TEST_NUMBER#t}\"\n>> -TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n>> -TEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n>> +if test -n \"$TEST_NO_RESULTS_OUTPUT\"\n>> +then\n>> +\tTEST_RESULTS_DIR=/dev/null\n>> +\tTEST_RESULTS_BASE=/dev/null\n>> +else\n>> +\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n>> +\tTEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n>> +fi\n>\n> I wondered about this use of /dev/null, since we'd generally use this as\n> a directory, and writing to \"/dev/null/foo\" is going to throw an error.\n>\n> But...\n>\n>>  TRASH_DIRECTORY=\"trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX\"\n>>  test -n \"$root\" && TRASH_DIRECTORY=\"$root/$TRASH_DIRECTORY\"\n>>  case \"$TRASH_DIRECTORY\" in\n>> @@ -1124,7 +1130,7 @@ test_done () {\n>>  \n>>  \tfinalize_junit_xml\n>>  \n>> -\tif test -z \"$HARNESS_ACTIVE\"\n>> +\tif test -z \"$HARNESS_ACTIVE$TEST_NO_RESULTS_OUTPUT\"\n>>  \tthen\n>>  \t\tmkdir -p \"$TEST_RESULTS_DIR\"\n>\n> ...here we would never look at those variables at all, so it is just a\n> sentinel that would let us know the assumption has been violated.\n>\n> We do look at them elsewhere, though (in --tee as you noted, and I think\n> for --stress). I'd prefer to notice the \"no results\" flag explicitly\n> there and report something sensible, rather than getting:\n\nIf we edit every single current callsite instead of setting it to\nsomething you can't write to then we're setting ourselves up for subtle\nbugss when someone uses $TEST_RESULTS_DIR for something else.\n\n>   mkdir: cannot create directory ‘/dev/null’: Not a directory\n>\n> or similar.\n\nYeah that error sucks, but nobody will see it unless they're hacking on\nthe guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.\n\nIn any case, I'll let Felipe decide what, if anything, to do with this\n:)\n"},{"id":"427449","messageId":"YMiKlmY3B/1cDrr8@coredump.intra.peff.net","threadId":"55889","inReplyTo":"87r1h4z8k0.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-15T11:10:14Z","receivedAt":"2021-06-15T11:10:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 14, 2021 at 06:55:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Indeed. With Felipe's original patch, the \"test\" target (but not\n> > \"prove\") in t/Makefile will report, whether you set\n> > TEST_OUTPUT_DIRECTORY or not:\n> >\n> >   failed test(s): t1234 t2345\n> >\n> >   fixed   0\n> >   success 23243\n> >   failed  2\n> >   broken  221\n> >   total   23647\n> >\n> > though curiously it doesn't exit non-zero back to make (usually we'd\n> > also see the failures from the individual make targets, and barf there).\n> \n> Odd.\n\nI think it's just that the aggregation script was never meant to signal\nto \"make\". In a regular \"make test\" (not using prove), each individual\ntest script is a dependency than can fail on its own. That means a\nfailure of any of them will signal \"make\" to fail the overall operation.\n\nInterestingly it means we will not run the \"aggregate-results\" at all in\nthat case, so we would not give the nice output (you can run \"make\naggregate-results\" yourself, though; it doesn't depend on the tests\nrunning itself, but assumes you've already run them).\n\nSo arguably we could do something like this:\n\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7913e206ed..6198b2ef6b 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -44,3 +44,5 @@ printf \"%-8s%d\\n\" success $success\n printf \"%-8s%d\\n\" failed $failed\n printf \"%-8s%d\\n\" broken $broken\n printf \"%-8s%d\\n\" total $total\n+\n+test -z \"$failed_tests\"\n\nbut it makes \"make aggregate-results\" after the fact a little noisier. I\ndunno. I don't really care that much about the output from this form of\nthe tests at all, since the \"prove\" output is _so_ much better, and I'd\nhighly recommend anybody use it instead.\n\nThe only thing preventing me from suggesting we get rid of the old\nmake-driven approach entirely is that there are probably platforms that\nrun the tests where \"prove\" is not available. And as long as it is not\ngenerating wrong results (e.g., returning 0 when a test has failed), it\nis doing that job OK.\n\n> > I'm OK with this general approach. I do think it would be nice if we let\n> > the environment supersede the on-disk GIT-BUILD-OPTIONS, which IMHO is\n> > the real root of the problem (and possibly others), but that may be more\n> > challenging to get right (I posted a patch earlier, but it does rely on\n> > stuffing all of \"set\" into a variable, which makes me concerned some\n> > less-able shells may complain).\n> \n> Yeah I don't know and haven't dug into who wants all this combination of\n> GIT-BUILD-OPTIONS, passing things in the env, or passing things as\n> paramaters to make (sometimes under the same names).\n\nTo be clear, I doubt it's all that important. It would occasionally be\nless surprising when trying to override the environment while running a\ntest script manually (which is after all, basically the same thing\nthat's happening here, just driven by a script). But if it's hard to do,\nI'm OK with an easier solution provided it doesn't regress any other\ncases.\n\n> > It also means that t0000 can't test the results output (since we don't\n> > write it), but I assume we don't do that now (I didn't actually try\n> > running with your patch).\n> \n> Yeah, but only in the trivial wrapper function, you can still write the\n> test script and check the output yourself.\n\nSort of. You can avoid its setting of TEST_NO_RESULTS_OUTPUT, but we're\nstill left without a way to reliably override TEST_OUTPUT_DIRECTORY.\nAgain, I'm OK punting on that for now if there are no such tests.\n\n> > We do look at them elsewhere, though (in --tee as you noted, and I think\n> > for --stress). I'd prefer to notice the \"no results\" flag explicitly\n> > there and report something sensible, rather than getting:\n> \n> If we edit every single current callsite instead of setting it to\n> something you can't write to then we're setting ourselves up for subtle\n> bugss when someone uses $TEST_RESULTS_DIR for something else.\n\nI was thinking we'd still set it to /dev/null as a belt-and-suspenders\n(so the worst case is just an ugly error message). But...\n\n> >   mkdir: cannot create directory ‘/dev/null’: Not a directory\n> >\n> > or similar.\n> \n> Yeah that error sucks, but nobody will see it unless they're hacking on\n> the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.\n\nI think that's a good point. You're unlikely to stumble into\naccidentally using TEST_NO_RESULTS_OUTPUT, so it might not be worth\ncaring about too much.\n\n-Peff\n"},{"id":"427451","messageId":"3caba1f7-026d-827e-d97a-8b5708dd9765@gmail.com","threadId":"55889","inReplyTo":"YMiKlmY3B/1cDrr8@coredump.intra.peff.net","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-06-15T11:21:00Z","receivedAt":"2021-06-15T11:21:16Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Hi Jeff,\n\nOn 15/06/21 18.10, Jeff King wrote:\n> I think it's just that the aggregation script was never meant to signal\n> to \"make\". In a regular \"make test\" (not using prove), each individual\n> test script is a dependency than can fail on its own. That means a\n> failure of any of them will signal \"make\" to fail the overall operation.\n> \nOnly one failure can trigger FTBFS when make test, right?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"427453","messageId":"YMiNyVUjeLrWt9iH@coredump.intra.peff.net","threadId":"55889","inReplyTo":"3caba1f7-026d-827e-d97a-8b5708dd9765@gmail.com","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-15T11:23:53Z","receivedAt":"2021-06-15T11:23:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 15, 2021 at 06:21:00PM +0700, Bagas Sanjaya wrote:\n\n> On 15/06/21 18.10, Jeff King wrote:\n> > I think it's just that the aggregation script was never meant to signal\n> > to \"make\". In a regular \"make test\" (not using prove), each individual\n> > test script is a dependency than can fail on its own. That means a\n> > failure of any of them will signal \"make\" to fail the overall operation.\n> > \n> Only one failure can trigger FTBFS when make test, right?\n\nI'm not sure I understand what your question means. I know that \"FTBFS\"\nmeans \"failed to build from source\", but that is not a term we use in\nthe Git project. So if you are asking whether \"make test\" will exit\nnon-zero if a single test fails, then yes.\n\nIf you're asking whether a Debian package would consider that an FTBFS,\nthen probably yes, but it depends on whether they run the tests.\n\n-Peff\n"},{"id":"427514","messageId":"60c8e7419b90c_12c282081a@natae.notmuch","threadId":"55889","inReplyTo":"8735tk22if.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T17:45:37Z","receivedAt":"2021-06-15T17:45:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jun 14 2021, Felipe Contreras wrote:\n> \n> > Jeff King wrote:\n\n> >> or even making things worse (as this patch did).\n> >\n> > I think breaking the test suite is objectively worse than having a few\n> > extra files in the output directory, but to each his own.\n> \n> We've got both in-tree and out-tree things that rely on e.g. the\n> *.counts in that directory to have a 1=1 mapping with \"real\"\n> tests. E.g. \"make aggregate-results\".\n\nYeah, that's not good, but I wouldn't call it \"worse\".\n\n> >> I don't know if anybody still uses it these days, though. I suspect it's\n> >> outlived its usefulness, in that we would typically not have any\n> >> valgrind errors at all (so coalescing them is not that interesting).\n> >> \n> >> Possibly folks investigating leak-checking via valgrind could find it\n> >> useful, but even there I think LSan is a much better path forward.\n> >\n> > Yeah, but even if they do run this tool, they can set\n> > TEST_OUTPUT_DIRECTORY manually.\n> >\n> > The needs of the few should not otweight needs of the many.\n> \n> Do we need to bring Spock into this?:)\n\nHopefully not.\n\n> I think the following alternate/on-top patch (which you should feel free\n> to squash in with my SOB if you agree) solves the issue both of you are\n> noting.\n> \n> It will barf if you run the tests under e.g. --tee with that environment\n> variable, but that's intentional. It's better to loudly error if we\n> don't have the expected test-results than to silently write in the wrong\n> place.\n\nI'm not sure how that patch was supposed to work:\n\n  'err' is not empty, it contains:\n  mkdir: cannot create directory '/dev/null': File exists\n  /home/felipec/dev/git.git/git/t/test-lib.sh: line 1133: /dev/null.counts: Permission denied\n  not ok 5 - pretend we have a fully passing test suite\n \nDoes this assume the rest of the framework will be updated to properly\nhandle TEST_NO_RESULTS_OUTPUT?\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"427516","messageId":"60c8eceeae8e8_12c2820895@natae.notmuch","threadId":"55889","inReplyTo":"YMiKlmY3B/1cDrr8@coredump.intra.peff.net","subject":"Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T18:09:50Z","receivedAt":"2021-06-15T18:09:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Mon, Jun 14, 2021 at 06:55:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > >   mkdir: cannot create directory ‘/dev/null’: Not a directory\n> > >\n> > > or similar.\n> > \n> > Yeah that error sucks, but nobody will see it unless they're hacking on\n> > the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.\n> \n> I think that's a good point. You're unlikely to stumble into\n> accidentally using TEST_NO_RESULTS_OUTPUT, so it might not be worth\n> caring about too much.\n\nBut check_sub_test_lib_test() cares:\n\n  test_must_be_empty err \n\n\n  'err' is not empty, it contains:\n  mkdir: cannot create directory '/dev/null': File exists\n  /home/felipec/dev/git.git/git/t/test-lib.sh: line 1133: /dev/null.counts: Permission denied\n  not ok 5 - pretend we have a fully passing test suite\n\n-- \nFelipe Contreras"}]}