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

Add timeouts to ros1 tests #132

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Conversation

Carter12s
Copy link
Collaborator

@Carter12s Carter12s commented Sep 30, 2023

Closes: #122

Also nudged some other timeouts up. Just trying to keep CI jobs from hanging for 20 minutes.

Doesn't actually solve root problem likely, but makes failure quicker / more obvious (hopefully with better logs too).

…d added more timeout checking to a lot of places so CI jobs don't hange for 20 minutes
@Carter12s Carter12s requested a review from ssnover September 30, 2023 17:43
@Carter12s Carter12s self-assigned this Sep 30, 2023
@Carter12s
Copy link
Collaborator Author

Note: I suspect after these timeouts are added these new ROS1 tests are still flaky and we should probably add "may fail" to them for the time being.

Copy link
Collaborator

@ssnover ssnover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for now. I think we should consider defining a roslibrust::test_with_timeout macro so that the timeout doesn't need to be added everywhere, it's kind of noisy.

@ssnover ssnover merged commit 40319e8 into master Oct 3, 2023
@ssnover ssnover deleted the improve-timeout-behavior-ros1 branch October 3, 2023 01:07
@Carter12s
Copy link
Collaborator Author

This looks good for now. I think we should consider defining a roslibrust::test_with_timeout macro so that the timeout doesn't need to be added everywhere, it's kind of noisy.

Yeah I ran into this same thing with the roslibrust client and ended up building it as a feature of the crate with timeout being configurable on the NodeHandle. Likely will want to do the same thing to the ros1 client eventually.

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

Successfully merging this pull request may close these issues.

Hung Integration Tests in Workflow
2 participants