Re: [PATCH 08/16] t5304: use helper to report failure of "test foo = bar"

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Wed, Oct 08, 2014 at 12:17:07AM +0200, Michael Haggerty wrote:
>
>> On 10/07/2014 11:53 PM, Junio C Hamano wrote:
>> > Hmph, your 'test' in that name is a generic verb "we check that...",
>> > which I think aligns better with the other test_foo functions.  When
>> > I suggested 'test_verbose', 'test' in that name was specifically
>> > meant to refer to the 'test' command.
>
> I actually meant "test" as a namespace to indicate it is part of the
> test suite (just like "test_seq" is not testing anything). I think that
> is why the names are so silly. We are using the "test" command in our
> "test" suite to "test" some conditions.
>
>> I like "verbose_test $foo = $bar" because it puts the word "test" next
>> to the condition, where the built-in command "test" would otherwise be.
>> 
>> We could even define a command
>> 
>> 	verbose () {
>> 		"$@" && return 0
>> 		echo >&2 "command failed: $*"
>> 		return 1
>> 	}
>> 
>> and use it like
>> 
>> 	verbose test $foo = $bar
>
> I kind of like this. It is easy to see which shell command is being
> invoked, and it would extend naturally to other silent commands.
>
>> Somehow I feel like I'm reinventing something that must already exist...
>
> Yes, we're basically reinventing "set -x" here, with the caveat that we
> only _really_ care about showing failed commands. The problem with "set
> -x" is that it also wants to apply itself to the test harness itself, so
> you end up with a lot of cruft.

As you have done, I've done my share of running tests under "sh -x",
and I agree that it gives too much noise. Which is almost impossible
to read through without a lot of patience, but at least it contains
everything we could ask, short of running a specific step under gdb.

The "verbose" thing that reports only when the expectation is violated
cuts down the cruft (so does your original test_cond/verbose_test),
so it may make it easier than the bare-metal "sh -x".

But it may cut down a bit too much.  Often in a test that consists
of multiple commands chained together with && inside a single
test_expect_success, what leads to the eventual unsatisfied
expectation is an earlier step that succeeds in the sense that it
satisfies the expectation of that step (e.g. "git branch foo &&" and
we only check it did not exit with non-zero status) but did not
actually do what we wanted it to do (e.g. that "git branch" did not
create a new branch 'foo' but created 'refs/foo' by mistake), and
then a later step is tripped by the unsatisified expectation that
was not explicitly tested (e.g. after that "git checkout foo" would
not quite fail but instead of checking out a branch to build upon,
it detaches the HEAD at that commit, because the earlier "git branch"
was faulty but we did not notice it).

So I dunno.  For harder cases we always have to resort to "sh -x",
but "verbose" thing may still help easier cases, just like the use
of test_cmp instead of cmp is helping us while viewing output from
running test with the "-v" option today, and that would be a good
enough improvement.