On Tue, Sep 07, 2021 at 05:33:31PM +0200, Ævar Arnfjörð Bjarmason wrote: > Subject: [PATCH v4 3/3] tests: add a test mode for SANITIZE=leak, run it in CI The patch looks OK to me. There are a bunch of typos/nits in the commit message which made it a little harder to read. I don't care _that_ much, but there's one inaccuracy I wanted to point out, and the others are along for the ride. :) > While git can be compiled with SANITIZE=leak we have not run > regression tests under that mode, memory leaks have only been fixed as > one-offs without structured regression testing. Funky comma placement. Maybe: While git can be compiled with SANITIZE=leak, we have not run regression tests under that mode. Memory leaks have only been fixed as one-offs without structured regression testing. > This change add CI testing for it. We'll now build with GCC under > Linux and test t000[04]*.sh with SANITIZE=leak, and likewise with GCC > on OSX. The new jobs are called "linux-SANITIZE=leak" and > "osx-SANITIZE=leak". s/add/adds/ A matter of taste, but I find the "linux-SANITIZE=leak" a little funny to read because of the mixed-caps and punctuation. Just linux-leaks or something is descriptive enough. Pure bikeshedding, of course. > On the topi of "LSAN_OPTIONS": It would be nice to have a mode to > aggregate all failures in our various scripts, see [2] for a start at > doing that which sets "log_path" in "LSAN_OPTIONS". I've punted on > that for now, it can be added later, and that proposed patch is also > hindered by us wanting to test e.g. test-tool leaks (and by proxy, any > API leaks they uncover), not just the "common-main.c" entry point. I think test-tool does actually use common-main.c, so we'd be covered there, too. That said, I'm perfectly fine to leave this for now (or perhaps never; if we can get the whole suite passing with leak-checking on, then aggregating the many leak reports without having test failures will become a moot point). > +# skip non-whitelisted tests when compiled with SANITIZE=leak > +if test -n "$SANITIZE_LEAK" > +then > + if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false > + then > + # We need to see it in "git env--helper" (via > + # test_bool_env) > + export TEST_PASSES_SANITIZE_LEAK > + > + if ! test_bool_env TEST_PASSES_SANITIZE_LEAK false > + then > + skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true" > + test_done > + fi > + fi > +elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false > +then > + error "GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak" > +fi I wondered if it would be helpful for this to be more forgiving. But there's not much point in setting GIT_TEST_PASSING_SANITIZE_LEAK all the time (say, in your config.mak), since it will just skip a bunch of tests. So it probably does make sense to alert the user that "oops, you did not actually build things correctly". -Peff
