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@xxxxxxxxx>
> ---
> 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
>
