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

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

> +# 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:

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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to