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
