On Thu, Jul 15, 2010 at 17:47, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html