[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v5 26/26] t7700: stop losing return codes of git commands
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2019-11-30 17:00:08
Message-ID: xmqqv9r12tbb.fsf () gitster-ct ! c ! googlers ! com
[Download RAW message or body]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Nov 30, 2019 at 5:48 AM Danh Doan <congdanhqx@gmail.com> wrote:
>> On 2019-11-27 11:54:04-0800, Denton Liu <liu.denton@gmail.com> wrote:
>> > +   ! grep "^$coid " packlist &&
>>
>> I think we want to use test_must_fail instead of !
>
> test_must_fail() is intended only for use with 'git' commands; "!"
> should be used otherwise. Quoting from t/README:
>
>     Don't use '! git cmd' when you want to make sure the git command
>     exits with failure in a controlled way by calling "die()".  Instead,
>     use 'test_must_fail git cmd'.  This will signal a failure if git
>     dies in an unexpected way (e.g. segfault).
>
>     On the other hand, don't use test_must_fail for running regular
>     platform commands; just use '! cmd'.  We are not in the business
>     of verifying that the world given to us sanely works.
>
> So, Denton's use of "!" here is correct.

I wonder we can make the framework a bit more self-documenting to
avoid having to waste time on discovering potential issues and
explaining why it is not an issue, like this exchange.

Some ideas:

 * Perhaps test_must_fail is not descriptive enough that it should
   apply only to git command invocation.  Would it make it more
   obvious to rename it to say git_must_fail?  That would also make
   it unnecessary to give this rather unfortunate comment in
   test-lib-functions.sh:

   # This is not among top-level (test_expect_success | test_expect_failure)
   # but is a prefix that can be used in the test script, like:
   #
   #	test_expect_success 'complain and die' '
   #           do something &&
   #           do something else &&
   #	    test_must_fail git checkout ../outerspace
   #	'
   #

 * If it is too much trouble to rename it, perhaps test_must_fail
   can be documented better up there?

   diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
   index b299ecc326..052d88c5da 100644
   --- a/t/test-lib-functions.sh
   +++ b/t/test-lib-functions.sh
   @@ -817,6 +817,13 @@ list_contains () {
    #     Multiple signals can be specified as a comma separated list.
    #     Currently recognized signal names are: sigpipe, success.
    #     (Don't use 'success', use 'test_might_fail' instead.)
   +#
   +# Do not use this to run anything but "git".  We are not in the business
   +# of vetting system supplied commands---IOW this is wrong:
   +#
   +#    test_must_fail grep pattern output
   +#
   +# Just use '!' instead.

    test_must_fail () {
           case "$1" in

 * Or perhaps we can detect its use on anything that is not "git"
   automatically?  This is merely to illustrate the idea (the
   exemption of "env" shown here is too broad for production use)

   diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
   index b299ecc326..7ab113cd50 100644
   --- a/t/test-lib-functions.sh
   +++ b/t/test-lib-functions.sh
   @@ -828,6 +828,10 @@ test_must_fail () {
                   _test_ok=
                   ;;
           esac
   +	case "$1" in
   +	git|test-tool|env) ;;
   +	*) echo >&7 "warning: test_must_fail $*???" ;;
   +	esac
           "$@" 2>&7
           exit_code=$?
           if test $exit_code -eq 0 && ! list_contains "$_test_ok" success

Hmm?
[prev in list] [next in list] [prev in thread] [next in thread]