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

On Thu, Jul 15 2021, Jeff King wrote:

On Wed, Jul 14, 2021 at 07:23:51PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

While git can be compiled with SANITIZE=leak there has been no
corresponding GIT_TEST_* mode for it, i.e. memory leaks have been
fixed as one-offs without structured regression testing.

This opening puzzled me. I'm not sure I understand why we need a special
GIT_TEST_* mode for it.  If you do "make SANITIZE=leak test", then your
binaries will leak-check while running the tests.

I.e., there is nothing that test-lib.sh itself needs to do differently
to enable it.

What we _do_ need is some mechanism of annotating to tests to say "this
is known to leak", so that we can skip them for normal integration runs.

And that is part of what's going on in this patch, but I'm not sure it
is the simplest way to do it. The first question is: how do we want to
annotate the tests. By marking individual scripts or tests in the
test-files themselves? Or by using a separate list of "these scripts or
tests are known to pass"?

IMHO the latter is preferable. It keeps the annotations out of the way
of normal work (they are a temporary thing until we eventually pass the
whole suite leak free, but I expect they'll be with us for a while). The
downside is that the annotations may get out of sync with test numbers.
But if we are primarily annotating whole scripts (and not individual
tests), then that is generally pretty stable.

And with that in mind, can we just use an existing mechanism for picking
which tests to run, and drive it externally from the CI job?

We already have GIT_SKIP_TESTS and --run. Those are perhaps a bit
awkward for feeding huge lists to, and there is no environment
equivalent for --run (so you can't trigger it easily from "make test").
But what if we could do something like:

  GIT_TEST_RUN_FROM=t/leak-free make SANITIZE=leak test

and then t/leak-free contained the usual patterns like:

  t000*
  t1234.5

and so on. That requires two new features in test-lib.sh:

  - making a GIT_TEST_RUN variable that is the opposite of GIT_TEST_SKIP
    (instead of just the command-line --run).

  - adding GIT_TEST_{RUN,SKIP}_FROM variables to read the values from a
    file rather than the environment (I suppose the caller could just
    stuff the contents into the variable, but I expect that test-lib.sh
    may want to pare down the entries that do not even apply to the
    current script for the sake of efficiency in checking each test).

That infrastructure would then be applicable to other cases, too. Or
even just useful for using another list (or no list at all) when you
are looking at whether other tests are leak-free or not.

I've included a mechanism for whitelisting specific globs, the idea was
not to have that be too detailed, but we'd e.g. get to the point of t00*
or whatever passing.

Anything that's a lot more granular than that is doing to suck,
e.g. exposing teh GIT_TEST_SKIP and --run features. of specific test
numbers, now you need to count your tests if you add one in the middle
of one of those, and more likely you won't test under the mode and just
see it in CI.

The marking at a distance I've done also has that problem in theory, but
I think in practice we'll use it carefully for globs of tests unlikely
to break.

This whole thing is much more with the GIT_TEST_SANITIZE_LEAK mode, it's
a really common case that we e.g. leak in some revision.c API user, we
should fix that, but holding up marking the rest of at test whose entire
tests otherwise pass is bad, it means you can't do any testing of a
given API or subsystem without getting the entire file to pass.

Whereas while we're fixing very common leaks in the codabase it's likely
that any given test file will have a few such tests.

It also means everything works by default, you get an appropriate notice
from prove(1), and even if you run one test manually it'll skip, but
emit a message saying you can set the env var to force its run.

quoted

This change add such a mode, we now have new
linux-{clang,gcc}-sanitize-leak CI targets, these targets run the same
tests as linux-{clang,gcc}, except that almost all of them are
skipped.

I'm not clear on what we expect to get out of running it with both clang
and gcc. They should be producing identical results.

Indeed, addressed elsewhere, i.e. it's just a thinko of mine.