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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2014, 21:56 UTC
Message-ID: <xmqqppdv1vnh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141013213808.GB32245@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 15 quoted lines

> Junio C Hamano wrote:
>> On Mon, Oct 13, 2014 at 2:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> It could segfault after producing the good output, but sure,
>>> count-objects code doesn't change very often.
>>
>> "Doesn't change very often" is not the issue. Here we are not testing
>> if it can count correctly without crashing, which *is* the real reason
>> why it is perfectly fine to use $(git count-objects | sed ...) pattern here.
>>
>> There of course should be a test for count-objects to make sure it
>> counts correctly without crashing.
>
> That also makes sense, but it is a pretty big change from the general
> strategy used in git tests today.

I would have to say that you are mistaken in reading what the "strategy used today" is. There is a subtle trade-off involved.

When we test, say, "git add a b", we may want to test these things:

    - "git add" when given addable paths a and b finishes without
      crashing;

    - "git add" will leave these paths in the index as expected.

And we write

	git add a b &&
	test_write_lines a b >expect &&
        git ls-files a b >actual &&
        test_cmp expect actual

NOT because we expect "printf" (which underlies test_write_lines) or "git ls-files" could somehow misbehave and dump core, but primarily because compared to an alternative, e.g.

	git add a b || return 1
        test_write_lines a b >expect
        git ls-files a b >actual
        test_cmp expect actual

it is far cleaner and easier to read with a rhythm. It is just an added bonus that we may catch errors due to filesystem quota when writing to "expect" or ls-files crashing. If the alternative had enough advantage over the established pattern (and here is where the trade off comes in---you need a certain taste to judge the advantage), it is perfectly fine to trade the exit value off and favor the advantage the alternative offers (e.g. a test that is easier to read).

Between these two, it is very sensible to take A. over B.

    A.
	git create-garbage &&
	test $(git count-objects | sed ... | wc -l) = 0

    B.
	git create-garbage &&
	test_when_finished "rm -f tmp" &&
        git count-objects >tmp &&
        test $(sed ... tmp | wc -l) = 0

It will shift the trade-off if the more verbose alternative gets wrapped into a helper that is well constructed, though, because readability advantage of A over B melts away when we do so.