Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99

[Jeff King <peff@peff.net>]

On Thu, Jun 09, 2022 at 10:09:25AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > IMHO the whole "let's trigger BUG() intentionally via test-tool" stuff
> > in t1406 is misguided. It's literally introducing broken code that is
> > not run in the normal Git binary in order to make sure that it's broken.
> 
> I haven't looked at that ref code in particular, but in general adding
> coverage for the case we actually expect to defend against with BUG()
> via a test-tool is something I think we could use in more cases.
> 
> E.g. parse-options.c will BUG() out on various bad options structs,
> there it makes sense to feed those bad structs in to make sure our
> assertion code is doing what we still expect, but we don't have those
> tests.

These seem like low-value tests to me. Once you've identified a case in
parse-options.c that should BUG(), it's not very many lines of code to
detect it and call the function. But to then create a test case,
including a harness that triggers the internal code in such a way that
is designed never to happen in the production code, seems much more
error prone to me.

I.e., it is very unlikely to me that such a test case will find an
actual problem, and it carries a lot of scaffolding cost.

Meanwhile the much more likely problem is that a BUG() can be triggered,
but we simply don't have test coverage of the right cases. Once you've
identified the case, the problem can be demonstrated (and fixed) in the
production code.

> I didn't know about that Jenkins case, but in any case I think we can
> probably stop worrying about this given that we haven't had complaints
> about ac14de13b22 (t4058: explore duplicate tree entry handling in a bit
> more detail, 2020-12-11) (although I've noticed in in "dmesg" before).

I didn't notice those ones. It's probable that the CI instance I was
using at GitHub changed under the hood. And GitHub is more likely than
most people to run git's test suite in a CI environment. :) So yeah, it
may not matter that much. I do still think dumping cores around the
filesystem (remembering that they do not always go into the trash
directory) and polluting the kernel log are a bit unfriendly. If we can
avoid it easily, it might be worth doing.

> On the other hand those segfaults in t4058 should probably be converted
> to a BUG()...

Agreed. Those stack-exhaustion ones (which were what originally alerted
me to the Jenkins issue) are hard to catch.

-Peff