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

From: Jeff King <peff@peff.net>
Date: Jun 9, 2022, 15:23 UTC
Message-ID: <YqIQghPvB/rbkos8@coredump.intra.peff.net>
In-Reply-To: <220609.86czfitfgz.gmgdl@evledraar.gmail.com>

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

Show 12 quoted lines

> > 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