| Message ID | 54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG | expand |
On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote: > [...] > Let's add the missing conditions in the if-elses chain to make it work > as expected. Thanks, I think this explains the situation better than the original. > + 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 OK, so you did add in the "else" here. :) I am obviously fine with that, and the patch overall looks good to me. -Peff
On 23-sep-2023 02:24:15, Jeff King wrote: > On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote: > > > [...] > > Let's add the missing conditions in the if-elses chain to make it work > > as expected. > > Thanks, I think this explains the situation better than the original. > > > + 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 > > OK, so you did add in the "else" here. :) Yes, easier to explain XD I had last minute doubts about the missing "invert_exit_code=", but decided not to include it. This way, "--invert-exit-code" works as expected with "failing" tests. Maybe we can make "--invert-exit-code" work in the other cases as well. But let's let the dust settle, before scratching that itch :) > I am obviously fine with that, > and the patch overall looks good to me. Thank you for keeping an eye on this. > > -Peff
diff --git a/t/test-lib.sh b/t/test-lib.sh index 5ea5d1d62a..945ede50ac 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1264,9 +1264,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-elses chain we have in "check_test_results_san_file_", we consider three variables: $passes_sanitize_leak, $sanitize_leak_check and lastly, 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 make 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 = 0" is always true at that point. But it is not: $ 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, exiting with a non-zero status! # Simulated failures as TODO & now exiting with 0 due to --invert-exit-code We need to use "invert_exit_code=t" only when "$test_failure = 0". Probably we have missed this because GIT_TEST_SANITIZE_LEAK_LOG is commonly used in combination with GIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above, for which we have, as already said, different considerations depending on the value of $test_failure. However GIT_TEST_SANITIZE_LEAK_LOG has value used on its own, as it was documented in faececa53f (test-lib: have the "check" mode for SANITIZE=leak consider leak logs, 2022-07-28). Let's add the missing conditions in the if-elses chain to make it work as expected. Note that we're not resetting the value of "invert_exit_code" when $test_failure is non-zero. Doing that is only needed when GIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Rubén Justo <rjusto@gmail.com> --- Changes since v1: - dropped 1/1 as it was erroneously included - reworded the message to better capture the situation and the goal - show a message also when $test_failure is non-zero It is probably worth revisiting the use of "invert_exit_code=t" in this SANITIZE_LEAK machinery. But I'll stop here. --- t/test-lib.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)