| Message ID | f4ae6e2a-218a-419c-b6c4-59a08be247a0@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG | expand |
On Sun, Jun 30, 2024 at 08:42:06AM +0200, Rubén Justo wrote: > This has already been sent: > https://lore.kernel.org/git/54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com/ Thanks for that link. As soon as I read the subject, I thought "Uh oh, wasn't there some tricky complexity here?". But going back to that thread explained it all. :) I think the patch you've sent here covers what was discussed there, and is the right thing to do. -Peff
Jeff King <peff@peff.net> writes: > On Sun, Jun 30, 2024 at 08:42:06AM +0200, Rubén Justo wrote: > >> This has already been sent: >> https://lore.kernel.org/git/54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com/ > > Thanks for that link. As soon as I read the subject, I thought "Uh oh, > wasn't there some tricky complexity here?". But going back to that > thread explained it all. :) > > I think the patch you've sent here covers what was discussed there, and > is the right thing to do. Thanks, both. Will queue.
Rubén Justo <rjusto@gmail.com> writes: > In the if-else's chain we have in "check_test_results_san_file_", we > consider three variables: $passes_sanitize_leak, $sanitize_leak_check > and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at > that point). Before this paragraph, we'd probably want to say what problem we are fixing. Using the verb "fix" on the subject line without saying what broken behaviour you see around GIT_TEST_SANITIZE_LEAK_LOG does not help, either. Your patch from September 2023 [*] did mention it upfront: GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the test return zero unintentionally. With that inserted in front of the proposed log message, the resulting explanation looks reasonable to me. > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 79d3e0e7d9..7ed6d3fc47 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () { > then > say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" && > invert_exit_code=t > - else > + elif test "$test_failure" = 0 > + 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 > } This is outside the scope of this patch simply because it is inherited from the original, but does ", exit non-zero!" part of the message really add any value? I am wondering if else - say "With GIT_TEST_SANITIZE_LEAK_LOG=true ..., exit non-zero!" + say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a leak." + test "$test_failure" != 0 || invert_exit_code=t fi might be what we eventually want to end up with, after the dust settles from this fix. Thanks.
diff --git a/t/test-lib.sh b/t/test-lib.sh index 79d3e0e7d9..7ed6d3fc47 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () { then say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" && invert_exit_code=t - else + elif test "$test_failure" = 0 + 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 }
In the if-else's chain we have in "check_test_results_san_file_", we consider three variables: $passes_sanitize_leak, $sanitize_leak_check and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at that point). For the first two variables we have different considerations depending on the value of $test_failure, which makes sense. However, for the third, GIT_TEST_SANITIZE_LEAK_LOG, we don't; regardless of $test_failure, we use "invert_exit_code=t" to produce a non-zero return value. That assumes "$test_failure" is always zero at that point. But it may not be: $ git checkout v2.40.1 $ make test SANITIZE=leak T=t3200-branch.sh # this fails $ make test SANITIZE=leak GIT_TEST_SANITIZE_LEAK_LOG=true T=t3200-branch.sh # this succeeds [...] With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status! # faked up failures as TODO & now exiting with 0 due to --invert-exit-code We need to use "invert_exit_code=t" only when "$test_failure" is zero. Let's add the missing conditions in the if-else's chain to make it work as expected. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Rubén Justo <rjusto@gmail.com> --- This has already been sent: https://lore.kernel.org/git/54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com/ I have simplified the message a little, but the change remains the same. Thanks. t/test-lib.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)