[llvm-dev] phab unit tests + libcxx tests w/concurrency (original) (raw)

Brian Cain via llvm-dev llvm-dev at lists.llvm.org
Thu Feb 27 19:57:19 PST 2020

I don't want to sound too picky but if the test is really "flaky" it probably doesn't belong in the test suite. But I appreciate that by design, it's dependent on system load and possibly other factors. So I guess a flaky test by any other name would still smell as sweet ;)

It would be ideal if FLAKY_TEST were omitted from the check-cxx / check-cxxabi target(s). Maybe it should be a lit feature and only passed on when a new cmake option like LIBCXX_ENABLE_EXHAUSTIVE_TESTS or LIBCXX_ENABLE_TIMING_TESTS is set? Or if you are saying the FLAKY_TEST annotation runs the test but then masks any failure then yes that probably would suffice (but in that case why run the test at all?).

On Wed, Feb 26, 2020 at 4:24 PM Eric Fiselier via llvm-dev < llvm-dev at lists.llvm.org> wrote:

We do have a // FLAKYTEST tag that is used to tell the test suite that a given test my fail for non-deterministic reasons. I'll add this annotation to more trylock tests. In the past this has resolved LIT turning up actual flaky failures.

Is this sufficient? /Eric On Tue, Feb 25, 2020 at 10:41 AM Brian Cain via llvm-dev <_ _llvm-dev at lists.llvm.org> wrote:

I think it may make sense to segregate the libcxx lit tests that expect a task to be completed in a particular threshold. Either they should move to the llvm test-suite or they should be under a feature guard that omits them from the default test target. These tests are sensitive to the load and/or capability of the target on which they’re tested. I appreciate that it’s likely impossible to write a noninteractive test for these library concurrency features that aren’t designed with some kind of thresholds.

I have an “innocuous” change that was automagically tested (pre-merge!) via phabricator -- https://reviews.llvm.org/D75085 -- but it triggered a test failure in one of the “threadmutexclass::trylock.pass.cpp” tests. It’s great and super convenient to have this test facility and I’m pretty sure I opted-in to it. I think it would be/would have been nice for it to be integrated with github PRs, but this seems functionally pretty close. Having this pre-merge check helps buoy confidence in my change – that it’s less likely to cause a buildbot to trigger. We should strive to eliminate false signals from this test suite. Either the test suite should omit check-cxx (not my preference) or the cxx tests must be descoped to reliably passing ones. Or maybe the test infrastructure could be partitioned/dedicated such that it’s very unlikely to have false failures like this. Also, thanks again to the teams who work on providing testing features like this, it’s a step in the right direction. -Brian

LLVM Developers mailing list llvm-dev at lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

LLVM Developers mailing list llvm-dev at lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-- -Brian -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200227/461c115c/attachment.html>

More information about the llvm-dev mailing list