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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 8, 2014, 16:58 UTC
Message-ID: <xmqq7g0acxc5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141008011304.GA26864@peff.net>

Jeff King <peff@peff.net> writes:

Show 37 quoted lines

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