| Message ID | f4ae6e2a-218a-419c-b6c4-59a08be247a0@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 47c6d4dad22a751068a4975f1c4177cc6c0c41d2 |
| 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.
On Mon, Jul 01, 2024 at 01:19:34PM -0700, Junio C Hamano wrote: > 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. I see that you have already added this paragraph to what is already in "seen". OK. > > 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? Explicitly indicating that the error is being forced due to "GIT_TEST_SANITIZE_LEAK_LOG=true", for a test that doesn't fail when run normally or even when run with just "GIT_TEST_PASSING_SANITIZE_LEAK=yes", could save us some confusion. So, I dunno. Anyway, I agree that this can be addressed later. Thanks.
On Wed, Jul 03, 2024 at 11:35:48PM +0200, Rubén Justo wrote: > On Mon, Jul 01, 2024 at 01:19:34PM -0700, Junio C Hamano wrote: > > > 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. > > I see that you have already added this paragraph to what is already in > "seen". OK. > > > > 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? > > Explicitly indicating that the error is being forced due to > "GIT_TEST_SANITIZE_LEAK_LOG=true", for a test that doesn't fail when run > normally or even when run with just > "GIT_TEST_PASSING_SANITIZE_LEAK=yes", could save us some confusion. > > So, I dunno. > > Anyway, I agree that this can be addressed later. > > Thanks. Maybe what we should do is integrate "GIT_TEST_SANITIZE_LEAK_LOG" into "GIT_TEST_PASSING_SANITIZE_LEAK" because I'm not sure what value we get by keeping them separate (test performance?). But that's another topic, even further out of scope of this patch :-)
On Wed, Jul 03, 2024 at 11:44:33PM +0200, Rubén Justo wrote: > > Explicitly indicating that the error is being forced due to > > "GIT_TEST_SANITIZE_LEAK_LOG=true", for a test that doesn't fail when run > > normally or even when run with just > > "GIT_TEST_PASSING_SANITIZE_LEAK=yes", could save us some confusion. > > > > So, I dunno. > > > > Anyway, I agree that this can be addressed later. > > > > Thanks. > > Maybe what we should do is integrate "GIT_TEST_SANITIZE_LEAK_LOG" into > "GIT_TEST_PASSING_SANITIZE_LEAK" because I'm not sure what value we get > by keeping them separate (test performance?). But that's another topic, > even further out of scope of this patch :-) I don't think we want to integrate them, but I'd suggest that SANITIZE_LEAK_LOG should be the default/only option. Without it, you are potentially missing leaks in programs whose failing exit codes do not trigger a test failure. So there is no point in running PASSING_SANITIZE_LEAK=check without also checking the logs. But it is still useful to set SANITIZE_LEAK_LOG just for normal runs to look for leaks. I don't know of any reason we couldn't always check the logs (for a leak-checking build), and I didn't see anything in the history. I think it was written that way only because there is otherwise no affirmative action by the user to say "and btw, look for leaks" (and if we are not looking for leaks, there might not be any logs!). But really, if you have done a leak-checking build, then every run of the tests is looking for leaks, whether you check the logs or not. So we should able to just check that $SANITIZE_LEAK is set. And then there would be one less thing for people checking for leaks to remember to set. -Peff
On Sat, Jul 06, 2024 at 02:18:50AM -0400, Jeff King wrote: > On Wed, Jul 03, 2024 at 11:44:33PM +0200, Rubén Justo wrote: > > > > Explicitly indicating that the error is being forced due to > > > "GIT_TEST_SANITIZE_LEAK_LOG=true", for a test that doesn't fail when run > > > normally or even when run with just > > > "GIT_TEST_PASSING_SANITIZE_LEAK=yes", could save us some confusion. > > > > > > So, I dunno. > > > > > > Anyway, I agree that this can be addressed later. > > > > > > Thanks. > > > > Maybe what we should do is integrate "GIT_TEST_SANITIZE_LEAK_LOG" into > > "GIT_TEST_PASSING_SANITIZE_LEAK" because I'm not sure what value we get > > by keeping them separate (test performance?). But that's another topic, > > even further out of scope of this patch :-) > > I don't think we want to integrate them, but I'd suggest that > SANITIZE_LEAK_LOG should be the default/only option. > > Without it, you are potentially missing leaks in programs whose failing > exit codes do not trigger a test failure. So there is no point in > running PASSING_SANITIZE_LEAK=check without also checking the logs. But > it is still useful to set SANITIZE_LEAK_LOG just for normal runs to look > for leaks. > > I don't know of any reason we couldn't always check the logs (for a > leak-checking build), and I didn't see anything in the history. I think > it was written that way only because there is otherwise no affirmative > action by the user to say "and btw, look for leaks" (and if we are not > looking for leaks, there might not be any logs!). > > But really, if you have done a leak-checking build, then every run of > the tests is looking for leaks, whether you check the logs or not. So we > should able to just check that $SANITIZE_LEAK is set. > And then there would be one less thing for people checking for leaks > to remember to set. I completely agree. Let's wait for the dust to settle after the fix in this series, and then I'll address the change as you described. Thanks.
On Sat, Jul 06, 2024 at 01:20:36PM +0200, Rubén Justo wrote: > > I don't think we want to integrate them, but I'd suggest that > > SANITIZE_LEAK_LOG should be the default/only option. > [...] > > I completely agree. > > Let's wait for the dust to settle after the fix in this series, and then > I'll address the change as you described. That sounds great. Thanks! -Peff
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(-)