-
Notifications
You must be signed in to change notification settings - Fork 310
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
Move test_utils module from demos repo #1955
Conversation
c5c21ff
to
2a8f8aa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1955 +/- ##
==========================================
+ Coverage 87.73% 88.24% +0.51%
==========================================
Files 122 123 +1
Lines 13052 13129 +77
Branches 1171 1183 +12
==========================================
+ Hits 11451 11586 +135
+ Misses 1169 1093 -76
- Partials 432 450 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A nice addition
namespace_api = "/" | ||
|
||
while time.time() - start < 10.0 and not all(found.values()): | ||
node_names_namespaces = node.get_node_names_and_namespaces() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't that the functionality that was causing trouble for us in the spawner when waiting for the controller manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, I can't remember. I added this some time ago in the demo tests, and it seemed to be useful back then ;) maybe the list_controllers service is now more robust and we could skip this node check in advance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Atleast in the ros2_control_demos, we haven't seen flaky tests due to this right?. We can remove it, if this is not interesting for us to have consistency or we can get back to it when we see some flakiness. Both are fine for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it didn't make any problems recently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve it as I don't have a strong opinion on this
676c61b
to
df209eb
Compare
As integration testing gets more popular I propose to release the
test_utils
from the ros2_control_demos repository.I added it to the controller_manager package to be able to use it in the test for the ros2_control_node.
I had to use
ament_cmake_pytest
instead oflaunch_testing_ament_cmake
, otherwise the coverage was not evaluated of the python files.Tested with semi-binary builds of ros-controls/ros2_control_demos#673