llvm dev - Feb 2020 - phab unit tests + libcxx tests w/concurrency

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 "thread_mutex_class::try_lock.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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200225/b3982a0c/attachment.html>

We do have a // FLAKY_TEST 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 try_lock 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 “thread_mutex_class::try_lock.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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200226/2be29251/attachment.html>

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 // FLAKY_TEST 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 try_lock 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 “thread_mutex_class::try_lock.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>