On 12-sep-2023 04:27:42, Jeff King wrote:

On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:

quoted

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.