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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 16, 2021, 14:46 UTC
Message-ID: <877dhqxqbt.fsf@evledraar.gmail.com>
In-Reply-To: <YPCjTpumyh1P/DQj@coredump.intra.peff.net>

On Thu, Jul 15 2021, Jeff King wrote:

Show 58 quoted lines

> On Wed, Jul 14, 2021 at 07:23:51PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.

Show 7 quoted lines

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