threads / patch / 60217

patch, 2 partsfix GIT_TEST_SANITIZE_LEAK_LOG=true

Subject: [PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true

## tl;dr

16 messages between Sep 9, 2023 and Sep 23, 2023. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Rubén Justo· Sep 9, 2023, 23:03 UTC · lore

While using "make test" I noticed that invert_exit_code used by GIT_TEST_SANITIZE_LEAK_LOG=true produces unexpected results.

   $ git checkout v2.40.1 
   $ make test SANITIZE=leak T=t3200-branch.sh # fails
   $ make test SANITIZE=leak GIT_TEST_SANITIZE_LEAK_LOG=true T=t3200-branch.sh # succeeds
Rubén Justo (2):
  test-lib: prevent misuses of --invert-exit-code
  test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
 t/test-lib.sh | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
-- 
2.40.1
Rubén Justo· Sep 9, 2023, 23:08 UTC · re: Rubén Justo · lore

[PATCH 1/2] test-lib: prevent misuses of --invert-exit-code

GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true use internnlly the --invert-exit-code machinery. Therefore if the user wants to use --invert-exit-code in combination with them, the result will be confusing.

For the same reason, we are already using BAIL_OUT if the user tries to combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.

Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-lib.sh | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
Show changes to t/test-lib.sh +14 −4
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 293caf0f20..88a34fba67 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1557,15 +1557,25 @@ then
 			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
 			invert_exit_code=t
 		fi
-	elif test -z "$passes_sanitize_leak" &&
-	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+	elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
 	then
-		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
-		test_done
+		if test -n "$invert_exit_code"
+		then
+			BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+		elif test -z "$passes_sanitize_leak"
+		then
+			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+			test_done
+		fi
 	fi
 
 	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
 	then
+		if test -n "$invert_exit_code"
+		then
+			BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
+		fi
+
 		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
 		then
 			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"

-- 
2.40.1
Eric Sunshine· Sep 10, 2023, 01:59 UTC · re: Rubén Justo · lore

Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code

On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:
> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> use internnlly the --invert-exit-code machinery.  Therefore if the user
s/internnlly/internally/
Show 10 quoted lines
> wants to use --invert-exit-code in combination with them, the result
> will be confusing.
>
> For the same reason, we are already using BAIL_OUT if the user tries to
> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
>
> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> GIT_TEST_SANITIZE_LEAK_LOG=true.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
Rubén Justo· Sep 10, 2023, 22:58 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code

On 10/9/23 3:59, Eric Sunshine wrote:
Show 5 quoted lines
> On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:
>> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
>> use internnlly the --invert-exit-code machinery.  Therefore if the user
> 
> s/internnlly/internally/
Thanks!
Show 11 quoted lines
> 
>> wants to use --invert-exit-code in combination with them, the result
>> will be confusing.
>>
>> For the same reason, we are already using BAIL_OUT if the user tries to
>> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
>>
>> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
>> GIT_TEST_SANITIZE_LEAK_LOG=true.
>>
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
Jeff King· Sep 12, 2023, 08:35 UTC · re: Rubén Justo · lore

Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code

On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:
Show 10 quoted lines
> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> use internnlly the --invert-exit-code machinery.  Therefore if the user
> wants to use --invert-exit-code in combination with them, the result
> will be confusing.
> 
> For the same reason, we are already using BAIL_OUT if the user tries to
> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
> 
> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> GIT_TEST_SANITIZE_LEAK_LOG=true.

OK, so we are trying to find a case where the user is triggering --invert-exit-code themselves and complaining. But in the code...

Show 19 quoted lines
> @@ -1557,15 +1557,25 @@ then
>  			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
>  			invert_exit_code=t
>  		fi
> -	elif test -z "$passes_sanitize_leak" &&
> -	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> +	elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  	then
> -		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> -		test_done
> +		if test -n "$invert_exit_code"
> +		then
> +			BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> +		elif test -z "$passes_sanitize_leak"
> +		then
> +			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> +			test_done
> +		fi
>  	fi

You can see at the top of the context that we will set invert_exit_code=t ourselves, which will then complain here:

Show 10 quoted lines
>  	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
>  	then
> +		if test -n "$invert_exit_code"
> +		then
> +			BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
> +		fi
> +
>  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
>  		then
>  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"

That varible-set in the earlier context is from running in "check" mode. So:

  make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true

will now always fail. But this is the main way you'd want to run it (enabling the leak log catches more stuff, and the log-check function you touch in patch 2 already covers check mode).

So I think you'd have to hoist your check above the if/else for setting up PASSING_SANITIZE_LEAK modes.

-Peff
Rubén Justo· Sep 15, 2023, 00:10 UTC · re: Jeff King · lore

Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code

On 12-sep-2023 04:35:28, Jeff King wrote:
Show 61 quoted lines
> On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:
> 
> > GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> > use internnlly the --invert-exit-code machinery.  Therefore if the user
> > wants to use --invert-exit-code in combination with them, the result
> > will be confusing.
> > 
> > For the same reason, we are already using BAIL_OUT if the user tries to
> > combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
> > 
> > Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> > GIT_TEST_SANITIZE_LEAK_LOG=true.
> 
> OK, so we are trying to find a case where the user is triggering
> --invert-exit-code themselves and complaining. But in the code...
> 
> > @@ -1557,15 +1557,25 @@ then
> >  			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
> >  			invert_exit_code=t
> >  		fi
> > -	elif test -z "$passes_sanitize_leak" &&
> > -	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> > +	elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> >  	then
> > -		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > -		test_done
> > +		if test -n "$invert_exit_code"
> > +		then
> > +			BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > +		elif test -z "$passes_sanitize_leak"
> > +		then
> > +			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > +			test_done
> > +		fi
> >  	fi
> 
> You can see at the top of the context that we will set
> invert_exit_code=t ourselves, which will then complain here:
> 
> >  	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> >  	then
> > +		if test -n "$invert_exit_code"
> > +		then
> > +			BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
> > +		fi
> > +
> >  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
> >  		then
> >  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
> 
> That varible-set in the earlier context is from running in "check" mode.
> So:
> 
>   make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true
> 
> will now always fail. But this is the main way you'd want to run it
> (enabling the leak log catches more stuff, and the log-check function
> you touch in patch 2 already covers check mode).
> 
> So I think you'd have to hoist your check above the if/else for setting
> up PASSING_SANITIZE_LEAK modes.
Arrg, sorry.  You're right.
This was part of the series by mistake.  Please, discard it.
In my tree, I have a previous commit with:
Show changes to t/test-lib.sh +0 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 87cfea9e9a..46b8a76e9c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1556,7 +1556,6 @@ then
                if test -z "$passes_sanitize_leak"
                then
                        say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
-                       invert_exit_code=t
                fi
        elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
        then

that is part of an unsent attempt to make:

  $ make SANITIZE=leak T=t7510-signed-commit.sh GIT_TEST_PASSING_SANITIZE_LEAK=check test
 
not to suggest, when GPG is missing, that t7510-signed-commit.sh is a
candidate to be marked as leak-free.  Which is distracting to me.

However I was not satisfied with the solution and discarded it.  But
unfortunately not entirely.  Sorry. 

> 
> -Peff
Rubén Justo· Sep 9, 2023, 23:09 UTC · re: Rubén Justo · lore

[PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

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

  $ git checkout v2.40.1
  $ make SANITIZE=leak
  $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
  ...
  With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
  # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
Let's use invert_exit_code only if needed.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/test-lib.sh +2 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 88a34fba67..87cfea9e9a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1263,7 +1263,8 @@ 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
 	fi

-- 
2.40.1
Jeff King· Sep 12, 2023, 08:27 UTC · re: Rubén Justo · lore

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:
Show 11 quoted lines
> GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
> test return zero unintentionally:
> 
>   $ git checkout v2.40.1
>   $ make SANITIZE=leak
>   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
>   ...
>   With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
>   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
> 
> Let's use invert_exit_code only if needed.

Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe not), but then we _also_ saw entries in the leak-log. So the inversion cancels out, and we accidentally say everything is OK, which is wrong.

I'm not quite sure of your fix, though. In the if-else chain you're touching, we know going in that we found a leak in the log. And then we cover these 5 cases:

  1. if the test is marked as passing-leak
    a. if we saw no test failures, invert (and mention the leaking log)
    b. otherwise, do not invert (and mention the log)
  2. else if we are in "check" mode
    a. if we saw no test failures, do not invert (we do have a leak,
       which is equivalent to a test failure). Mention the log.
    b. otherwise, invert (to switch back to "success", since we are
       looking for leaks), but still mention the log.
  3. invert to trigger failure (and mention the log)

And the problem is in (3). You switch it to trigger only if we have no failures (fixing the inversion). But should we have the same a/b split for this case? I.e.:

  3a. if we saw no test failures, invert to cause a failure
  3b. we saw other failures; do not invert, but _do_ mention that the
      log found extra leaks

In 3b we are explaining to the user what happened. Though maybe it is not super important, because I think we'd have dumped the log contents anyway?

Other than that, I think the patch is correct. I wondered when we ran this "check_test_results_san_file_" code, but it is only at the end of the script. So we are OK to make a definitive call on the zero/non-zero count of failed tests.

-Peff
Rubén Justo· Sep 15, 2023, 00:28 UTC · re: Jeff King · lore

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

On 12-sep-2023 04:27:42, Jeff King wrote:
Show 43 quoted lines
> On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:
> 
> > GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
> > test return zero unintentionally:
> > 
> >   $ git checkout v2.40.1
> >   $ make SANITIZE=leak
> >   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
> >   ...
> >   With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
> >   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
> > 
> > Let's use invert_exit_code only if needed.
> 
> Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe
> not), but then we _also_ saw entries in the leak-log. So the inversion
> cancels out, and we accidentally say everything is OK, which is wrong.
> 
> I'm not quite sure of your fix, though. In the if-else chain you're
> touching, we know going in that we found a leak in the log. And then we
> cover these 5 cases:
> 
>   1. if the test is marked as passing-leak
>     a. if we saw no test failures, invert (and mention the leaking log)
>     b. otherwise, do not invert (and mention the log)
>   2. else if we are in "check" mode
>     a. if we saw no test failures, do not invert (we do have a leak,
>        which is equivalent to a test failure). Mention the log.
>     b. otherwise, invert (to switch back to "success", since we are
>        looking for leaks), but still mention the log.
>   3. invert to trigger failure (and mention the log)
> 
> And the problem is in (3). You switch it to trigger only if we have no
> failures (fixing the inversion). But should we have the same a/b split
> for this case? I.e.:
> 
>   3a. if we saw no test failures, invert to cause a failure
>   3b. we saw other failures; do not invert, but _do_ mention that the
>       log found extra leaks
> 
> In 3b we are explaining to the user what happened. Though maybe it is
> not super important, because I think we'd have dumped the log contents
> anyway?

