Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 15, 2010, 18:44 UTC
Message-ID: <AANLkTim-4I5Sdu653yJOmnVYDqI8bb71839Vqfd3FKCi@mail.gmail.com>
In-Reply-To: <7vtyo0vdpz.fsf@alter.siamese.dyndns.org>

On Thu, Jul 15, 2010 at 17:47, Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>
>>>> +if git grep ile a
>>>> +then
>>>> +     test_expect_success 'git grep ile a' 'git grep ile a'
>>>> +else
>>>> +     test_expect_failure 'git grep ile a' 'git grep ile a'
>>>> +fi
>>>
>>> So if command "X" is known to succeed, we run it inside expect_success
>>> and if not we run it inside expect_failure?
>>>
>>> What kind of idiocy is that, I have to wonder...
>>
>> Well, the point is to normalize the test suite so that we never have
>> passing TODO tests if everything's OK.
>
> I do not consider a test that passes under some condition but doesn't
> under some other condition "everything is OK".  Marking the test as
> "expect failure" as René originally did makes a lot of sense to me.
>
> The quoted patch is even worse as it will _actively_ prevent you from
> catching a new error you just introduced while futzing "git grep" on a
> platform that used to work.  Your "if" statement will say "ah, grep is
> broken", and you will use expect-failure, not because your platform does
> not support REG_STARTEND, but because you broke "git grep".
>
> The point of having tests is to help you catch your bugs while you
> develop.  A test that turns itself off when the feature it is testing is
> broken helps nobody.
>
> So forget about "passing TODO tests", whatever a "TODO test" is.  The
> change in question is actively _wrong_.

I was under the impression that REG_STARTEND was considered purely icing on the cake, i.e. that the tests should be passing whether or not it was present.

I guess my reasoning at the time was that if that wasn't the case, reporting an unexpected pass by default, as opposed to a failing TODO on platforms without REG_STARTEND. Since only TAP will report this, I thought that was just an omission.

Anyway, since REG_STARTEND *isn't* obviously considered icing you're of course right, but the test is still broken as-is. Now it reports an abnormal condition if REG_STARTEND is present (passing TODO test), it should instead have a failing TODO test where REG_STARTEND isn't present. I'll come up with a patch to fix that.

We should also just upgrade the GNU regex library in compat/regex to the version that supports REG_STARTEND. Unfortunately that seems easier said than done, since the library is now part of glibc, and has aquired a lot of glibc specific macros and other constructs that would need to be #defined away or otherwise worked around.

Thanks for the review.