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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Mar 3, 2019, 16:04 UTC
Message-ID: <20190303160459.GB28939@szeder.dev>
In-Reply-To: <20190226210101.GA27914@sigill.intra.peff.net>

On Tue, Feb 26, 2019 at 04:01:01PM -0500, Jeff King wrote:

Show 6 quoted lines

> On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:
> 
> > > > 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).

Show 71 quoted lines

> > 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.
> 
> Yeah, this is inherently a global flag that we're playing games with. It
> does seem like it would be easy to get it wrong. I guess the right model
> is considering it like a stack, like:
> 
> -- >8 --
> #!/bin/sh
> 
> x_counter=0
> pop_x() {
> 	ret=$?
> 	case "$x_counter" in
> 	0)
> 		echo >&2 "BUG: too many pops"
> 		exit 1
> 		;;
> 	1)
> 		x_counter=0
> 		set -x
> 		;;
> 	*)
> 		x_counter=$((x_counter - 1))
> 		;;
> 	esac
> 	{ return $ret; } 2>/dev/null
> }
> 
> # you _must_ call this as "{ push_x; } 2>/dev/null" to avoid polluting
> # trace output with the push call
> push_x() {
> 	set +x 2>/dev/null
> 	x_counter=$((x_counter + 1))
> }
> 
> bar() {
> 	{ push_x; } 2>/dev/null
> 	echo in bar
> 	pop_x
> }
> 
> foo() {
> 	{ push_x; } 2>/dev/null
> 	echo in foo, before bar
> 	bar
> 	echo in foo, after bar
> 	false
> 	pop_x
> }
> 
> set -x
> foo
> echo \$? is $?
> -- 8< --
> 
> I wish there was a way to avoid having to do the block-and-redirect in
> the push_x calls in each function, though.
> 
> I dunno. I do like the output, but this is rapidly getting complex.
> 
> >   - && chains in test helper functions; we must make sure that the
> >     tracing is restored even in case of a failure.

Actually, the && chain is not really an issue, because we can simply break the && chain at the very end:

  test_func () {
        { disable_tracing ; } 2>/dev/null 4>&2
        do this &&
        do that
        restore_tracing
  }

and make restore_tracing exit with $? (like you did above in pop_x()).

> Yeah, there is no "goto out" to help give a common exit point from the
> function. You could probably do it with a wrapper, like:

Yeah, the wrapper works. There are only a few test helper functions with multiple 'return' statements, and refactoring them to have a single 'return $ret' at the end worked, too.

Show 21 quoted lines

>   foo() {
> 	{ push_x; } 2>/dev/null
> 	real_foo "$@"
> 	pop_x
>   }
> 
> and then real_foo() is free to return however it likes. I wonder if you
> could even wrap that up in a helper:
> 
>   disable_function_tracing () {
> 	# rename foo() to orig_foo(); this works in bash, but I'm not
> 	# sure if there's a portable way to do it (and ideally one that
> 	# wouldn't involve an extra process).
> 	eval "real_$1 () $(declare -f $1 | tail -n +2)"
> 
> 	# and then install a wrapper which pushes/pops tracing
> 	eval "$1 () { { push_x; } 2>/dev/null; real_$1 \"\$@\"; pop_x; }"
>   }
> 
>   foo () { .... }
>   disable_function_tracing foo

We can wrap all functions at once:

  eval "$(declare -f \
                test_cmp \
                test_cmp_bin \
                <....> \
                write_script |
        sed -e 's%^\([a-zA-Z0-9_]*\) ()% \
                \1 () { \
                        { disable_tracing; } 2>/dev/null 4>/dev/null \
                        real_\1 \"\$@\" \
                        restore_tracing \
                } \
                real_\1 ()%')"

Yeah, not particularly pretty, but with the s/// command broken up into several lines it's not all that terrible either... And at least it doesn't need extra processes for each wrapped function.

We should also be careful and don't switch on tracing when returning from test helper functions invoked outside of tests, e.g. 'test_create_repo' while initializing the trash directory or 'test_set_port' while sourcing a daemon-specific lib.

Alas, 'declare' is Bash-only, and I don't see any way around that. Bummer.

On a mostly unrelated note, but I just noticed it while playing around with this: 't0000'-basic.sh' runs its internal tests with $SHELL_PATH instead of $TEST_SHELL_PATH. I'm not sure whether that's right or wrong.