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

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