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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Oct 7, 2014, 22:17 UTC
Message-ID: <54346663.7090006@alum.mit.edu>
In-Reply-To: <xmqqiojvczrd.fsf@gitster.dls.corp.google.com>

On 10/07/2014 11:53 PM, Junio C Hamano wrote:

Show 52 quoted lines

> Jeff King <peff@peff.net> writes:
> 
>> On Tue, Oct 07, 2014 at 01:35:15PM -0700, Junio C Hamano wrote:
>>
>>> Yeah, if we are going to reduce it down to the above implementation,
>>> intereseting things like "test -f $frotz" will become possible and
>>> "cmp-str" stops making sense.  It really is about "We run test and
>>> expect it to yield true.  Report the failure a bit more prominently
>>> under the '-v' option to help us debug".
>>
>> We already have test_path_is_file to do the same thing just for "-f". We
>> could in theory switch all of those to this new, more generic wrapper. I
>> don't know if it is worth doing a mass-conversion, but we could
>> discourage test_path_is_file in new tests. We could also implement
>> test_path_is_{dir,file} on top of this.
> 
> Oh, I wasn't going in that direction when I mentioned "-f"; I just
> wanted to say that 'test "$@"' is clearly about 'test' (/bin/test or
> shell built-in) and less about 'compare string'.  I do not think it
> is necessarily a good direction to go in to replace test-path-is-file
> with the test_cond thing; after all, type specific tests have chance
> to report breakage of expectation in type specifc ways, e.g.
> 
> 	test_path_is_file () {
> 		test -f "$1" && return 0
> 		echo >&2 "expected '$1' to be file"
> 		if test -e "$1"
>                 then
> 	               	echo >&2 "but it is missing"
> 		else
>                 	echo >&2 "but it is a non-file"
> 			ls >&2 -ld "$1"
> 		fi
>                 return 1
> 	}
> 
> But that is also just in theory ;-).
> 
>>> So among the ones you listed, test_verbose may be the least silly, I
>>> would think.
>>
>> Somehow test_verbose seems to me like checking the "verbose" option of
>> the test suite. I prefer "test_cond", but I do not feel too strongly, if
>> you want to override me.
> 
> 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.
> 
> Still "test_cond" feels somewhat funny, as "we check that..." always
> validates some condition, but I don't think of anything better X-<.

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

Somehow I feel like I'm reinventing something that must already exist...

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu