Re: [PATCH v4 3/3] tests: add a test mode for SANITIZE=leak, run it in CI

[Jeff King <peff@peff.net>]

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