Re: UNLEAK(), leak checking in the default tests etc.

[Jeff King <peff@peff.net>]

On Thu, Jun 10, 2021 at 05:32:41PM +0200, Andrzej Hunt wrote:

> > I do think it would be worth splitting out ASan from leak-checking. The
> > whole suite should run clean with regular ASan already, and we'd want to
> > find regressions there even in the tests that aren't leak-clean. I do
> > periodic ASan runs already; the main argument against doing it for every
> > CI run is just that's a lot more CPU. But maybe not enough to be
> > prohibitive? It's probably still way cheaper than running the test suite
> > on Windows.
> 
> I've been running tests with ASAN in the Github Actions environment, and a
> single run takes just over 30 minutes [1] - which I believe is similar to
> the normal test jobs (they do run the test suite twice in that time I
> think).
> 
> I've been doing the same with UBSAN, and that's even faster at 15-20 minutes
> [2]. However I get the impression that ASAN issues are both more common (at
> least on seen), and more impactful - so I would argue that ASAN should be
> prioritised if there's spare capacity. (I have no idea if ASAN+UBSAN can be
> combined, but I suspect that doing so would make the tests slower?)

I routinely do SANITIZE=address,undefined since they are both useful
(and we do not trigger either in the current test suite). I never
measured the time of their combined use versus just one, but surely it's
faster the two-at-once approach is faster than running the test suite
twice.

> I'm also running LSAN tests in CI to try and catch regressions, but I've
> only enabled a handful of tests so far. My much simpler approach was to
> specify the range of tests to run as 0-X, and as we make progress on fixing
> leaks, X will slowly approach 9999 (currently we're at something like X~=5,
> although I'm not too far off sending out some patches to boost that to 99).
> The skip-tests approach seems much more useful!

Depending how fine-grained you get with skip-tests, it can create a
hassle as individual tests are removed or reordered (and now somebody
has to maintain the skip list). Doing it with whole scripts (whether
saying "these ones are OK" or "these ones are known bad") seems like
less maintenance overall. The results aren't as fine-grained, but for
something that is meant to be a transitional step, I'm not sure it's
worth the trouble to get more specific.

-Peff