[PATCH 1/2] tests: don't mess with fd 7 of test helper functions

* [PATCH 1/2] tests: don't mess with fd 7 of test helper functions
@ 2021-02-21 19:25 SZEDER Gábor
  2021-02-21 19:25 ` [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' SZEDER Gábor
  2021-02-21 21:50 ` [PATCH 1/2] tests: don't mess with fd 7 of test helper functions Jeff King
  0 siblings, 2 replies; 8+ messages in thread
From: SZEDER Gábor @ 2021-02-21 19:25 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Denton Liu, SZEDER Gábor

In test helper functions exercising git commands, e.g.
'test_must_fail', 'test_env' and friends, we can't access the test
script's original standard error, and, consequently, BUG() doesn't
work as expected.

The root of the issue stems from a5bf824f3b (t: prevent '-x' tracing
from interfering with test helpers' stderr, 2018-02-25), where we
started to use a couple of file descriptor duplications and
redirections to separate the standard error of git commands exercised
in test helper functions from the stderr containing the '-x' trace
output of said helper functions.  To achieve that the git command's
stderr is redirected to the test helper function's fd 7, which was
previously duplicated from the helper's stderr.  Alas, fd 7 was not
the right choice for this purpose, because fd 7 is the original
standard error of the test script, and, consequently, we now can't
send error messages from within such test helper functions directly to
the test script's stderr.  Since BUG() does want to send its error
message there it doesn't work as expected in such test helper
functions, because:

  - If the test helper's stderr were redirected to a file (as is often
    the case e.g. with 'test_must_fail'), then the "bug in the test
    script" error message would end up in that file.

  - If the test script is invoked without any of the verbose options,
    then that error message would get lost to /dev/null, leaving no
    clues about why the test script aborted so suddenly.

We don't have any BUG() calls in such test helper functions yet, but
6a67c75948 (test-lib-functions: restrict test_must_fail usage,
2020-07-07) did start to verify the parameters of 'test_must_fail' and
report the error with:

    echo >&7 "test_must_fail: only 'git' is allowed: $*"
    return 1

instead of BUG() that we usually use to bail out in case of bogus
parameters.

Use fd 6 instead of fd 7 for these '-x' tracing related duplications
and redirections.  It is a better choice for this purpose, because fd
6 is the test script's original standard input, and neither these test
helper functions not the git commands exercised by them should ever
read from the test scipt's stdin, see 781f76b158 (test-lib: redirect
stdin of tests, 2011-12-15).  Update the aforementioned error
reporting in 'test_must_fail' to send the error message to fd 6 as
well; the next patch will update it to use BUG() instead.

The only two other functions that want to directly write to the test
script's stderr are 'test_pause' and 'debug', but they are not
affected by this issue, because, being special "interactive" debug
aids, their fd 7 were not redirected in a5bf824f3b.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-terminal.sh       |  4 ++--
 t/test-lib-functions.sh | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index e3809dcead..454909c087 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -9,8 +9,8 @@ test_terminal () {
 		echo >&4 "test_terminal: need to declare TTY prerequisite"
 		return 127
 	fi
-	perl "$TEST_DIRECTORY"/test-terminal.perl "$@" 2>&7
-} 7>&2 2>&4
+	perl "$TEST_DIRECTORY"/test-terminal.perl "$@" 2>&6
+} 6>&2 2>&4
 
 test_lazy_prereq TTY '
 	test_have_prereq PERL &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 05dc2cc6be..a40c1c5d83 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -910,10 +910,10 @@ test_must_fail () {
 	esac
 	if ! test_must_fail_acceptable "$@"
 	then
-		echo >&7 "test_must_fail: only 'git' is allowed: $*"
+		echo >&6 "test_must_fail: only 'git' is allowed: $*"
 		return 1
 	fi
-	"$@" 2>&7
+	"$@" 2>&6
 	exit_code=$?
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
@@ -936,7 +936,7 @@ test_must_fail () {
 		return 1
 	fi
 	return 0
-} 7>&2 2>&4
+} 6>&2 2>&4
 
 # Similar to test_must_fail, but tolerates success, too.  This is
 # meant to be used in contexts like:
@@ -952,8 +952,8 @@ test_must_fail () {
 # Accepts the same options as test_must_fail.
 
 test_might_fail () {
-	test_must_fail ok=success "$@" 2>&7
-} 7>&2 2>&4
+	test_must_fail ok=success "$@" 2>&6
+} 6>&2 2>&4
 
 # Similar to test_must_fail and test_might_fail, but check that a
 # given command exited with a given exit code. Meant to be used as:
@@ -965,7 +965,7 @@ test_might_fail () {
 test_expect_code () {
 	want_code=$1
 	shift
-	"$@" 2>&7
+	"$@" 2>&6
 	exit_code=$?
 	if test $exit_code = $want_code
 	then
@@ -974,7 +974,7 @@ test_expect_code () {
 
 	echo >&4 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
 	return 1
-} 7>&2 2>&4
+} 6>&2 2>&4
 
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
@@ -1261,8 +1261,8 @@ test_write_lines () {
 }
 
 perl () {
-	command "$PERL_PATH" "$@" 2>&7
-} 7>&2 2>&4
+	command "$PERL_PATH" "$@" 2>&6
+} 6>&2 2>&4
 
 # Given the name of an environment variable with a bool value, normalize
 # its value to a 0 (true) or 1 (false or empty string) return code.
@@ -1388,13 +1388,13 @@ test_env () {
 				shift
 				;;
 			*)
-				"$@" 2>&7
+				"$@" 2>&6
 				exit
 				;;
 			esac
 		done
 	)
-} 7>&2 2>&4
+} 6>&2 2>&4
 
 # Returns true if the numeric exit code in "$2" represents the expected signal
 # in "$1". Signals should be given numerically.
@@ -1436,9 +1436,9 @@ nongit () {
 		GIT_CEILING_DIRECTORIES=$(pwd) &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non-repo &&
-		"$@" 2>&7
+		"$@" 2>&6
 	)
-} 7>&2 2>&4
+} 6>&2 2>&4
 
 # convert function arguments or stdin (if not arguments given) to pktline
 # representation. If multiple arguments are given, they are separated by
-- 
2.30.1.940.gce394404de


^ permalink raw reply related	[flat|nested] 8+ messages in thread