| Message ID | 7c158acadf40b44edb3cf186860a3f60818f76f0.1724656120.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.6) | expand |
Patrick Steinhardt <ps@pks.im> writes: > With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check > whether a memory leak fix caused some test suites to become leak free. > It is somewhat slow to execute though because it runs all of our test > suites with the leak sanitizer enabled. It is also pointless in most > cases, because the only test suites that need to be checked are those > which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`. > > Introduce a new value "check-failing". If set, we will only check those > tests which are not yet marked as leak free. A very welcome addition. I am already liking it while running locally. Thanks. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > t/test-lib.sh | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 54247604cbc..64bd36531c1 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1558,8 +1558,16 @@ then > passes_sanitize_leak=t > fi > > - if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" > + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || > + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" > then > + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" && > + test -n "$passes_sanitize_leak" > + then > + skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing" > + test_done > + fi > + > sanitize_leak_check=t > if test -n "$invert_exit_code" > then > @@ -1597,6 +1605,7 @@ then > export LSAN_OPTIONS > > elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || > + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" || > test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false > then > BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
Patrick Steinhardt <ps@pks.im> writes: > With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check > whether a memory leak fix caused some test suites to become leak free. > It is somewhat slow to execute though because it runs all of our test > suites with the leak sanitizer enabled. It is also pointless in most > cases, because the only test suites that need to be checked are those > which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`. What I understand from `t/README` the "check" value is used to test whether the presence or absence of `TEST_PASSES_SANITIZE_LEAK=true` matches the expectations. I think it's better to express that in the first sentence, because it sounds a bit misleading at the moment if you don't know that. > Introduce a new value "check-failing". If set, we will only check those > tests which are not yet marked as leak free. I would also mention this still has the effect that tests which *are* leak-free but do not have `TEST_PASSES_SANITIZE_LEAK=true` fail due to the use of "--invert-exit-code". Also, can you add a short paragraph about this value in "t/README"? -- Toon
On Thu, Aug 29, 2024 at 04:15:58PM +0200, Toon claes wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check > > whether a memory leak fix caused some test suites to become leak free. > > It is somewhat slow to execute though because it runs all of our test > > suites with the leak sanitizer enabled. It is also pointless in most > > cases, because the only test suites that need to be checked are those > > which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`. > > What I understand from `t/README` the "check" value is used to test > whether the presence or absence of `TEST_PASSES_SANITIZE_LEAK=true` > matches the expectations. I think it's better to express that in the > first sentence, because it sounds a bit misleading at the moment if you > don't know that. > > > Introduce a new value "check-failing". If set, we will only check those > > tests which are not yet marked as leak free. > > I would also mention this still has the effect that tests which *are* > leak-free but do not have `TEST_PASSES_SANITIZE_LEAK=true` fail due to > the use of "--invert-exit-code". I don't want to go too much into the implementation details here, but will note that we continue to behave the same as with "check", except that we skip already-leak-free tests. > Also, can you add a short paragraph about this value in "t/README"? Yup, will do. Patrick
diff --git a/t/test-lib.sh b/t/test-lib.sh index 54247604cbc..64bd36531c1 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1558,8 +1558,16 @@ then passes_sanitize_leak=t fi - if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" then + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" && + test -n "$passes_sanitize_leak" + then + skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing" + test_done + fi + sanitize_leak_check=t if test -n "$invert_exit_code" then @@ -1597,6 +1605,7 @@ then export LSAN_OPTIONS elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" || test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false then BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check whether a memory leak fix caused some test suites to become leak free. It is somewhat slow to execute though because it runs all of our test suites with the leak sanitizer enabled. It is also pointless in most cases, because the only test suites that need to be checked are those which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`. Introduce a new value "check-failing". If set, we will only check those tests which are not yet marked as leak free. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/test-lib.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)