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

["Rubén Justo" <rjusto@gmail.com>]

On 12-sep-2023 04:27:42, Jeff King wrote:
> 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:

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.