Skip to content

Remove timeout on tests#862

Closed
zmichaels11 wants to merge 1 commit intoros2:dashingfrom
aws-ros-dev:remove-timeout-on-tests
Closed

Remove timeout on tests#862
zmichaels11 wants to merge 1 commit intoros2:dashingfrom
aws-ros-dev:remove-timeout-on-tests

Conversation

@zmichaels11
Copy link
Copy Markdown
Contributor

  • timeout was not required since the request will fail if the node is not found

Signed-off-by: Zachary Michaels zmichaels11@gmail.com

* timeout was not required since the request will fail if the node is not found

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@dirk-thomas
Copy link
Copy Markdown
Member

Removing this timeout here doesn't make any sense imo. If the call doesn't finish within 5s it is extremely unlikely that it will finish in the remaining time of the (usually using a CTest timeout of 30 or 60 seconds). Though if it fails to complete within 5s it is safe to fail the test.

The node not being found is only one of the possible scenarios. It can absolutely happen that the node was found and the request sent but no response is ever received. And in that case the explicit timeout makes sure the test fails quickly.

@mabelzhang mabelzhang added the enhancement New feature or request label Sep 26, 2019
@mjcarroll
Copy link
Copy Markdown
Member

@hidmic believes that there may be a race condition specifically effecting FastRTPS, and has a proposed solution here: #876

@zmichaels11
Copy link
Copy Markdown
Contributor Author

The intention of this PR was to improve stability with this test. Instead it looks like there's another underlying issue.

There's an open issue on the flakiness of this test here: #863

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants