Re: test &&-chain lint

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2015, 17:04 UTC
Message-ID: <xmqq384zha6s.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqd244go0h.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 31 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> [+cc Jonathan, whose patch I apparently subconsciously copied]
>>
>> On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:
>>
>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>> index c096778..02a03d5 100644
>>> --- a/t/test-lib.sh
>>> +++ b/t/test-lib.sh
>>> @@ -524,6 +524,21 @@ test_eval_ () {
>>>  test_run_ () {
>>>  	test_cleanup=:
>>>  	expecting_failure=$2
>>> +
>>> +	if test -n "$GIT_TEST_CHAIN_LINT"; then
>>> +		# 117 is unlikely to match the exit code of
>>> +		# another part of the chain
>>> +		test_eval_ "(exit 117) && $1"
>>> +		if test "$?" != 117; then
>>> +			# all bets are off for continuing with other tests;
>>> +			# we expected none of the rest of the test commands to
>>> +			# run, but at least some did. Who knows what weird
>>> +			# state we're in? Just bail, and the user can diagnose
>>> +			# by running in --verbose mode
>>> +			error "bug in the test script: broken &&-chain"
>>> +		fi
>>> +	fi
>> ...
> Hmmm, they do look similar and unfamiliar ;-)  It happened while I
> was offline, it seems.

One case where this might misdetect a good test would be this one:

    test_expect_success 'either succeed or fail with status 1' '
	git subcmd || case "$?" in 1) : happy ;; *) false failure ;; esac
    '

which would turn into

	(exit 117) && git subcmd || case ...

and fail to set $? to 117, triggering a false positive.

I do not have a good solution fo that, though. Obviously, turning the check into

	(exit 117) && {
        	$1
	}

misses the entire point of the chain-lint.

I wonder if another valid way to make it harder for us to commit "broken && chain" errors in our test may be to make it not an error in the first place. Do we know how buggy various implementations of shells are with respect to their handling of "set -e"?

We know that chaining commands with && is much less likely to be broken in various reimplementation of bourne shells, and that is the primary reason we stick to this style in our tests, but if everybody implements "set -e" reliably and consistently, flipping that bit in test_eval_ and removing the need to &&-cascade the commands might not be such a bad idea.

Just thinking aloud...