Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Jul 6, 2024, 11:20 UTC
- Message-ID
- <75a635b9-7622-42f7-b202-6991775e75f0@gmail.com>
- In-Reply-To
- <20240706061850.GB700645@coredump.intra.peff.net>
On Sat, Jul 06, 2024 at 02:18:50AM -0400, Jeff King wrote:
Show 36 quoted lines
> On Wed, Jul 03, 2024 at 11:44:33PM +0200, Rubén Justo wrote: > > > > Explicitly indicating that the error is being forced due to > > > "GIT_TEST_SANITIZE_LEAK_LOG=true", for a test that doesn't fail when run > > > normally or even when run with just > > > "GIT_TEST_PASSING_SANITIZE_LEAK=yes", could save us some confusion. > > > > > > So, I dunno. > > > > > > Anyway, I agree that this can be addressed later. > > > > > > Thanks. > > > > Maybe what we should do is integrate "GIT_TEST_SANITIZE_LEAK_LOG" into > > "GIT_TEST_PASSING_SANITIZE_LEAK" because I'm not sure what value we get > > by keeping them separate (test performance?). But that's another topic, > > even further out of scope of this patch :-) > > I don't think we want to integrate them, but I'd suggest that > SANITIZE_LEAK_LOG should be the default/only option. > > Without it, you are potentially missing leaks in programs whose failing > exit codes do not trigger a test failure. So there is no point in > running PASSING_SANITIZE_LEAK=check without also checking the logs. But > it is still useful to set SANITIZE_LEAK_LOG just for normal runs to look > for leaks. > > I don't know of any reason we couldn't always check the logs (for a > leak-checking build), and I didn't see anything in the history. I think > it was written that way only because there is otherwise no affirmative > action by the user to say "and btw, look for leaks" (and if we are not > looking for leaks, there might not be any logs!). > > But really, if you have done a leak-checking build, then every run of > the tests is looking for leaks, whether you check the logs or not. So we > should able to just check that $SANITIZE_LEAK is set.
> And then there would be one less thing for people checking for leaks > to remember to set.
I completely agree.
Let's wait for the dust to settle after the fix in this series, and then I'll address the change as you described.
Thanks.