Re: [PATCH v2 3/6] test-lib: verbose mode for only tests matching a pattern

From: Jeff King <peff@peff.net>
Date: Jun 18, 2013, 05:37 UTC
Message-ID: <20130618053710.GB5916@sigill.intra.peff.net>
In-Reply-To: <8739d3ba5947c60d402030fc35d47115e7a51b0f.1371460265.git.trast@inf.ethz.ch>

On Mon, Jun 17, 2013 at 11:18:48AM +0200, Thomas Rast wrote:

Show 5 quoted lines

> As suggested by Jeff King, this takes care to wrap the entire test_expect_*
> block, but nothing else, in the verbose toggling.  To that end we use
> a new pair of hook functions.  The placement is a bit weird because we
> need to wait until the beginning of test_skip for $test_count to be
> incremented.

I guess it is not surprising because I suggested it, but I find the new setup/teardown logic easier to follow than the old toggle. The weird placement did throw me for a minute while reading the patch. I wonder if it is worth pulling the increment out of test_skip, to have something like this in test_expect_*:

  test_start ;# increment number, run setup hooks
  if ! test_skip
  then
    ...
  fi
  test_finish ;# teardown hooks

Then it is a bit easier to see that each start has a finish (whereas in the current version, the setups in test_skip are matched by individual teardowns in each caller). I did not look too hard at it, though, so I wouldn't be surprised if there is some other hidden order dependency that makes that not work. :)

Show 11 quoted lines

> +# Called from test_skip after it has incremented $test_count.  This
> +# means it runs before any test-specific code and output.
> +test_setup_hook_ () {
> +	maybe_setup_verbose
> +}
> +
> +# Called at the end of test_expect_*.  This means it runs after all
> +# test-specific code and output.
> +test_teardown_hook_ () {
> +	maybe_teardown_verbose
> +}

So these do your verbose setup/teardown. Makes sense.

But then what is this hunk doing:

Show 13 quoted lines

>  test_eval_ () {
>  	# This is a separate function because some tests use
>  	# "return" to end a test_expect_success block early.
> @@ -358,9 +399,7 @@ test_run_ () {
>  
>  	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
>  	then
> -		setup_malloc_check
>  		test_eval_ "$test_cleanup"
> -		teardown_malloc_check
>  	fi
>  	if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"
>  	then

At first I thought these should go into your setup/teardown hooks, but I don't think so. They are made redundant by patch 1/6, which wraps all of test_eval with malloc setup/teardown, aren't they? Should this hunk be in patch 1?

-Peff