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?
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
