Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

From

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

Date

Feb 26, 2019, 19:39 UTC

Message-ID

<20190226193912.GD19739@szeder.dev>

In-Reply-To

<20190226174316.GD19606@sigill.intra.peff.net>

On Tue, Feb 26, 2019 at 12:43:17PM -0500, Jeff King wrote:

> On Tue, Feb 26, 2019 at 06:04:00PM +0100, SZEDER Gábor wrote:
> 
> > > Whereas:
> > > 
> > >     + test -f doesnotexist
> > >     + echo File doesnotexist doesn't exist.
> > >     File doesnotexist doesn't exist.
> > >     + false
> > >     error: last command exited with $?=1
> > > 
> > > Gives me the same thing, but I have to read 5 lines instead of 2 that
> > > ultimately don't tell me any more (and a bit of "huh, 'false' returned
> > > 1? Of course! Oh! It's faking things up and it's the 'echo' that
> > > matters...").
> > 
> > I didn't find this to be an issue, but because of functions like
> > 'test_seq' and 'test_must_fail' I've thought about suppressing '-x'
> > output for test helpers (haven't actually done anything about it,
> > though).
> 
> I'd be curious how you'd do that.

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..16adcd54c9 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -664,7 +664,15 @@ list_contains () {
 #     Currently recognized signal names are: sigpipe, success.
 #     (Don't use 'success', use 'test_might_fail' instead.)
 
+restore_tracing () {
+	if test -n "$trace"
+	then
+		set -x
+	fi
+} 2>/dev/null 4>/dev/null
+
 test_must_fail () {
+	{ set +x ; } 2>/dev/null 4>/dev/null
 	case "$1" in
 	ok=*)
 		_test_ok=${1#ok=}
@@ -679,24 +687,29 @@ test_must_fail () {
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
 		echo >&4 "test_must_fail: command succeeded: $*"
+		restore_tracing
 		return 1
 	elif test_match_signal 13 $exit_code && list_contains "$_test_ok" sigpipe
 	then
+		restore_tracing
 		return 0
 	elif test $exit_code -gt 129 && test $exit_code -le 192
 	then
 		echo >&4 "test_must_fail: died by signal $(($exit_code - 128)): $*"
+		restore_tracing
 		return 1
 	elif test $exit_code -eq 127
 	then
 		echo >&4 "test_must_fail: command not found: $*"
+		restore_tracing
 		return 1
 	elif test $exit_code -eq 126
 	then
 		echo >&4 "test_must_fail: valgrind error: $*"
+		restore_tracing
 		return 1
 	fi
-	return 0
+	restore_tracing
 } 7>&2 2>&4
 
 # Similar to test_must_fail, but tolerates success, too.  This is


Yeah, it's a hassle, especially in a function with as many return
paths as 'test_must_fail', but look at its output:

  + test_must_fail git rev-parse nope --
  fatal: bad revision 'nope'
  + test_must_fail git rev-parse HEAD --
  48ab21c1a5972e0fa9d87da7c5da9982872b8db2
  test_must_fail: command succeeded: git rev-parse HEAD --
  + return 1
  error: last command exited with $?=1

Not even the 'set +x' shows up in the trace output!  Unfortunately,
that line is not particularly pleasing on the eyes, but I don't see
any way around that...

Perhaps we could even go one step further with this 'restore_tracing'
helper and add a parameter specifying its return code, so we could
make it the last command invoked in the test helper function, and then
even that 'return 1' would disappear from the trace output.
Furthermore, this would be helpful in those functions where the last
command's return code is relevant, e.g: 

  test_cmp() {
        { set +x ; } 2>/dev/null 4>/dev/null
        $GIT_TEST_CMP "$@"
        restore_tracing $?
  }


There are a couple of tricky cases:

  - Some test helper functions call other test helper functions, and
    in those cases tracing would be enabled upon returning from the
    inner helper function.  This is not an issue with e.g.
    'test_might_fail' or 'test_cmp_config', because the inner helper
    function is the last command anyway.  However, there is
    'test_must_be_empty', 'test_dir_is_empty', 'test_config',
    'test_commit', etc. which call the other test helper functions
    right at the start or in the middle.

  - && chains in test helper functions; we must make sure that the
    tracing is restored even in case of a failure.