On Thu, Jul 15, 2010 at 18:44, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > 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. Well to clarify: The TAP is arguably right, although semantically these sort of tests should probably be a SKIP on unsupported platforms, not a passing TODO. prove(1) also features passing TODO tests a bit too prominently for my tastes. I've filed a bug for that: https://rt.cpan.org/Public/Bug/Display.html?id=59428 > 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. This is what we should be focusing on, the patch by Jonathan Nieder is a good start. -- 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