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

From: Jeff King <peff@peff.net>
Date: Sep 12, 2023, 08:27 UTC
Message-ID: <20230912082742.GB1630538@coredump.intra.peff.net>
In-Reply-To: <64825651-00d2-d2c5-2da2-91a9917bef21@gmail.com>

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