Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

From: Jeff King <peff@peff.net>
Date: Jul 6, 2024, 06:18 UTC
Message-ID: <20240706061850.GB700645@coredump.intra.peff.net>
In-Reply-To: <72b69a20-3f51-4f51-8ebc-ead20e3eebcb@gmail.com>

On Wed, Jul 03, 2024 at 11:44:33PM +0200, Rubén Justo wrote:

Show 15 quoted lines

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

-Peff