Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

["SZEDER Gábor" <szeder.dev@gmail.com>]

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
>