[PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

In the if-else's chain we have in "check_test_results_san_file_", we
consider three variables: $passes_sanitize_leak, $sanitize_leak_check
and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at
that point).

For the first two variables we have different considerations depending
on the value of $test_failure, which makes sense.  However, for the
third, GIT_TEST_SANITIZE_LEAK_LOG, we don't;  regardless of
$test_failure, we use "invert_exit_code=t" to produce a non-zero
return value.

That assumes "$test_failure" is always zero at that point.  But it
may not be:

   $ git checkout v2.40.1
   $ make test SANITIZE=leak T=t3200-branch.sh # this fails
   $ make test SANITIZE=leak GIT_TEST_SANITIZE_LEAK_LOG=true T=t3200-branch.sh # this succeeds
   [...]
   With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status!
   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code

We need to use "invert_exit_code=t" only when "$test_failure" is zero.

Let's add the missing conditions in the if-else's chain to make it work
as expected.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This has already been sent: 
https://lore.kernel.org/git/54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com/

I have simplified the message a little, but the change remains the same.

Thanks.

 t/test-lib.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 79d3e0e7d9..7ed6d3fc47 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1269,9 +1269,12 @@ check_test_results_san_file_ () {
 	then
 		say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
 		invert_exit_code=t
-	else
+	elif test "$test_failure" = 0
+	then
 		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
 		invert_exit_code=t
+	else
+		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
 	fi
 }
 
-- 
2.45.1

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

On Sun, Jun 30, 2024 at 08:42:06AM +0200, Rubén Justo wrote:

> This has already been sent: 
> https://lore.kernel.org/git/54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com/

Thanks for that link. As soon as I read the subject, I thought "Uh oh,
wasn't there some tricky complexity here?". But going back to that
thread explained it all. :)

I think the patch you've sent here covers what was discussed there, and
is the right thing to do.

-Peff

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jun 30, 2024 at 08:42:06AM +0200, Rubén Justo wrote:
>
>> This has already been sent: 
>> https://lore.kernel.org/git/54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com/
>
> Thanks for that link. As soon as I read the subject, I thought "Uh oh,
> wasn't there some tricky complexity here?". But going back to that
> thread explained it all. :)
>
> I think the patch you've sent here covers what was discussed there, and
> is the right thing to do.

Thanks, both.
Will queue.

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> In the if-else's chain we have in "check_test_results_san_file_", we
> consider three variables: $passes_sanitize_leak, $sanitize_leak_check
> and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at
> that point).

Before this paragraph, we'd probably want to say what problem we are
fixing.  Using the verb "fix" on the subject line without saying what
broken behaviour you see around GIT_TEST_SANITIZE_LEAK_LOG does not
help, either.

Your patch from September 2023 [*] did mention it upfront:

    GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will
    make the test return zero unintentionally.

With that inserted in front of the proposed log message, the
resulting explanation looks reasonable to me.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 79d3e0e7d9..7ed6d3fc47 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () {
>  	then
>  		say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
>  		invert_exit_code=t
> -	else
> +	elif test "$test_failure" = 0
> +	then
>  		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
>  		invert_exit_code=t
> +	else
> +		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
>  	fi
>  }

This is outside the scope of this patch simply because it is
inherited from the original, but does ", exit non-zero!"  part of
the message really add any value?  I am wondering if

	else
-		say "With GIT_TEST_SANITIZE_LEAK_LOG=true ..., exit non-zero!"
+		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a leak."
+		test "$test_failure" != 0 || invert_exit_code=t
	fi

might be what we eventually want to end up with, after the dust
settles from this fix.

Thanks.

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

On Mon, Jul 01, 2024 at 01:19:34PM -0700, Junio C Hamano wrote:

> Your patch from September 2023 [*] did mention it upfront:
> 
>     GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will
>     make the test return zero unintentionally.
> 
> With that inserted in front of the proposed log message, the
> resulting explanation looks reasonable to me.

I see that you have already added this paragraph to what is already in
"seen". OK.

> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 79d3e0e7d9..7ed6d3fc47 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () {
> >  	then
> >  		say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
> >  		invert_exit_code=t
> > -	else
> > +	elif test "$test_failure" = 0
> > +	then
> >  		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
> >  		invert_exit_code=t
> > +	else
> > +		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
> >  	fi
> >  }
> 
> This is outside the scope of this patch simply because it is
> inherited from the original, but does ", exit non-zero!"  part of
> the message really add any value?

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.

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

On Wed, Jul 03, 2024 at 11:35:48PM +0200, Rubén Justo wrote:
> On Mon, Jul 01, 2024 at 01:19:34PM -0700, Junio C Hamano wrote:
> 
> > Your patch from September 2023 [*] did mention it upfront:
> > 
> >     GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will
> >     make the test return zero unintentionally.
> > 
> > With that inserted in front of the proposed log message, the
> > resulting explanation looks reasonable to me.
> 
> I see that you have already added this paragraph to what is already in
> "seen". OK.
> 
> > > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > > index 79d3e0e7d9..7ed6d3fc47 100644
> > > --- a/t/test-lib.sh
> > > +++ b/t/test-lib.sh
> > > @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () {
> > >  	then
> > >  		say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
> > >  		invert_exit_code=t
> > > -	else
> > > +	elif test "$test_failure" = 0
> > > +	then
> > >  		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
> > >  		invert_exit_code=t
> > > +	else
> > > +		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
> > >  	fi
> > >  }
> > 
> > This is outside the scope of this patch simply because it is
> > inherited from the original, but does ", exit non-zero!"  part of
> > the message really add any value?
> 
> 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 :-)

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

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.

-Peff

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

On Sat, Jul 06, 2024 at 02:18:50AM -0400, Jeff King wrote:
> 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.

Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

On Sat, Jul 06, 2024 at 01:20:36PM +0200, Rubén Justo wrote:

> > I don't think we want to integrate them, but I'd suggest that
> > SANITIZE_LEAK_LOG should be the default/only option.
> [...]
> 
> 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.

That sounds great. Thanks!

-Peff