Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improves lunar-devel jenkins test stability #1135

Merged
merged 4 commits into from
Aug 14, 2017

Conversation

kmactavish
Copy link
Contributor

@kmactavish kmactavish commented Aug 12, 2017

This PR is to establish a Jenkins test-result baseline for lunar-devel. #1133 is failing stochastically, and this PR is being used to establish a baseline of which stochastic failures are expected on devel (if any).
Update: Also fixes one flaky test:

  • extend random_play.py timeout

@kmactavish
Copy link
Contributor Author

kmactavish commented Aug 12, 2017

Triggered jobs 324-331. Update: only one failed, new commit to extend that timeout.

@kmactavish kmactavish force-pushed the lunar-devel-jenkins-baseline branch from f581da6 to 50785ed Compare August 12, 2017 15:24
@kmactavish
Copy link
Contributor Author

kmactavish commented Aug 12, 2017

Triggered jobs 332-339. Update: 338 failed, but I can't find any "RESULT: FAIL" in the output...

@kmactavish kmactavish changed the title Baseline lunar-devel jenkins job Improves lunar-devel jenkins test stability Aug 12, 2017
@mikaelarguedas
Copy link
Member

@kmactavish If it's of any help: It looks like the failing test is a nosetest added( using catkin_add_nosetests) and not a rostest.
Nosetests output results as "OK" or FAILED (failures=1) or FAILED (errors=1). The "SUMMARY" is part of rostest I believe, and thus not printed by "pure" nose of gtest tests.

@kmactavish
Copy link
Contributor Author

Build 345 failed (not this branch). It was in raceConditionCallback, which is flawed.
Explanation (main is the main thread, add is the spawned thread):

  • main gets to line 471 (queue is cleared)
  • add sees sync is true, does its thing (setting sync to false, adding the callback to the queue)
  • main sets sync back to true! and calls the callback (setting one to false)
  • add sees sync is true, does its thing (setting one to true)
  • main wakes up, and having called the callback, checks to make sure one is false. It's not!
    So, ironically, there was a secondary, unintentional race condition in the race condition check 😱.

Fixed by only setting the sync flag in one thread. It means adding another check to see if the queue is empty in the second thread.

@kmactavish
Copy link
Contributor Author

@mikaelarguedas Thanks! That helped.

@kmactavish
Copy link
Contributor Author

Build 355 (not this branch) also failed due to the compression ratio test thresholds, but in the other direction, relaxed them more.

@kmactavish
Copy link
Contributor Author

@esteve, I had to change your raceConditionCallback test (see above). You might want to have a look and make sure it still tests for the right things after my modifications.

@dirk-thomas
Copy link
Member

Thank you for improving the stability of the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants