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

Avoid the limitation of not being able to import rclpy C extensions at module level #417

Closed
wants to merge 1 commit into from

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Sep 5, 2019

Kind of reverting the old #147, plus solving the "not nice grouping" problem in another way.

See discussion: #413 (comment)


There's some new nightly failures that are somewhat related with this problem, eg:
https://ci.ros2.org/view/nightly/job/nightly_win_rel/1302/testReport/junit/(root)/projectroot/test_client/

Basically, a pytest plugin is trying to import the C extensions before we "hack" the import in __init__.py file of the test directory.
This PR doesn't solve that problem.
I'm not to sure why this is only hitting as on Windows.
There are some possible solutions:

  • avoiding importing rclpy at module level in the plugin
  • Figuring out why we're loading that pytest plugin on Windows and not in the other os (rclpy doesn't depend on the package that provides the plugin).
  • Change how we install python packages, to avoid doing weird imports while testing.

…t module level

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the enhancement New feature or request label Sep 5, 2019
@ivanpauno ivanpauno self-assigned this Sep 5, 2019
@dirk-thomas
Copy link
Member

There's some new nightly failures that are somewhat related with this problem, eg:
https://ci.ros2.org/view/nightly/job/nightly_win_deb/1366/testReport/junit/(root)/projectroot/test_client/

The referenced test passed?

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

There's some new nightly failures that are somewhat related with this problem, eg:
https://ci.ros2.org/view/nightly/job/nightly_win_deb/1366/testReport/junit/(root)/projectroot/test_client/

The referenced test passed?

Sorry, wrong link. Updated post.

@dirk-thomas
Copy link
Member

@hidmic
Copy link
Contributor

hidmic commented Sep 5, 2019

The original link shows the regression https://ci.ros2.org/view/nightly/job/nightly_win_deb/1366/testReport/junit/(root)/projectroot/test_client/, but it can also be seen everywhere in CI. There's ros2/build_farmer#235 tracking it.

@ivanpauno
Copy link
Member Author

There's something weird in the links, when I enter the first time, they show the regression and the error message.
When I enter again a time later, they show Passed.
If I enter to the job status, navigate through the failing tests and enter again, I see the correct regression message ... and the same link as before.

It happened again with @hidmic link ....

@ivanpauno
Copy link
Member Author

ivanpauno commented Sep 5, 2019

The grouping of the tests is looking well after this patch, see:
https://ci.ros2.org/job/ci_linux/8072/testReport/

import rclpy.impl # noqa
import test_rclpy # noqa
# this will load rclpy from the build folder
sys.path.insert(0, os.environ.get('RCLPY_TEST_LIBRARY_DIR'))
Copy link
Contributor

@hidmic hidmic Sep 6, 2019

Choose a reason for hiding this comment

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

@ivanpauno using that envvar is so shady... there has to be a natural way to do this. I wonder what setuptools does under the hood when installing a regular Python package.

@dirk-thomas
Copy link
Member

There's something weird in the links

If you look at the list of tests (https://ci.ros2.org/view/nightly/job/nightly_win_deb/1366/testReport/junit/(root)/projectroot/) you will find that there are three test results named test_client.

@ivanpauno
Copy link
Member Author

If you look at the list of tests (https://ci.ros2.org/view/nightly/job/nightly_win_deb/1366/testReport/junit/(root)/projectroot/) you will find that there are three test results named test_client.

That's really inconvenient. This other doesn't have a repeated name:
https://ci.ros2.org/view/nightly/job/nightly_win_deb/1366/testReport/junit/(root)/projectroot/test_parameters_callback/

@ivanpauno using that envvar is so shady... there has to be a natural way to do this. I wonder what setuptools does under the hood when installing a regular Python package.

The current solution is also shady ...

@dirk-thomas
Copy link
Member

That's really inconvenient.

More like not smart naming of our tests. All three tests have the same name:

  • rclcpp test client
  • rclcpp_action test client
  • rclpy test client

Giving them more precise and therefore unique names would resolve this.

@ivanpauno
Copy link
Member Author

Checking than this still have failures on windows when launch_testing_ros is present:

  • Windows Build Status

@ivanpauno
Copy link
Member Author

Closing, because this is outdated.
We should still investigate ways of solving this issue.

@ivanpauno ivanpauno closed this May 22, 2020
@ivanpauno ivanpauno deleted the ivanpauno/avoid-limiting-impl-import branch May 22, 2020 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants