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.

Sign up for GitHub

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

Jump to bottom

[Noetic] Rework and re-enable spawning and switching CLI tests #448

Closed
bmagyar opened this issue Apr 23, 2020 · 3 comments
Closed

[Noetic] Rework and re-enable spawning and switching CLI tests #448

bmagyar opened this issue Apr 23, 2020 · 3 comments
Labels
good first issue ROS Noetic

Comments

@bmagyar
Copy link
Member

bmagyar commented Apr 23, 2020

With the preparations for Noetic, the controller manager CLI tests became flaky.
These tests were not compatible with Python3 in the first place and a very rudimentary porting took place.

Please

  1. Rework the test in a more modern Python fashion.
  2. Re-enable the commented out section checking for spawning and switching controllers.

Related PR where I disabled these tests:
#446
This was referenced Apr 23, 2020
Closed
Closed
@bmagyar bmagyar mentioned this issue May 10, 2020
Closed
1 task
@olivier-stasse
Copy link
Contributor

olivier-stasse commented Jun 6, 2020 •
edited
Loading

Dear @bmagyar,
I gave it a try to this issue.

The test alone (all the other tests disables) works without python3 mistake. When cm_test AND cm_msgs_utils_rostest.test are enabled, my_controller3 is loaded before my_controller1 and then the resulting string is different from the expected output and makes the test failed.

Should we enforce the fact that my_controller1 has to be loaded before my_controller3 ? If this is not the case, then we should test both output possibilities I guess.

Wrt to new python3 do you suggest to use mock ?

olivier-stasse added a commit to olivier-stasse/ros_control that referenced this issue Jun 8, 2020
@olivier-stasse
[tests] Split controller_manager_scripts.txt in two parts.
9418002 
The second part is now called controller_manager_interface_test.
This fixes part 2 of issue ros-controls#448
olivier-stasse added a commit to olivier-stasse/ros_control that referenced this issue Jun 8, 2020
@olivier-stasse
[tests] Split controller_manager_scripts.txt in two parts.
4162e78 
The second part is now called controller_manager_interface_test.
This fixes part 2 of issue ros-controls#448
@olivier-stasse
Copy link
Contributor

PR #462 provides a way of testing both output.
It does not implements a more modern Python test but is python3 compliant.

bmagyar pushed a commit that referenced this issue Jun 17, 2020
@olivier-stasse
Fixes part 2 of issue#448 [Noetic] Rework and re-enable spawning and …
ab33807 
…switching CLI tests (#462)

* [tests] Split controller_manager_scripts.txt in two parts.
The second part is now called controller_manager_interface_test.
This fixes part 2 of issue #448

* [test] Modification of the validation test.
When commenting the first test (cm_msg_utils_test.py) the output
is correct, while uncommented an inversion of the output happens.
As this test is a pure python with no interaction with the
controller_manager, it is very likely a race condition due either to
Python or to the operating system. As the output is not fundamentally
wrong, and as the tests are heavily relying on timing and subprocesses
which are making the reproducibility difficult, this PR accepts both
solution:
[my_controller1, my_controller3] order
or
[my_controller3, my_controller1] order
@bmagyar
Copy link
Member Author

bmagyar commented Jun 17, 2020

Thank you for this, I think we can live with the quality of that Python code.

@bmagyar bmagyar closed this as completed Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue ROS Noetic
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@olivier-stasse @bmagyar