{"thread":{"id":"55937","subject":"[PATCH v2] test: fix for TEST_OUTPUT_DIRECTORY","startedAt":"2021-06-15T18:32:08Z","lastAt":"2021-06-15T19:20:49Z","messageCount":2,"participants":["Felipe Contreras","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"427518","messageId":"20210615183157.104999-1-felipe.contreras@gmail.com","threadId":"55937","inReplyTo":null,"subject":"[PATCH v2] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T18:31:57Z","receivedAt":"2021-06-15T18:32:08Z","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 try to specify --root, as 6883047071 (t0000: set\nTEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28) suggested, but then the\nresults of subtests would leak out because TEST_RESULTS_DIR would not\nbe changed from the parent.\n\nInstead, let's revert part of 2d14e13c56 (test output: respect\n$TEST_OUTPUT_DIRECTORY, 2013-04-29) by removing TEST_OUTPUT_DIRECTORY\nfrom GIT-BUILD-OPTIONS.\n\nIt's unclear how much value t/valgrind/analyze.sh provides today, but\nusers of that script that use TEST_OUTPUT_DIRECTORY as well can simply\ncall the script with that variable in the environment.\n\nIt doesn't make much sense to break t0000-basic.sh for users of\nTEST_OUTPUT_DIRECTORY, just to provide a little convenience for the\nusers of t/valgrind/analyze.sh.\n\nPresumably this was broken since 900721e15c (test-lib: introduce\n'test_atexit', 2019-03-13).\n\nCc: John Keeping \nCc: Johannes Schindelin \nHelped-by: Jeff King \nSigned-off-by: Felipe Contreras \n---\n\nSince v1 I completely changed the approach and instead of using --root\nwhich leaks test results, I remove TEST_OUTPUT_DIRECTORY from\nGIT-BUILD-OPTIONS.\n\nApparently only people who would care are the users of\nt/valgrind/analyze.sh which now would need to specify that variable\nthemselves.\n\nMarginal convenience for the users of an obscure script is not a good\nreason to break t0000-basic.sh.\n\nRange-diff against v1:\n1: 04047359b9 < -: ---------- test: fix for TEST_OUTPUT_DIRECTORY\n-: ---------- > 1: d8430aee08 test: fix for TEST_OUTPUT_DIRECTORY\n\n Makefile | 3 ---\n t/valgrind/analyze.sh | 3 ---\n 2 files changed, 6 deletions(-)\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 \t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n \t@echo DC_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_SHA1)))'\\' >>$@+\n \t@echo X=\\'$(X)\\' >>$@+\n-ifdef TEST_OUTPUT_DIRECTORY\n-\t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n-endif\n ifdef GIT_TEST_OPTS\n \t@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-- \n2.32.0\n\n"},{"id":"427522","messageId":"87y2bbx7cq.fsf@evledraar.gmail.com","threadId":"55937","inReplyTo":"20210615183157.104999-1-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] test: fix for TEST_OUTPUT_DIRECTORY","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-15T19:16:34Z","receivedAt":"2021-06-15T19:20:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 15 2021, 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> We could try to specify --root, as 6883047071 (t0000: set\n> TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28) suggested, but then the\n> results of subtests would leak out because TEST_RESULTS_DIR would not\n> be changed from the parent.\n>\n> Instead, let's revert part of 2d14e13c56 (test output: respect\n> $TEST_OUTPUT_DIRECTORY, 2013-04-29) by removing TEST_OUTPUT_DIRECTORY\n> from GIT-BUILD-OPTIONS.\n>\n> It's unclear how much value t/valgrind/analyze.sh provides today, but\n> users of that script that use TEST_OUTPUT_DIRECTORY as well can simply\n> call the script with that variable in the environment.\n\nI've only skimmed this & the patch, but this approach sounds good to me.\n\n> It doesn't make much sense to break t0000-basic.sh for users of\n> TEST_OUTPUT_DIRECTORY, just to provide a little convenience for the\n> users of t/valgrind/analyze.sh.\n\nI would also think that if anyone really cares about this\nvalgrind/analyze.sh script the thing to do is to make a Makefile target\nfor it, similar to the aggregate target(s).\n\nWe could then make that target go through some light wrapper in\ntest-lib.sh so it would e.g. pick up GIT_TEST_OPTS (which can have\n--root=*), and otherwise find things in the same location that\ntest-lib.sh put them / thinks the are in.\n\n> Presumably this was broken since 900721e15c (test-lib: introduce\n> 'test_atexit', 2019-03-13).\n>\n> Cc: John Keeping \n> Cc: Johannes Schindelin \n> Helped-by: Jeff King \n> Signed-off-by: Felipe Contreras \n> ---\n>\n> Since v1 I completely changed the approach and instead of using --root\n> which leaks test results, I remove TEST_OUTPUT_DIRECTORY from\n> GIT-BUILD-OPTIONS.\n>\n> Apparently only people who would care are the users of\n> t/valgrind/analyze.sh which now would need to specify that variable\n> themselves.\n>\n> Marginal convenience for the users of an obscure script is not a good\n> reason to break t0000-basic.sh.\n>\n> Range-diff against v1:\n> 1: 04047359b9 < -: ---------- test: fix for TEST_OUTPUT_DIRECTORY\n> -: ---------- > 1: d8430aee08 test: fix for TEST_OUTPUT_DIRECTORY\n>\n> Makefile | 3 ---\n> t/valgrind/analyze.sh | 3 ---\n> 2 files changed, 6 deletions(-)\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> \t@echo PAGER_ENV=\\''$(subst ','\\'',$(subst ','\\'',$(PAGER_ENV)))'\\' >>$@+\n> \t@echo DC_SHA1=\\''$(subst ','\\'',$(subst ','\\'',$(DC_SHA1)))'\\' >>$@+\n> \t@echo X=\\'$(X)\\' >>$@+\n> -ifdef TEST_OUTPUT_DIRECTORY\n> -\t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@+\n> -endif\n> ifdef GIT_TEST_OPTS\n> \t@echo GIT_TEST_OPTS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_OPTS)))'\\' >>$@+\n> endif\n> diff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh\n> index 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"}]}