From: Junio C Hamano Date: Thu, 15 Jul 2010 17:47:52 GMT Subject: Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported Message-ID: <7vtyo0vdpz.fsf@alter.siamese.dyndns.org> In-Reply-To: Ævar Arnfjörð Bjarmason writes: > On Thu, Jul 8, 2010 at 19:40, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason   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_.