| Message ID | 44006e7b0bdda50dc51153cc2efb6ae954d4eecb.1626762728.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t0000: fix test if run with TEST_OUTPUT_DIRECTORY | expand |
On Tue, Jul 20, 2021 at 08:32:26AM +0200, Patrick Steinhardt wrote: > Fix the issue by adding a new TEST_OUTPUT_DIRECTORY_OVERRIDE variable. > If set, then we'll always override the TEST_OUTPUT_DIRECTORY with its > value after sourcing GIT-BUILD-OPTIONS. Thanks, I like this approach much better than removing TEST_OUTPUT_DIRECTORY entirely (and I confirmed that it fixes the problem). I do wish we had a more generic way of overriding stuff in GIT-BUILD-OPTIONS, rather than introducing manual _OVERRIDE variables for each. But there's not an easy solution here (see the earlier thread for some discussion), so this seems like a good immediate step to take. One small note on the commit message: > While this works as expected in the general case, it falls apart when > the developer has TEST_OUTPUT_DIRECTORY explicitly defined either via > the environment or via config.mak. The mention of the environment confused me for a moment, since: TEST_OUTPUT_DIRECTORY=/tmp/foo ./t0000-basic.sh is already OK. But you probably meant that: TEST_OUTPUT_DIRECTORY=/tmp/foo make test would fail, since "make" would pick up the variable and then write it into GIT-BUILD-OPTIONS (just as it would if you put it in config.mak, or on the command-line of make). I don't think it's sufficiently confusing to rewrite the commit message, but just something I noted while reading it. > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -57,6 +57,15 @@ fi > . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS > export PERL_PATH SHELL_PATH > > +# In t0000, we need to override test directories of nested testcases. In case > +# the developer has TEST_OUTPUT_DIRECTORY part of his build options, then we'd > +# reset this value to instead contain what the developer has specified. We thus > +# have this knob to allow overriding the directory. > +if test -n "${TEST_OUTPUT_DIRECTORY_OVERRIDE}" > +then > + TEST_OUTPUT_DIRECTORY="${TEST_OUTPUT_DIRECTORY_OVERRIDE}" > +fi Thanks for this comment. Hopefully that will make it clear to anybody that the override mechanism is not meant for general use by developers. -Peff
Patrick Steinhardt wrote: > Testcases in t0000 are quite special given that they many of them run > nested testcases to verify that testing functionality itself works as > expected. These nested testcases are realized by writing a new ad-hoc > test script which again sources test-lib.sh, where the new script is > created in a nested subdirectory located beneath the current trash > directory. We then execute the new test script with the nested > subdirectory as current working directory and explicitly re-export > TEST_OUTPUT_DIRECTORY to point to that directory. > > While this works as expected in the general case, it falls apart when > the developer has TEST_OUTPUT_DIRECTORY explicitly defined either via > the environment or via config.mak. In that case, test-lib.sh will > clobber the value that we've just carefully set up to instead contain > what the developer has defined. As a result, the TEST_OUTPUT_DIRECTORY > continues to point at the root output directory, not at the nested one. > > This issue causes breakage in the 'test_atexit is run' test case: the > nested test case writes files into "../../", which is assumed to be the > parent's trash directory. But because TEST_OUTPUT_DIRECTORY already > points to to the root output directory, we instead end up writing those > files outside of the output directory. The parent test case will then > try to check whether those files still exist in its own trash directory, > which thus must fail now. > > Fix the issue by adding a new TEST_OUTPUT_DIRECTORY_OVERRIDE variable. > If set, then we'll always override the TEST_OUTPUT_DIRECTORY with its > value after sourcing GIT-BUILD-OPTIONS. This is a very *very* narrowly-specific hack, but I guess it's better than the test suite failing.
Jeff King <peff@peff.net> writes: > On Tue, Jul 20, 2021 at 08:32:26AM +0200, Patrick Steinhardt wrote: > >> Fix the issue by adding a new TEST_OUTPUT_DIRECTORY_OVERRIDE variable. >> If set, then we'll always override the TEST_OUTPUT_DIRECTORY with its >> value after sourcing GIT-BUILD-OPTIONS. > > Thanks, I like this approach much better than removing > TEST_OUTPUT_DIRECTORY entirely (and I confirmed that it fixes the > problem). Yup, and it combines with your subtests-skip fix rather nicely to solve both problems. Thanks for working well together. > I do wish we had a more generic way of overriding stuff in > GIT-BUILD-OPTIONS, rather than introducing manual _OVERRIDE variables > for each. But there's not an easy solution here (see the earlier thread > for some discussion), so this seems like a good immediate step to take. > > One small note on the commit message: > >> While this works as expected in the general case, it falls apart when >> the developer has TEST_OUTPUT_DIRECTORY explicitly defined either via >> the environment or via config.mak. > > The mention of the environment confused me for a moment, since: > > TEST_OUTPUT_DIRECTORY=/tmp/foo ./t0000-basic.sh > > is already OK. But you probably meant that: > > TEST_OUTPUT_DIRECTORY=/tmp/foo make test > > would fail, since "make" would pick up the variable and then write it > into GIT-BUILD-OPTIONS (just as it would if you put it in config.mak, or > on the command-line of make). > > I don't think it's sufficiently confusing to rewrite the commit message, > but just something I noted while reading it. I'll just insert "and runs 'make test'" after "via config.mak" while queuing. Again, thanks, both.
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 2c6e34b947..09d2202748 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -89,8 +89,11 @@ _run_sub_test_lib_test_common () { EOF cat >>"$name.sh" && export TEST_DIRECTORY && - TEST_OUTPUT_DIRECTORY=$(pwd) && - export TEST_OUTPUT_DIRECTORY && + # The child test re-sources GIT-BUILD-OPTIONS and may thus + # override the test output directory. We thus pass it as an + # explicit override to the child. + TEST_OUTPUT_DIRECTORY_OVERRIDE=$(pwd) && + export TEST_OUTPUT_DIRECTORY_OVERRIDE && sane_unset GIT_TEST_FAIL_PREREQS && if test -z "$neg" then diff --git a/t/test-lib.sh b/t/test-lib.sh index 9e26860544..da13190970 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -57,6 +57,15 @@ fi . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS export PERL_PATH SHELL_PATH +# In t0000, we need to override test directories of nested testcases. In case +# the developer has TEST_OUTPUT_DIRECTORY part of his build options, then we'd +# reset this value to instead contain what the developer has specified. We thus +# have this knob to allow overriding the directory. +if test -n "${TEST_OUTPUT_DIRECTORY_OVERRIDE}" +then + TEST_OUTPUT_DIRECTORY="${TEST_OUTPUT_DIRECTORY_OVERRIDE}" +fi + # Disallow the use of abbreviated options in the test suite by default if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}" then
Testcases in t0000 are quite special given that they many of them run nested testcases to verify that testing functionality itself works as expected. These nested testcases are realized by writing a new ad-hoc test script which again sources test-lib.sh, where the new script is created in a nested subdirectory located beneath the current trash directory. We then execute the new test script with the nested subdirectory as current working directory and explicitly re-export TEST_OUTPUT_DIRECTORY to point to that directory. While this works as expected in the general case, it falls apart when the developer has TEST_OUTPUT_DIRECTORY explicitly defined either via the environment or via config.mak. In that case, test-lib.sh will clobber the value that we've just carefully set up to instead contain what the developer has defined. As a result, the TEST_OUTPUT_DIRECTORY continues to point at the root output directory, not at the nested one. This issue causes breakage in the 'test_atexit is run' test case: the nested test case writes files into "../../", which is assumed to be the parent's trash directory. But because TEST_OUTPUT_DIRECTORY already points to to the root output directory, we instead end up writing those files outside of the output directory. The parent test case will then try to check whether those files still exist in its own trash directory, which thus must fail now. Fix the issue by adding a new TEST_OUTPUT_DIRECTORY_OVERRIDE variable. If set, then we'll always override the TEST_OUTPUT_DIRECTORY with its value after sourcing GIT-BUILD-OPTIONS. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t0000-basic.sh | 7 +++++-- t/test-lib.sh | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-)