I think so too. At that point we've already dumped the contents of the $TEST_RESULTS_SAN_FILE file.

IMO, when $test_failure is zero (the "if" I'm touching), the message makes sense not so much to say that a leak has been found, but rather because we're forcing the non-zero exit.

But when $test_failure is not zero, after we've already dumped the log, maybe this is somewhat redundant:

Show changes to t/test-lib.sh +2 −0
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 87cfea9e9a..b160ae3f7a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
        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
 }

However, if you or anyone else thinks it adds value, I have no objection
to re-roll with it.

> Other than that, I think the patch is correct. I wondered when we ran
> this "check_test_results_san_file_" code, but it is only at the end of
> the script. So we are OK to make a definitive call on the zero/non-zero
> count of failed tests.
> 
> -Peff

Thank you for taking the time to review these series.
Jeff King· Sep 15, 2023, 11:29 UTC · re: Rubén Justo · lore

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

On Fri, Sep 15, 2023 at 02:28:15AM +0200, Rubén Justo wrote:
Show 37 quoted lines
> > And the problem is in (3). You switch it to trigger only if we have no
> > failures (fixing the inversion). But should we have the same a/b split
> > for this case? I.e.:
> > 
> >   3a. if we saw no test failures, invert to cause a failure
> >   3b. we saw other failures; do not invert, but _do_ mention that the
> >       log found extra leaks
> > 
> > In 3b we are explaining to the user what happened. Though maybe it is
> > not super important, because I think we'd have dumped the log contents
> > anyway?
> 
> I think so too.  At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
> 
> IMO, when $test_failure is zero (the "if" I'm touching), the message
> makes sense not so much to say that a leak has been found, but rather
> because we're forcing the non-zero exit.
> 
> But when $test_failure is not zero, after we've already dumped the
> log, maybe this is somewhat redundant:
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 87cfea9e9a..b160ae3f7a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
>         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
>  }
> 
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.

