| Message ID | patch-v3-2.2-a7fc794e20d-20211210T100512Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD | expand |
On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote: > In the preceding commit the use of "test_untraceable=UnfortunatelyYes" > was removed from "t1510-repo-setup.sh" in favor of more narrow > redirections of the output of specific commands (and not entire > sub-shells or functions). > > This is in line with the fixes in the series that introduced the > "test_untraceable" facility. See 571e472dc43 (Merge branch > 'sg/test-x', 2018-03-14) for the series as a whole, and > e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a > subshell, 2018-02-24) for a commit that's in line with the changes in > the preceding commit. > > We've thus solved the TODO item noted when "test_untraceable" was > added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark > as untraceable with '-x', 2018-02-24). > > So let's remove the feature entirely. Not only is it currently unused, > but it actively encourages an anti-pattern in our tests. We should be > testing the output of specific commands, not entire subshells or > functions. > > That the "-x" output had to be disabled as a result is only one > symptom, but even under bash those tests will be harder to debug as > the subsequent check of the redirected file will be far removed from > the command that emitted the output. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/README | 3 --- > t/test-lib.sh | 66 ++++++--------------------------------------------- > 2 files changed, 7 insertions(+), 62 deletions(-) > > diff --git a/t/README b/t/README > index 29f72354bf1..3d30bbff34a 100644 > --- a/t/README > +++ b/t/README > @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e. > -x:: > Turn on shell tracing (i.e., `set -x`) during the tests > themselves. Implies `--verbose`. > - Ignored in test scripts that set the variable 'test_untraceable' > - to a non-empty value, unless it's run with a Bash version > - supporting BASH_XTRACEFD, i.e. v4.1 or later. > > -d:: > --debug:: > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 57efcc5e97a..b008716917b 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -381,29 +381,6 @@ then > exit > fi > > -if test -n "$trace" && test -n "$test_untraceable" > -then > - # '-x' tracing requested, but this test script can't be reliably > - # traced, unless it is run with a Bash version supporting > - # BASH_XTRACEFD (introduced in Bash v4.1). > - # > - # Perform this version check _after_ the test script was > - # potentially re-executed with $TEST_SHELL_PATH for '--tee' or > - # '--verbose-log', so the right shell is checked and the > - # warning is issued only once. > - if test -n "$BASH_VERSION" && eval ' > - test ${BASH_VERSINFO[0]} -gt 4 || { > - test ${BASH_VERSINFO[0]} -eq 4 && > - test ${BASH_VERSINFO[1]} -ge 1 > - } > - ' > - then > - : Executed by a Bash version supporting BASH_XTRACEFD. Good. > - else > - echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" > - trace= > - fi > -fi > if test -n "$trace" && test -z "$verbose_log" > then > verbose=t > @@ -650,19 +627,6 @@ else > exec 4>/dev/null 3>/dev/null > fi > > -# Send any "-x" output directly to stderr to avoid polluting tests > -# which capture stderr. We can do this unconditionally since it > -# has no effect if tracing isn't turned on. > -# > -# Note that this sets up the trace fd as soon as we assign the variable, so it > -# must come after the creation of descriptor 4 above. Likewise, we must never > -# unset this, as it has the side effect of closing descriptor 4, which we > -# use to show verbose tests to the user. > -# > -# Note also that we don't need or want to export it. The tracing is local to > -# this shell, and we would not want to influence any shells we exec. > -BASH_XTRACEFD=4 Please do not remove BASH_XTRACEFD. And especially not like this, without even mentioning it in the commit message. > test_failure=0 > test_count=0 > test_fixed=0 > @@ -949,36 +913,20 @@ test_eval_ () { > # the shell from printing the "set +x" to turn it off (nor the saving > # of $? before that). But we can make sure that the output goes to > # /dev/null. > - # > - # There are a few subtleties here: > - # > - # - we have to redirect descriptor 4 in addition to 2, to cover > - # BASH_XTRACEFD > - # > - # - the actual eval has to come before the redirection block (since > - # it needs to see descriptor 4 to set up its stderr) > - # > - # - likewise, any error message we print must be outside the block to > - # access descriptor 4 > - # > - # - checking $? has to come immediately after the eval, but it must > - # be _inside_ the block to avoid polluting the "set -x" output > - # > - > - test_eval_inner_ "$@" </dev/null >&3 2>&4 > { > + test_eval_inner_ "$@" </dev/null >&3 2>&4 > test_eval_ret_=$? > if want_trace > then > test 1 = $trace_level_ && set +x > trace_level_=$(($trace_level_-1)) > - fi > - } 2>/dev/null 4>&2 > > - if test "$test_eval_ret_" != 0 && want_trace > - then > - say_color error >&4 "error: last command exited with \$?=$test_eval_ret_" > - fi > + if test "$test_eval_ret_" != 0 > + then > + say_color error >&4 "error: last command exited with \$?=$test_eval_ret_" > + fi > + fi > + } 2>/dev/null > return $test_eval_ret_ > } > > -- > 2.34.1.932.g36842105b61 >
On Sun, Dec 12 2021, SZEDER Gábor wrote: > On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote: >> In the preceding commit the use of "test_untraceable=UnfortunatelyYes" >> was removed from "t1510-repo-setup.sh" in favor of more narrow >> redirections of the output of specific commands (and not entire >> sub-shells or functions). >> >> This is in line with the fixes in the series that introduced the >> "test_untraceable" facility. See 571e472dc43 (Merge branch >> 'sg/test-x', 2018-03-14) for the series as a whole, and >> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a >> subshell, 2018-02-24) for a commit that's in line with the changes in >> the preceding commit. >> >> We've thus solved the TODO item noted when "test_untraceable" was >> added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark >> as untraceable with '-x', 2018-02-24). >> >> So let's remove the feature entirely. Not only is it currently unused, >> but it actively encourages an anti-pattern in our tests. We should be >> testing the output of specific commands, not entire subshells or >> functions. >> >> That the "-x" output had to be disabled as a result is only one >> symptom, but even under bash those tests will be harder to debug as >> the subsequent check of the redirected file will be far removed from >> the command that emitted the output. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> t/README | 3 --- >> t/test-lib.sh | 66 ++++++--------------------------------------------- >> 2 files changed, 7 insertions(+), 62 deletions(-) >> >> diff --git a/t/README b/t/README >> index 29f72354bf1..3d30bbff34a 100644 >> --- a/t/README >> +++ b/t/README >> @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e. >> -x:: >> Turn on shell tracing (i.e., `set -x`) during the tests >> themselves. Implies `--verbose`. >> - Ignored in test scripts that set the variable 'test_untraceable' >> - to a non-empty value, unless it's run with a Bash version >> - supporting BASH_XTRACEFD, i.e. v4.1 or later. >> >> -d:: >> --debug:: >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> index 57efcc5e97a..b008716917b 100644 >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -381,29 +381,6 @@ then >> exit >> fi >> >> -if test -n "$trace" && test -n "$test_untraceable" >> -then >> - # '-x' tracing requested, but this test script can't be reliably >> - # traced, unless it is run with a Bash version supporting >> - # BASH_XTRACEFD (introduced in Bash v4.1). >> - # >> - # Perform this version check _after_ the test script was >> - # potentially re-executed with $TEST_SHELL_PATH for '--tee' or >> - # '--verbose-log', so the right shell is checked and the >> - # warning is issued only once. >> - if test -n "$BASH_VERSION" && eval ' >> - test ${BASH_VERSINFO[0]} -gt 4 || { >> - test ${BASH_VERSINFO[0]} -eq 4 && >> - test ${BASH_VERSINFO[1]} -ge 1 >> - } >> - ' >> - then >> - : Executed by a Bash version supporting BASH_XTRACEFD. Good. >> - else >> - echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" >> - trace= >> - fi >> -fi >> if test -n "$trace" && test -z "$verbose_log" >> then >> verbose=t >> @@ -650,19 +627,6 @@ else >> exec 4>/dev/null 3>/dev/null >> fi >> >> -# Send any "-x" output directly to stderr to avoid polluting tests >> -# which capture stderr. We can do this unconditionally since it >> -# has no effect if tracing isn't turned on. >> -# >> -# Note that this sets up the trace fd as soon as we assign the variable, so it >> -# must come after the creation of descriptor 4 above. Likewise, we must never >> -# unset this, as it has the side effect of closing descriptor 4, which we >> -# use to show verbose tests to the user. >> -# >> -# Note also that we don't need or want to export it. The tracing is local to >> -# this shell, and we would not want to influence any shells we exec. >> -BASH_XTRACEFD=4 > > Please do not remove BASH_XTRACEFD. And especially not like this, > without even mentioning it in the commit message. I can re-roll with an amended commit message that explicitly mentions it, but that doesn't address your "please do not remove", Do you see reason to keep it at the end-state fo this series? E.g. a counter-argument to https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?
On Sun, Dec 12, 2021 at 06:06:31PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Dec 12 2021, SZEDER Gábor wrote: > > > On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> In the preceding commit the use of "test_untraceable=UnfortunatelyYes" > >> was removed from "t1510-repo-setup.sh" in favor of more narrow > >> redirections of the output of specific commands (and not entire > >> sub-shells or functions). > >> > >> This is in line with the fixes in the series that introduced the > >> "test_untraceable" facility. See 571e472dc43 (Merge branch > >> 'sg/test-x', 2018-03-14) for the series as a whole, and > >> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a > >> subshell, 2018-02-24) for a commit that's in line with the changes in > >> the preceding commit. > >> > >> We've thus solved the TODO item noted when "test_untraceable" was > >> added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark > >> as untraceable with '-x', 2018-02-24). > >> > >> So let's remove the feature entirely. Not only is it currently unused, > >> but it actively encourages an anti-pattern in our tests. We should be > >> testing the output of specific commands, not entire subshells or > >> functions. > >> > >> That the "-x" output had to be disabled as a result is only one > >> symptom, but even under bash those tests will be harder to debug as > >> the subsequent check of the redirected file will be far removed from > >> the command that emitted the output. > >> > >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > >> --- > >> t/README | 3 --- > >> t/test-lib.sh | 66 ++++++--------------------------------------------- > >> 2 files changed, 7 insertions(+), 62 deletions(-) > >> > >> diff --git a/t/README b/t/README > >> index 29f72354bf1..3d30bbff34a 100644 > >> --- a/t/README > >> +++ b/t/README > >> @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e. > >> -x:: > >> Turn on shell tracing (i.e., `set -x`) during the tests > >> themselves. Implies `--verbose`. > >> - Ignored in test scripts that set the variable 'test_untraceable' > >> - to a non-empty value, unless it's run with a Bash version > >> - supporting BASH_XTRACEFD, i.e. v4.1 or later. > >> > >> -d:: > >> --debug:: > >> diff --git a/t/test-lib.sh b/t/test-lib.sh > >> index 57efcc5e97a..b008716917b 100644 > >> --- a/t/test-lib.sh > >> +++ b/t/test-lib.sh > >> @@ -381,29 +381,6 @@ then > >> exit > >> fi > >> > >> -if test -n "$trace" && test -n "$test_untraceable" > >> -then > >> - # '-x' tracing requested, but this test script can't be reliably > >> - # traced, unless it is run with a Bash version supporting > >> - # BASH_XTRACEFD (introduced in Bash v4.1). > >> - # > >> - # Perform this version check _after_ the test script was > >> - # potentially re-executed with $TEST_SHELL_PATH for '--tee' or > >> - # '--verbose-log', so the right shell is checked and the > >> - # warning is issued only once. > >> - if test -n "$BASH_VERSION" && eval ' > >> - test ${BASH_VERSINFO[0]} -gt 4 || { > >> - test ${BASH_VERSINFO[0]} -eq 4 && > >> - test ${BASH_VERSINFO[1]} -ge 1 > >> - } > >> - ' > >> - then > >> - : Executed by a Bash version supporting BASH_XTRACEFD. Good. > >> - else > >> - echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" > >> - trace= > >> - fi > >> -fi > >> if test -n "$trace" && test -z "$verbose_log" > >> then > >> verbose=t > >> @@ -650,19 +627,6 @@ else > >> exec 4>/dev/null 3>/dev/null > >> fi > >> > >> -# Send any "-x" output directly to stderr to avoid polluting tests > >> -# which capture stderr. We can do this unconditionally since it > >> -# has no effect if tracing isn't turned on. > >> -# > >> -# Note that this sets up the trace fd as soon as we assign the variable, so it > >> -# must come after the creation of descriptor 4 above. Likewise, we must never > >> -# unset this, as it has the side effect of closing descriptor 4, which we > >> -# use to show verbose tests to the user. > >> -# > >> -# Note also that we don't need or want to export it. The tracing is local to > >> -# this shell, and we would not want to influence any shells we exec. > >> -BASH_XTRACEFD=4 > > > > Please do not remove BASH_XTRACEFD. And especially not like this, > > without even mentioning it in the commit message. > > I can re-roll with an amended commit message that explicitly mentions > it It should rather be a separate patch... >, but that doesn't address your "please do not remove", > > Do you see reason to keep it at the end-state fo this series? E.g. a > counter-argument to > https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/? I don't see any argument pertinent to BASH_XTRACEFD in general in that email, of in favor of its removal in particular, and there are no valid arguments for its removal in earlier emails in this thread either.
SZEDER Gábor <szeder.dev@gmail.com> writes: >> > Please do not remove BASH_XTRACEFD. And especially not like this, >> > without even mentioning it in the commit message. >> >> I can re-roll with an amended commit message that explicitly mentions >> it > > It should rather be a separate patch... > >>, but that doesn't address your "please do not remove", >> >> Do you see reason to keep it at the end-state fo this series? E.g. a >> counter-argument to >> https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/? > > I don't see any argument pertinent to BASH_XTRACEFD in general in that > email, of in favor of its removal in particular, and there are no > valid arguments for its removal in earlier emails in this thread > either. If I am reading Ævar right, the argument is "dash would not be fixed with BASH_XTRACEFD, so there needs another way that would work there, and if the approach happens to work also for bash, then there is no reason to use BASH_XTRACEFD", I think. Now, if the way Ævar came up with to help shells with "-x" not to contaminate their standard error stream that our test scripts want to inspect is worse to write, understand, and maintain, compared to the way we have been writing our tests that inspect their standard errors, without having to worry about "-x" output thanks to the use of BASH_XTRACEFD, it may make a regression to developer productivity, but I am not sure if that is the case. I think [1/2] of this same series can serve an example of how tests must be tweaked to live under the world order without BASH_XTRACEFD? Having to set and use a temporary file to capture the standard error output and append to it upfront looks uglier than each individual test locally capturing the standard error output from a single invocation of a helper function, but it does not look _too_ bad to me. Can we find another example to argue for BASH_XTRACEFD, how much it makes it easier to write tests that work even under "-x"?
On Mon, Dec 13, 2021 at 10:51:14AM -0800, Junio C Hamano wrote: > > I don't see any argument pertinent to BASH_XTRACEFD in general in that > > email, of in favor of its removal in particular, and there are no > > valid arguments for its removal in earlier emails in this thread > > either. > > If I am reading Ævar right, the argument is "dash would not be fixed > with BASH_XTRACEFD, so there needs another way that would work there, > and if the approach happens to work also for bash, then there is no > reason to use BASH_XTRACEFD", I think. > > Now, if the way Ævar came up with to help shells with "-x" not to > contaminate their standard error stream that our test scripts want > to inspect is worse to write, understand, and maintain, compared to > the way we have been writing our tests that inspect their standard > errors, without having to worry about "-x" output thanks to the use > of BASH_XTRACEFD, it may make a regression to developer > productivity, but I am not sure if that is the case. I think the method for handling this in the test scripts _is_ worse to write, understand, and maintain. The problem to me is less that it's ugly to workaround (which as you note in this case is not great, but not _too_ bad), but that it's a subtle friction point that may jump up and bite any test-writer who does something like: (foo && bar) 2>stderr So my view had always been that BASH_XTRACEFD is the good solution, and if people want to make "-x" work reliably under other shells, then I won't stop them. But somewhere along the way Gábor did a bunch of fixes to get things mostly running, and the use of dash with "-x" got added to CI, so now it's a de facto requirement (if you care about CI complaining, which we increasingly do). Maybe that's OK. We've had fewer incidences of the problem popping up than I would have expected. My vision was that we'd leave BASH_XTRACEFD so people using it could remain oblivious if they chose, but if the ship has sailed via CI, then that might have less value. > I think [1/2] of this same series can serve an example of how tests > must be tweaked to live under the world order without BASH_XTRACEFD? > Having to set and use a temporary file to capture the standard error > output and append to it upfront looks uglier than each individual > test locally capturing the standard error output from a single > invocation of a helper function, but it does not look _too_ bad to > me. Can we find another example to argue for BASH_XTRACEFD, how > much it makes it easier to write tests that work even under "-x"? I think the fixes from 571e472dc4 (Merge branch 'sg/test-x', 2018-03-14) show what had to be done to get where we are today. -Peff
Jeff King <peff@peff.net> writes: > I think the method for handling this in the test scripts _is_ worse to > write, understand, and maintain. The problem to me is less that it's > ugly to workaround (which as you note in this case is not great, but not > _too_ bad), but that it's a subtle friction point that may jump up and > bite any test-writer who does something like: > > (foo && bar) 2>stderr Yeah, that is a simple example that clearly shows how removal of BASH_XTRACEFD makes developer's life horrible. > So my view had always been that BASH_XTRACEFD is the good solution, and > if people want to make "-x" work reliably under other shells, then I > won't stop them. But somewhere along the way Gábor did a bunch of fixes > to get things mostly running, and the use of dash with "-x" got added to > CI, so now it's a de facto requirement (if you care about CI > complaining, which we increasingly do). > ... > My vision was that we'd leave BASH_XTRACEFD so people using it could > remain oblivious if they chose, but if the ship has sailed via CI, then > that might have less value. Yeah, that matches my understanding. Unfortunately we cannot easily remove "dash -x" from CI while keeping "dash" without "-x" X-<. Still. I wonder if keeping BASH_XTRACEFD helps developers, when they need to diagnose a new breakage? If their new test fails only in the "dash -x" run but not "bash -x" at the CI, for example?
On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote: > Still. I wonder if keeping BASH_XTRACEFD helps developers, when > they need to diagnose a new breakage? If their new test fails only > in the "dash -x" run but not "bash -x" at the CI, for example? I have done that, but usually only after realizing that "./t1234-foo.sh" passes and "./t1234-foo.sh -x" does not. So I think just tweaking use of "-x" would be the main tool. -Peff
Jeff King <peff@peff.net> writes: > On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote: > >> Still. I wonder if keeping BASH_XTRACEFD helps developers, when >> they need to diagnose a new breakage? If their new test fails only >> in the "dash -x" run but not "bash -x" at the CI, for example? > > I have done that, but usually only after realizing that "./t1234-foo.sh" > passes and "./t1234-foo.sh -x" does not. So I think just tweaking use of > "-x" would be the main tool. Ah, that's true. You only need to compare "sh -x test.sh" with "sh test.sh" with any value of "sh", especially after BASH_XTRACEFD is removed, demoting "bash" to the same state as "dash" wrt "-x". OK, I am more OK with the removal of BASH_XTRACEFD support than before ;-)
[Replying to the thread-at-large] On Tue, Dec 14 2021, Jeff King wrote: > On Mon, Dec 13, 2021 at 10:51:14AM -0800, Junio C Hamano wrote: > >> > I don't see any argument pertinent to BASH_XTRACEFD in general in that >> > email, of in favor of its removal in particular, and there are no >> > valid arguments for its removal in earlier emails in this thread >> > either. >> >> If I am reading Ævar right, the argument is "dash would not be fixed >> with BASH_XTRACEFD, so there needs another way that would work there, >> and if the approach happens to work also for bash, then there is no >> reason to use BASH_XTRACEFD", I think. >> >> Now, if the way Ævar came up with to help shells with "-x" not to >> contaminate their standard error stream that our test scripts want >> to inspect is worse to write, understand, and maintain, compared to >> the way we have been writing our tests that inspect their standard >> errors, without having to worry about "-x" output thanks to the use >> of BASH_XTRACEFD, it may make a regression to developer >> productivity, but I am not sure if that is the case. > > I think the method for handling this in the test scripts _is_ worse to > write, understand, and maintain. The problem to me is less that it's > ugly to workaround (which as you note in this case is not great, but not > _too_ bad), but that it's a subtle friction point that may jump up and > bite any test-writer who does something like: > > (foo && bar) 2>stderr > > So my view had always been that BASH_XTRACEFD is the good > solution[...] Yes, I agree that's much better than what you need to do when not under bash and when you can't benefit from BASH_XTRACEFD. But unless we're talking about requiring bash and/or not supporting -x at all (which seems to be overkill, seeing as only one test scipt wasn't compatible with it) this discussion seems a bit like talking about how some code is nicer in C17 than C99 or C89. Sure, it is. But if you're also supporting the latter two it's usually not worth it to maintain both with an ifdef. Similarly, we can't really get any real benefits from BASH_XTRACEFD as long as we're going to support "-x" with other shells. Literally the only part of the test suite where we hard-depended on it is the code adjusted in my 1/2 here. I do think if we could rely on the pre-image it would be nicer, but not so nice that I don't want "-x" working under "dash". And in any case carrying a "BASH_XTRACEFD" seems to be just a dead end for that reason. On Wed, Dec 15 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote: >> >>> Still. I wonder if keeping BASH_XTRACEFD helps developers, when >>> they need to diagnose a new breakage? If their new test fails only >>> in the "dash -x" run but not "bash -x" at the CI, for example? >> >> I have done that, but usually only after realizing that "./t1234-foo.sh" >> passes and "./t1234-foo.sh -x" does not. So I think just tweaking use of >> "-x" would be the main tool. > > Ah, that's true. You only need to compare "sh -x test.sh" with "sh > test.sh" with any value of "sh", especially after BASH_XTRACEFD is > removed, demoting "bash" to the same state as "dash" wrt "-x". > > OK, I am more OK with the removal of BASH_XTRACEFD support than > before ;-) Yes, I've run into those cases and I don't think BASH_XTRACEFD really helps at all. Yes you'll get a test failure because trace output got into something we're invoking "test_cmp" on, or whatever. But that's never a subtle failure, and the fix for it brings us back to the question of whether we're going to support non-bash, or take the more drastic step of marking the whole test script as bash-only under "-x". I think the answers to those are always going to be "yes" and "no", and thus we're not getting any benefit from "BASH_XTRACEFD".
diff --git a/t/README b/t/README index 29f72354bf1..3d30bbff34a 100644 --- a/t/README +++ b/t/README @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e. -x:: Turn on shell tracing (i.e., `set -x`) during the tests themselves. Implies `--verbose`. - Ignored in test scripts that set the variable 'test_untraceable' - to a non-empty value, unless it's run with a Bash version - supporting BASH_XTRACEFD, i.e. v4.1 or later. -d:: --debug:: diff --git a/t/test-lib.sh b/t/test-lib.sh index 57efcc5e97a..b008716917b 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -381,29 +381,6 @@ then exit fi -if test -n "$trace" && test -n "$test_untraceable" -then - # '-x' tracing requested, but this test script can't be reliably - # traced, unless it is run with a Bash version supporting - # BASH_XTRACEFD (introduced in Bash v4.1). - # - # Perform this version check _after_ the test script was - # potentially re-executed with $TEST_SHELL_PATH for '--tee' or - # '--verbose-log', so the right shell is checked and the - # warning is issued only once. - if test -n "$BASH_VERSION" && eval ' - test ${BASH_VERSINFO[0]} -gt 4 || { - test ${BASH_VERSINFO[0]} -eq 4 && - test ${BASH_VERSINFO[1]} -ge 1 - } - ' - then - : Executed by a Bash version supporting BASH_XTRACEFD. Good. - else - echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" - trace= - fi -fi if test -n "$trace" && test -z "$verbose_log" then verbose=t @@ -650,19 +627,6 @@ else exec 4>/dev/null 3>/dev/null fi -# Send any "-x" output directly to stderr to avoid polluting tests -# which capture stderr. We can do this unconditionally since it -# has no effect if tracing isn't turned on. -# -# Note that this sets up the trace fd as soon as we assign the variable, so it -# must come after the creation of descriptor 4 above. Likewise, we must never -# unset this, as it has the side effect of closing descriptor 4, which we -# use to show verbose tests to the user. -# -# Note also that we don't need or want to export it. The tracing is local to -# this shell, and we would not want to influence any shells we exec. -BASH_XTRACEFD=4 - test_failure=0 test_count=0 test_fixed=0 @@ -949,36 +913,20 @@ test_eval_ () { # the shell from printing the "set +x" to turn it off (nor the saving # of $? before that). But we can make sure that the output goes to # /dev/null. - # - # There are a few subtleties here: - # - # - we have to redirect descriptor 4 in addition to 2, to cover - # BASH_XTRACEFD - # - # - the actual eval has to come before the redirection block (since - # it needs to see descriptor 4 to set up its stderr) - # - # - likewise, any error message we print must be outside the block to - # access descriptor 4 - # - # - checking $? has to come immediately after the eval, but it must - # be _inside_ the block to avoid polluting the "set -x" output - # - - test_eval_inner_ "$@" </dev/null >&3 2>&4 { + test_eval_inner_ "$@" </dev/null >&3 2>&4 test_eval_ret_=$? if want_trace then test 1 = $trace_level_ && set +x trace_level_=$(($trace_level_-1)) - fi - } 2>/dev/null 4>&2 - if test "$test_eval_ret_" != 0 && want_trace - then - say_color error >&4 "error: last command exited with \$?=$test_eval_ret_" - fi + if test "$test_eval_ret_" != 0 + then + say_color error >&4 "error: last command exited with \$?=$test_eval_ret_" + fi + fi + } 2>/dev/null return $test_eval_ret_ }
In the preceding commit the use of "test_untraceable=UnfortunatelyYes" was removed from "t1510-repo-setup.sh" in favor of more narrow redirections of the output of specific commands (and not entire sub-shells or functions). This is in line with the fixes in the series that introduced the "test_untraceable" facility. See 571e472dc43 (Merge branch 'sg/test-x', 2018-03-14) for the series as a whole, and e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a subshell, 2018-02-24) for a commit that's in line with the changes in the preceding commit. We've thus solved the TODO item noted when "test_untraceable" was added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark as untraceable with '-x', 2018-02-24). So let's remove the feature entirely. Not only is it currently unused, but it actively encourages an anti-pattern in our tests. We should be testing the output of specific commands, not entire subshells or functions. That the "-x" output had to be disabled as a result is only one symptom, but even under bash those tests will be harder to debug as the subsequent check of the redirected file will be far removed from the command that emitted the output. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/README | 3 --- t/test-lib.sh | 66 ++++++--------------------------------------------- 2 files changed, 7 insertions(+), 62 deletions(-)