Should multi_threaded_executor_test have the upper bound check?

[cho3]

Bug report

The multithreaded executor test is kind of flaky.

Required Info:

• Operating System: Ubuntu 16.04
• Installation type: Source
• Version or commit hash:
• DDS implementation: fastRTPS
• Client library (if applicable): rclcpp

Steps to reproduce issue

Below vaguely reproduces CI conditions

# terminal 1
colcon build --packages-up-to rclcpp
while ./build/rclcpp/test_multi_threaded_executor ; do :;  done

# terminal 2, where 12 = number of cores
stress -m 12 -c 12

Expected behavior

Test should be able to run without failing

Actual behavior

Test fails

Additional information

The test failure is generally because the upper bound is not satisfied. I think the upper bound condition is outside the scope of the test, which is intended to ensure that the timer is not double taken (i.e. small period). On a stressed system (such as CI), we can't guarantee how much CPU time the test will receive, making the upper bound hard to satisfy.

I think if we remove the upper bound condition on the test, we won't be compromising the integrity of the test, and we'll make it less flaky on CI.

[clalancette]

I apologize for the long delay here. Looking at it, I agree with you that we shouldn't need the upper bound here; would you mind submitting a pull request to remove it? Thanks.

[clalancette]

We don't actually have the upper bound anymore, so I'm closing this out.