Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

From: Rubén Justo <rjusto@gmail.com>
Date: Jul 3, 2024, 21:35 UTC
Message-ID: <bda01080-1231-476a-9770-88b62a75ffe2@gmail.com>
In-Reply-To: <xmqqv81ovp9l.fsf@gitster.g>

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

Show 7 quoted lines

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

Show 21 quoted lines

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