| Message ID | 20240924213836.GC1142403@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cf1464331b10bb6783243a31ab80a86ac6b2a485 |
| Headers | show |
| Series | LSan quality of life improvements | expand |
On Tue, Sep 24, 2024 at 05:38:36PM -0400, Jeff King wrote: > If you are trying to find and fix leaks in a large test script, it can > be overwhelming to see the leak logs for every test at once. The > previous commit let you use "--immediate" to see the logs after the > first failing test, but this isn't always the first leak. As discussed > there, we may see leaks from previous tests that didn't happen to fail. > > To catch those, let's check for any logs that appeared after each test > snippet is run, meaning that in a SANITIZE=leak build, any leak is an > immediate failure of the test snippet. > > This check is mostly free in non-leak builds (just a "test -z"), and > only a few extra processes in a leak build, so I don't think the > overhead should matter (if it does, we could probably optimize for the > common "no logs" case without even spending a process). So previously, `--immediate` didn't detect tests that should have failed because they were leaks, and now it does? Sounds like a sensible change to me, too. Patrick
On Thu, Sep 26, 2024 at 04:19:24PM +0200, Patrick Steinhardt wrote: > On Tue, Sep 24, 2024 at 05:38:36PM -0400, Jeff King wrote: > > If you are trying to find and fix leaks in a large test script, it can > > be overwhelming to see the leak logs for every test at once. The > > previous commit let you use "--immediate" to see the logs after the > > first failing test, but this isn't always the first leak. As discussed > > there, we may see leaks from previous tests that didn't happen to fail. > > > > To catch those, let's check for any logs that appeared after each test > > snippet is run, meaning that in a SANITIZE=leak build, any leak is an > > immediate failure of the test snippet. > > > > This check is mostly free in non-leak builds (just a "test -z"), and > > only a few extra processes in a leak build, so I don't think the > > overhead should matter (if it does, we could probably optimize for the > > common "no logs" case without even spending a process). > > So previously, `--immediate` didn't detect tests that should have failed > because they were leaks, and now it does? Sounds like a sensible change > to me, too. Yes, though just to be pedantic, they are marked as failures even without --immediate. It is just that doing so is a lot more useful with --immediate, since otherwise we'd find the leaks at the end (but even that it may still be useful to point to a particular test). So it really is "if you are in a SANITIZE=leak build, generating a leak log fails the test even if it would otherwise pass". -Peff
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index fde9bf54fc..78e054ab50 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -926,7 +926,8 @@ test_expect_success () { test_body_or_stdin test_body "$2" test -n "$test_skip_test_preamble" || say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $test_body" - if test_run_ "$test_body" + if test_run_ "$test_body" && + check_test_results_san_file_empty_ then test_ok_ "$1" else diff --git a/t/test-lib.sh b/t/test-lib.sh index d624ee186c..b1a8ee5c00 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1215,12 +1215,13 @@ test_atexit_handler () { teardown_malloc_check } +check_test_results_san_file_empty_ () { + test -z "$TEST_RESULTS_SAN_FILE" || + test "$(nr_san_dir_leaks_)" = 0 +} + check_test_results_san_file_ () { - if test -z "$TEST_RESULTS_SAN_FILE" - then - return - fi && - if test "$(nr_san_dir_leaks_)" = 0 + if check_test_results_san_file_empty_ then return fi &&
If you are trying to find and fix leaks in a large test script, it can be overwhelming to see the leak logs for every test at once. The previous commit let you use "--immediate" to see the logs after the first failing test, but this isn't always the first leak. As discussed there, we may see leaks from previous tests that didn't happen to fail. To catch those, let's check for any logs that appeared after each test snippet is run, meaning that in a SANITIZE=leak build, any leak is an immediate failure of the test snippet. This check is mostly free in non-leak builds (just a "test -z"), and only a few extra processes in a leak build, so I don't think the overhead should matter (if it does, we could probably optimize for the common "no logs" case without even spending a process). Signed-off-by: Jeff King <peff@peff.net> --- t/test-lib-functions.sh | 3 ++- t/test-lib.sh | 11 ++++++----- 2 files changed, 8 insertions(+), 6 deletions(-)