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

From: Jeff King <peff@peff.net>
Date: Mar 5, 2019, 04:55 UTC
Message-ID: <20190305045535.GI19800@sigill.intra.peff.net>
In-Reply-To: <20190303160459.GB28939@szeder.dev>

On Sun, Mar 03, 2019 at 05:04:59PM +0100, SZEDER Gábor wrote:

Show 14 quoted lines

> > >   - && 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, good point.

Show 7 quoted lines

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

Yeah, that might be less sneaky than this wrapper business. Or we could just do a few basic wrappers. The non-portable bit in my wrapper suggestion was the renaming of the old function. But if we accept just:

  real_foo() {
	... do stuff with multiple returns ...
  }
  disable_function_tracing real_foo foo

then that is pretty trivial to do with an eval. It does disallow your "wrap all functions at once", but I think that is OK. We might want to only do a subset anyway.

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

Yeah, it would probably make sense in the "push" half to check that we are actually tracing at that moment.

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

I'd say probably wrong, though it likely doesn't matter that much in practice.

-Peff