search

ApexAI / apex_rostest

Framework for ROS2 Integration Testing
5 stars 6 forks source link

[QUESTION] Naming and location #18

Closed hidmic closed 5 years ago

hidmic commented 5 years ago

As this testing framework matures and attains widespread use throughout ROS 2 packages, a couple questions come up:

hidmic commented 5 years ago

Ping @wjwwood @mjcarroll @sloretz.

mjcarroll commented 5 years ago

I think it would be best to live in ros2/rostest, if the Apex folks are okay with that.

pbaughman commented 5 years ago

I think @dejanpan can answer for the Apex side of this question.

wjwwood commented 5 years ago

Well there's actually two things in this repository right now, launch testing stuff and ROS specific launch testing stuff. This is my invisioned end state:

Not all of these changes need to be realized right now (or to complete this task), the important thing is that the ROS agnostic code in this repository is moved to the launch or launch_testing packages in the ros2/launch repository, and that the rest of the code here is moved into the ros2 organization.

We don't have to split off the ros2/launch_ros repository yet, but we might as well while we're refactoring.

Just to reinforce why the split is needed, launch_ros and therefore code in this repository use rclpy, so rclpy itself and anything below it like rcl or maybe even rclcpp (if we end up needing it in launch_ros or rostest in the future) cannot use launch_ros or rostest, but they still need some facilities to do testing with launch, so instead they'd depend on and use launch_testing.

I had consider renaming this repository launch_ros_testing to follow the naming scheme in ROS 2 already, but rostest is more concise and you guys seem to prefer that, which I'm fine with. On the other hand, it has more collision with rostest from ROS 1. It's been nice to have some conceptual space between launch/launch_ros and roslaunch from ROS 1.

hidmic commented 5 years ago

I think that makes a lot of sense @wjwwood. @dejanpan thoughts?

pbaughman commented 5 years ago

This PR will remove the last rclpy dependency from apex_launchtest, and put in a CI check that will notify us if it's accidentally reintroduced. The dependency tree will look like this:

$ colcon list --topological-graph --packages-up-to apex_launchtest
ament_flake8        + *****
osrf_pycommon        +   *.
ament_pep257          +****
ament_copyright        + **
ament_index_python      + *
launch                   +*
apex_launchtest           +
dejanpan commented 5 years ago

@wjwwood @hidmic @mjcarroll @sloretz
regarding https://github.com/ApexAI/apex_rostest/issues/18#issuecomment-468468732, Apex has no problem wrt to moving this repo into the ros2 org and renaming stuff. We can do it in whatever way as long as it will help make ROS 2 be more tested.

Now, @pbaughman and I think that we do not 100% understand the proposed split so we will try to repeat what @wjwwood said with the links:

And then we also have https://github.com/ApexAI/apex_rostest/tree/master/apex_launchtest_cmake (cmake integration) which would be needed for some of the above packages - which ones?

The 2 remaining Qs then are:

  1. who will do the refactor and split (we can as long as we well define the process)?
  2. will Pete and I have access to above 3 repos in ros2 org?
wjwwood commented 5 years ago

No not exactly, in my imagined layout test_launch_ros is a test package (indicated by starting with test_) which tests launch_ros itself. I was thinking that rostest in my above layout would be where https://github.com/ApexAI/apex_rostest/tree/master/apex_launchtest_ros would live. I was copying your name apex_rostest without the apex prefix.

And then we also have https://github.com/ApexAI/apex_rostest/tree/master/apex_launchtest_cmake (cmake integration) which would be needed for some of the above packages - which ones?

I would guess this goes in the rostest package, but it could be separated, that way the launch testing stuff could be used without cmake (e.g. from a pure Python package or something else).

wjwwood commented 5 years ago

I created this issue on the ros2/launch repository to track the work on this:

https://github.com/ros2/launch/issues/208

Please have a look to make sure there are no large issues. We're beginning work on this now.

I'm going to close this in favor of that issue.