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

Move pytest entrypoints to own module #278

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Oct 11, 2021

Alternative to #277
Attempts to fix ros2/rclpy#831

This moves the pytest entrypoints to a separate module, and makes them import anything ROS specific as late as possible. This should prevent packages like ament_package importing downstream packages like rclpy.

@sloretz
Copy link
Contributor Author

sloretz commented Oct 11, 2021

This branch is on the commit just before #276, so windows debug CI should show if the issue is fixed or not.

Windows debug: build / test all Build Status

Edit: But I forgot CI merges with master - trying again.

@sloretz
Copy link
Contributor Author

sloretz commented Oct 11, 2021

The second commit also reverts #276 for testing the approach purposes

Windows debug CI all: Build Status

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Besides the test failure, the change LGTM.

Do you know if isolating pytest hooks like this is a best practice documented anywhere?
Also, should we consider doing something similar for launch_testing?

@sloretz
Copy link
Contributor Author

sloretz commented Oct 14, 2021

Do you know if isolating pytest hooks like this is a best practice documented anywhere?

I've never heard of it before. IIUC this PR is solving something specific to merged workspaces and how we run CI, and not implementing a general Python workflow. In our CI all the packages are built before any tests are run, which installs them into the install space. If that install space is merged then when tests are run all packages have access to all packages that have been built. Even the lowest level packages (like ament_package) get pytest hooks in higher level ones.

Also, should we consider doing something similar for launch_testing?

Maybe, but I haven't figured out why the _rclpy_pybind11 import failed (it had access to the install space after all) so I don't know if the same issue is likely to apply to that package given its dependencies.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

The only weird thing is that the file called hooks.py does not contain the definition of any pytest hook now.
(IIRC pytest_launch_collect_makemodule and pytest_configure are called "hooks" in pytest parlance)

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
@sloretz
Copy link
Contributor Author

sloretz commented Oct 27, 2021

Rebased to include just the fix, and not a revert revert of #274

@sloretz
Copy link
Contributor Author

sloretz commented Oct 27, 2021

Full CI

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

@jacobperron
Copy link
Member

jacobperron commented Nov 11, 2021

I'm not sure why we are seeing those qt_dotgraph test failures. That package does not transitively depend on launch_ros...
I was not able to reproduce the failures locally and haven't seen them appear in any other job. Re-triggering to see if it's still an issue:

  • Linux: Build Status
  • Linux-aarch64: Build Status (edit: rclcpp.test_time is an existing failure).
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

macOS build failure seems unrelated, running it again:

  • macOS Build Status

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.

Regressions in windows debug CI DLL load failed while importing _rclpy_pybind11
3 participants