All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Taylor Blau" <me@ttaylorr.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility
Date: Fri, 10 Dec 2021 11:07:55 +0100	[thread overview]
Message-ID: <patch-v3-2.2-a7fc794e20d-20211210T100512Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com>

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


  parent reply	other threads:[~2021-12-10 10:08 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-29 13:47 [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh Derrick Stolee via GitGitGadget
2021-11-29 13:47 ` [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING Derrick Stolee via GitGitGadget
2021-11-29 14:12   ` Ævar Arnfjörð Bjarmason
2021-11-29 18:28     ` Junio C Hamano
2021-11-29 18:32     ` Junio C Hamano
2021-11-29 13:47 ` [PATCH 2/2] t/t*: remove custom GIT_TRACE2_EVENT_NESTING Derrick Stolee via GitGitGadget
2021-11-29 18:40 ` [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh Jeff King
2021-11-29 18:44   ` Jeff King
2021-11-30  0:04     ` Taylor Blau
2021-11-30 15:34   ` Derrick Stolee
2021-11-30 22:43     ` Jeff Hostetler
2021-12-01 19:46       ` Jeff King
2021-11-29 20:13 ` [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-11-29 23:23   ` Eric Sunshine
2021-11-30 21:08   ` Jeff King
2021-11-30 21:50     ` Ævar Arnfjörð Bjarmason
2021-11-30 22:44     ` SZEDER Gábor
2021-12-01 14:06       ` Ævar Arnfjörð Bjarmason
2021-12-01 19:38         ` Jeff King
2021-12-01 18:38   ` Junio C Hamano
2021-12-01 20:11   ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason
2021-12-01 20:11     ` [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-02 19:16       ` SZEDER Gábor
2021-12-02 19:28         ` SZEDER Gábor
2021-12-10  9:47         ` Jeff King
2021-12-10 10:08           ` Ævar Arnfjörð Bjarmason
2022-02-06 21:40           ` SZEDER Gábor
2021-12-01 20:11     ` [PATCH v2 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-10 10:07     ` [PATCH v3 0/2] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-12-10 10:07       ` [PATCH v3 1/2] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-10 10:07       ` Ævar Arnfjörð Bjarmason [this message]
2021-12-12 16:32         ` [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility SZEDER Gábor
2021-12-12 17:06           ` Ævar Arnfjörð Bjarmason
2021-12-12 20:14             ` SZEDER Gábor
2021-12-13 18:51               ` Junio C Hamano
2021-12-14 16:43                 ` Jeff King
2021-12-15 17:05                   ` Junio C Hamano
2021-12-15 17:17                     ` Jeff King
2021-12-15 17:32                       ` Junio C Hamano
2021-12-16 13:04                         ` Ævar Arnfjörð Bjarmason
2021-12-13  1:38       ` [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-12-13  1:38         ` [PATCH v4 1/3] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-13  1:38         ` [PATCH v4 2/3] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-13  1:38         ` [PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD" Ævar Arnfjörð Bjarmason
2022-02-21 23:11           ` SZEDER Gábor
2022-02-22 15:14             ` Ævar Arnfjörð Bjarmason
2021-12-13  5:43         ` [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD SZEDER Gábor
2022-02-21 19:52           ` Ævar Arnfjörð Bjarmason
2022-02-21 21:03             ` SZEDER Gábor
2022-02-21 22:41               ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-v3-2.2-a7fc794e20d-20211210T100512Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.