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

From: Jeff King <peff@peff.net>
Date: Sep 7, 2021, 16:51 UTC
Message-ID: <YTeYrq1/BlFobAGk@coredump.intra.peff.net>
In-Reply-To: <patch-v4-3.3-fffbfc35c00-20210907T151855Z-avarab@gmail.com>

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.

Show 6 quoted lines

> 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).

Show 19 quoted lines

> +# 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