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
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. :)
> +# 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?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html