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.
