| Message ID | xmqq4k8s6eri.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re* [PATCH v8 2/2] tests: add a test mode for SANITIZE=leak, run it in CI | expand |
Junio C Hamano <gitster@pobox.com> writes: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test >> mode. When running in that mode, we'll assert that we were compiled >> with SANITIZE=leak. We'll then skip all tests, except those that we've >> opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true". >> ... >> This is how tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will >> be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true: > > I've been playing with this locally, but cannot shake the nagging > feeling that GIT_TEST_PASSING_SANITIZE_LEAK must default to true. > Otherwise, it is one more thing they need to find out and set when > they do > > make SANITYZE=leak test > > because they want to be a good developer and to ensure that they did > not introduce new leaks. > > If we want to encourage folks to locally run the leak checks before > declaring their own work "done", that is. > > Those who are hunting for and cleaning up existing leaks can and > should set it to false, no? Another thing while I am at it, I have a feeling that the polarity of the TEST_PASSES_SANITIZE_LEAK declaration is the other way around. Marking the tests that do not yet pass the leak check with a special annotation will make it easier to find not-yet-clean tests for those who have too much time on their hands ;-) to find ones that are affected by the leaky tests.
On Wed, Nov 03 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test >> mode. When running in that mode, we'll assert that we were compiled >> with SANITIZE=leak. We'll then skip all tests, except those that we've >> opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true". >> ... >> This is how tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will >> be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true: > > I've been playing with this locally, but cannot shake the nagging > feeling that GIT_TEST_PASSING_SANITIZE_LEAK must default to true. > Otherwise, it is one more thing they need to find out and set when > they do > > make SANITYZE=leak test > > because they want to be a good developer and to ensure that they did > not introduce new leaks. > > If we want to encourage folks to locally run the leak checks before > declaring their own work "done", that is. > > Those who are hunting for and cleaning up existing leaks can and > should set it to false, no? I agree that that would make a lot more sense and be more useful :) That was the behavior of the patch I originally suggested for integrating this SANITIZE=leak[1], but due to feedback on it I ended up keeping the pre-image behavior of how SANITIZE=leak worked, unless there were any opt-in test modes etc. in play: https://lore.kernel.org/git/patch-1.4-a61a294132-20210714T001007Z-avarab@gmail.com/ I think at this point it's probably better to just keep it as it is... > in any case, here is a small fallout out of my adventure into this > corner. > > ----- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: t0006: date_mode can leak .strftime_fmt member > > As there is no date_mode_release() API function, and given the > set of current callers it probably is not worth adding one, let's > release the .strftime_fmt member that is obtained from strdup() > before the caller of show_date() is done with it. > > This allows us to mark t0006 as passing under the leak sanitizer. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/helper/test-date.c | 2 ++ > t/t0006-date.sh | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git c/t/helper/test-date.c w/t/helper/test-date.c > index 099eff4f0f..e15ea02626 100644 > --- c/t/helper/test-date.c > +++ w/t/helper/test-date.c > @@ -53,6 +53,8 @@ static void show_dates(const char **argv, const char *format) > > printf("%s -> %s\n", *argv, show_date(t, tz, &mode)); > } > + > + free((void *)mode.strftime_fmt); > } I'd notice that failure before, but hadn't looked into it. That was easier to fix than I thought. This fix looks good to me, except that you also need to change this at the top: diff --git a/t/helper/test-date.c b/t/helper/test-date.c index e15ea026267..9defeb57360 100644 --- a/t/helper/test-date.c +++ b/t/helper/test-date.c @@ -34,7 +34,7 @@ static void show_human_dates(const char **argv) static void show_dates(const char **argv, const char *format) { - struct date_mode mode; + struct date_mode mode = { 0 }; parse_date_format(format, &mode); for (; *argv; argv++) { I.e. this makes this specific thing pass, but in other tests we'd end up freeing a non-NULL and randomly initialized pointer unless we init it to zero. > > static void parse_dates(const char **argv) > diff --git c/t/t0006-date.sh w/t/t0006-date.sh > index 6b757d7169..5d01f57b27 100755 > --- c/t/t0006-date.sh > +++ w/t/t0006-date.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='test date parsing and printing' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > # arbitrary reference time: 2009-08-30 19:20:00 And yeah, that's all that's needed in the test file then.
diff --git c/t/helper/test-date.c w/t/helper/test-date.c index 099eff4f0f..e15ea02626 100644 --- c/t/helper/test-date.c +++ w/t/helper/test-date.c @@ -53,6 +53,8 @@ static void show_dates(const char **argv, const char *format) printf("%s -> %s\n", *argv, show_date(t, tz, &mode)); } + + free((void *)mode.strftime_fmt); } static void parse_dates(const char **argv) diff --git c/t/t0006-date.sh w/t/t0006-date.sh index 6b757d7169..5d01f57b27 100755 --- c/t/t0006-date.sh +++ w/t/t0006-date.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test date parsing and printing' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # arbitrary reference time: 2009-08-30 19:20:00