improve SteadyTimer tests #1120
Conversation
instead of checking when the callback was actually called, check for when it was added to the callback queue. This *should* make the test more reliable.
Actually this is not quite correct... If I see this correctly there are two possible causes for the flaky tests:
IMHO there is no way to distinguish the two without changing |
|
|
||
| if (!first) | ||
| { | ||
| if (fabsf(expected_next_call_.toSec() - start.toSec()) > 0.1f) | ||
| if (fabsf(e.current_expected.toSec() - e.current_real.toSec()) > 0.1f) |
There was a problem hiding this comment.
fabsf is for floats, but toSec returns a double, use fabs(...) > 0.1 instead.
There was a problem hiding this comment.
I still like splitting up too early, too late into separate checks. Even if you don't relax the too-late threshold. It should never be called too early, so the check can be really tight.
|
@ros-pull-request-builder retest this please |
|
It's not going to build, dependencies broke devel. See #1119 for some of the fixes (it's incomplete). Feel free to push to my branch on that PR to finish it up. I ran out of time over the next few days. |
What exactly do you mean with that? It built successfully before and only failed some tests. I have addressed 2 of the 4 failing tests in #1125 already. Do you think this patch is going to fix one of the two remaining test failures (I thought that was the idea based on the description)? Update: the latest PR build has the same failing tests as the latest devel job. So this patch doesn't seem to improve the failing tests. |
|
I listed the issues I found in #1119, they are changes in roscpp, which aren't version controlled by this repo. |
|
@ros-pull-request-builder retest this please |
|
I actually rebased this patch in #1129 in the hope that it reduces the flakyness of the test. |
Instead of checking when the callback was actually called, check for when it was added to the callback queue.
(Not sure why it was done differently in the WallTimer tests in the first place)...
If I understand things correctly this should make the test more reliable and be a better solution than #1118
So if the tests fail because the callbacks in the queue are delayed, this should fix it (or at least be better). If the source of the sporadic test failures is that the thread actually sleeps too long, this doesn't change anything and thresholds might still need to be loosened up a bit like in #1118 or #1119