I'm on the fence. It is probably not a big deal, and my biggest issue is just that I had to walk through the explanation in my previous mail to convince myself the change was not missing an important case.

But having done so, the main value in re-rolling would be preventing somebody else from reading the code and having the same question. But this discussion in the archive is probably sufficient.

-Peff
Junio C Hamano· Sep 15, 2023, 17:51 UTC · re: Jeff King · lore

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

Jeff King <peff@peff.net> writes:
> But having done so, the main value in re-rolling would be preventing
> somebody else from reading the code and having the same question.

Indeed. It would be valuable to help future developers not to waste time on wondering what we already know they may do so on.

Jeff King· Sep 16, 2023, 05:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

On Fri, Sep 15, 2023 at 10:51:17AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > But having done so, the main value in re-rolling would be preventing
> > somebody else from reading the code and having the same question.
> 
> Indeed.  It would be valuable to help future developers not to waste
> time on wondering what we already know they may do so on.

I guess I was thinking "maybe that is not a good reason to change what the code does" when I wrote my other email. But it is probably a good reason to make a note in the commit message. :)

-Peff
Junio C Hamano· Sep 15, 2023, 17:49 UTC · re: Rubén Justo · lore

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

Rubén Justo <rjusto@gmail.com> writes:
Show 17 quoted lines
>> And the problem is in (3). You switch it to trigger only if we have no
>> failures (fixing the inversion). But should we have the same a/b split
>> for this case? I.e.:
>> 
>>   3a. if we saw no test failures, invert to cause a failure
>>   3b. we saw other failures; do not invert, but _do_ mention that the
>>       log found extra leaks
>> 
>> In 3b we are explaining to the user what happened. Though maybe it is
>> not super important, because I think we'd have dumped the log contents
>> anyway?
>
> I think so too.  At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
> ...
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.

