Re: [PATCH] t/README: clarify test_must_fail description

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 20, 2010, 18:00 UTC
Message-ID: <7veieym3sh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <0JXkybOAPrkw1RCkgKLY0ocfkmfqHFq_bWFMVWrzymAet2VX-veTSoZP1hBzIyN5JSrPw-IZjfI@cipher.nrlssc.navy.mil>

Brandon Casey <casey@nrlssc.navy.mil> writes:

Show 47 quoted lines

> From: Brandon Casey <drafnel@gmail.com>
>
> Some have found the wording of the description to be somewhat ambiguous
> with respect to when it is desirable to use test_must_fail instead of
> "! <git-command>".  Tweak the wording somewhat to hopefully clarify that
> it is _because_ test_must_fail can detect segmentation fault that it is
> desirable to use it instead of "! <git-command>".
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>
>
> On 07/20/2010 11:38 AM, Jared Hance wrote:
>> I think the wording of description of test_must_fail is slightly
>> ambiguous. I read it to mean that:
>> 
>>     Use test_must_fail only when you are testing to see if git will
>>     segfault.
>
> I think that is a correct interpretation.  But I ask you this:
> Are there times when we would _not_ want to test for segfault? :)
>
>> Rather than:
>>     
>>     Use test_must_fail to be safe from git segfaults.
>> 
>> 
>> Perhaps the description should be updated to be a bit more clear?
>
> How about this?
>
>
>  t/README |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/README b/t/README
> index b906ceb..a830daa 100644
> --- a/t/README
> +++ b/t/README
> @@ -451,8 +451,8 @@ library for your script to use.
>   - test_must_fail <git-command>
>  
>     Run a git command and ensure it fails in a controlled way.  Use
> -   this instead of "! <git-command>" to fail when git commands
> -   segfault.
> +   this instead of "! <git-command>" since it will fail when git
> +   commands segfault.

I found your earlier

   Use test_must_fail instead of "! <git-command>" since test_must_fail
   will fail when <git-command> segfaults.

slightly clearer.

 - "it" in "since it will fail" is a bit ambiguous: is it "!" or
   "test_must_fail"?

 - it is not obvious if "fail" in "since it will fail" is a good thing or
   a bad thing; as we are discussing test_MUST_fail, it may even be a good
   thing that it "will fail"---which is not what we want our audience to
   read from this.

How about being more explicit?

    Run a git command and ensure it fails in a controlled way.  Use
    this instead of "! <git-command>".  When git-command dies due to a
    segfault, test_must_fail diagnoses it as an error; "! <git-command>"
    treats it as just another expected failure. letting such a bug go
    unnoticed.

Show 10 quoted lines

>  
>   - test_might_fail <git-command>
>  
> -- 
> 1.6.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html