| Message ID | 598149bf-6541-4c9e-8c94-a108e3ee7fd7@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default | expand |
On Wed, Jul 10, 2024 at 02:51:58AM +0200, Rubén Justo wrote: > As we describe in t/README, it can happen that: > > Some tests run "git" (or "test-tool" etc.) without properly checking > the exit code, or git will invoke itself and fail to ferry the > abort() exit code to the original caller. > > Therefore, GIT_TEST_SANITIZE_LEAK_LOG must be set to true to capture all > memory leaks triggered by the tests when SANITIZE=leak. > > Set it to true by default, and stop worrying about someone checking for > leaks who isn't aware of this option and might be missing some leaks. I'm obviously in favor of this direction, but...why stop here? Do we expect somebody to set it to false? If not, then can't we just get rid of it entirely? -Peff
On Tue, Jul 09, 2024 at 09:12:06PM -0400, Jeff King wrote: > On Wed, Jul 10, 2024 at 02:51:58AM +0200, Rubén Justo wrote: > > > As we describe in t/README, it can happen that: > > > > Some tests run "git" (or "test-tool" etc.) without properly checking > > the exit code, or git will invoke itself and fail to ferry the > > abort() exit code to the original caller. > > > > Therefore, GIT_TEST_SANITIZE_LEAK_LOG must be set to true to capture all > > memory leaks triggered by the tests when SANITIZE=leak. > > > > Set it to true by default, and stop worrying about someone checking for > > leaks who isn't aware of this option and might be missing some leaks. > > I'm obviously in favor of this direction, but...why stop here? Do we > expect somebody to set it to false? If not, then can't we just get rid > of it entirely? I'd also be strongly in favor of just removing this variable altogether. I found it quite tedious to remember setting it when working on the memory leak fixes recently, and I'm not aware of any downsides. Patrick
diff --git a/ci/lib.sh b/ci/lib.sh index ff66ad356b..fe52954828 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -374,7 +374,6 @@ linux-musl) linux-leaks|linux-reftable-leaks) export SANITIZE=leak export GIT_TEST_PASSING_SANITIZE_LEAK=true - export GIT_TEST_SANITIZE_LEAK_LOG=true ;; linux-asan-ubsan) export SANITIZE=address,undefined diff --git a/t/README b/t/README index d9e0e07506..1c97bc3331 100644 --- a/t/README +++ b/t/README @@ -382,10 +382,10 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" and those tests that pass under "SANITIZE=leak". This is especially useful when testing a series that fixes various memory leaks with "git rebase -x". -GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to +GIT_TEST_SANITIZE_LEAK_LOG=<boolean> controls logging of memory leaks to "test-results/$TEST_NAME.leak/trace.*" files. The logs include a "dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to -make logs +machine-readable. +make logs +machine-readable. Defaults to "true" when SANITIZE=leak. With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs before exiting and exit on failure if the logs showed that we had a diff --git a/t/test-lib.sh b/t/test-lib.sh index 7ed6d3fc47..1dd2ea4e07 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1578,7 +1578,7 @@ then test_done fi - if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false + if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG true then if ! mkdir -p "$TEST_RESULTS_SAN_DIR" then
As we describe in t/README, it can happen that: Some tests run "git" (or "test-tool" etc.) without properly checking the exit code, or git will invoke itself and fail to ferry the abort() exit code to the original caller. Therefore, GIT_TEST_SANITIZE_LEAK_LOG must be set to true to capture all memory leaks triggered by the tests when SANITIZE=leak. Set it to true by default, and stop worrying about someone checking for leaks who isn't aware of this option and might be missing some leaks. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- ci/lib.sh | 1 - t/README | 4 ++-- t/test-lib.sh | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-)