I do not know offhand if we need the code update to implement what Peff called "maybe it is not super important", but if we decide not to, at least it would help future developers to document the fact that we were aware of the issue when the code was developed, and why we decided not to address it (in other words, describe why we decided it is not super important).

Thanks both for polishing the series and making it better.
Rubén Justo· Sep 22, 2023, 20:38 UTC · re: Rubén Justo · lore

[PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

In the if-elses chain we have in "check_test_results_san_file_", we consider three variables: $passes_sanitize_leak, $sanitize_leak_check and lastly, 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 make 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 = 0" is always true at that point. But it is not:

   $ git checkout v2.40.1
   $ make SANITIZE=leak
   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
   ...
   With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status!
   # Simulated failures as TODO & now exiting with 0 due to --invert-exit-code
We need to use "invert_exit_code=t" only when "$test_failure = 0".

Probably we have missed this because GIT_TEST_SANITIZE_LEAK_LOG is commonly used in combination with GIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above, for which we have, as already said, different considerations depending on the value of $test_failure.

However GIT_TEST_SANITIZE_LEAK_LOG has value used on its own, as it was documented in faececa53f (test-lib: have the "check" mode for SANITIZE=leak consider leak logs, 2022-07-28).

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

Note that we're not resetting the value of "invert_exit_code" when $test_failure is non-zero. Doing that is only needed when GIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Changes since v1:
- dropped 1/1 as it was erroneously included
- reworded the message to better capture the situation and the goal
- show a message also when $test_failure is non-zero

It is probably worth revisiting the use of "invert_exit_code=t" in this SANITIZE_LEAK machinery. But I'll stop here.

---
 t/test-lib.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Show changes to t/test-lib.sh +4 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5ea5d1d62a..945ede50ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1264,9 +1264,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.34.1
Jeff King· Sep 23, 2023, 06:24 UTC · re: Rubén Justo · lore

Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:
> [...]
> Let's add the missing conditions in the if-elses chain to make it work
> as expected.
Thanks, I think this explains the situation better than the original.
Show 7 quoted lines
> +	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

OK, so you did add in the "else" here. :) I am obviously fine with that, and the patch overall looks good to me.

-Peff
Rubén Justo· Sep 23, 2023, 08:11 UTC · re: Jeff King · lore

Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

On 23-sep-2023 02:24:15, Jeff King wrote:
Show 17 quoted lines
> On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:
> 
> > [...]
> > Let's add the missing conditions in the if-elses chain to make it work
> > as expected.
> 
> Thanks, I think this explains the situation better than the original.
> 
> > +	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
> 
> OK, so you did add in the "else" here. :)
Yes, easier to explain XD

I had last minute doubts about the missing "invert_exit_code=", but decided not to include it. This way, "--invert-exit-code" works as expected with "failing" tests.

Maybe we can make "--invert-exit-code" work in the other cases as well.
But let's let the dust settle, before scratching that itch :)
> I am obviously fine with that,
> and the patch overall looks good to me.
Thank you for keeping an eye on this.
> 
> -Peff

← back to recent threads