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

remove conceptual dependency on ROS_DOMAIN_ID in launch_testing #253

Closed
wjwwood opened this issue May 29, 2019 · 11 comments · Fixed by #256
Closed

remove conceptual dependency on ROS_DOMAIN_ID in launch_testing #253

wjwwood opened this issue May 29, 2019 · 11 comments · Fixed by #256
Assignees
Labels
bug Something isn't working

Comments

@wjwwood
Copy link
Member

wjwwood commented May 29, 2019

Currently launch_testing has options/features related to ROS_DOMAIN_ID:

# TODO(hidmic): Provide this option for rostests only.

This should not be the case, it is inappropriate for anything in this repository to depend on, reference, or really even mention anything from ROS.

There's a TODO, which I linked to, but I opened this issue to raise the severity. This needs to be changed.

@hidmic @pbaughman please don't let things like this leak into launch or launch_testing in the future.

@wjwwood wjwwood added the bug Something isn't working label May 29, 2019
@pbaughman
Copy link
Contributor

Do you have any suggestions about how to fix it? The cure (removing this) seems worse than the disease right now

@pbaughman
Copy link
Contributor

Could we add an extension point to launch_test so that when launch_testing_ros is installed you get the isolation behavior?

@pbaughman
Copy link
Contributor

I guess we could also add a different runner with a different name to run tests that need ROS

@hidmic
Copy link
Contributor

hidmic commented May 30, 2019

This should not be the case, it is inappropriate for anything in this repository to depend on, reference, or really even mention anything from ROS.

I fully agree. As it stands, this option could be moved to the ros2test CLI command with little to no effort (owing to the fact it's just an envvar).

@pbaughman
Copy link
Contributor

@hidmic will we add a different cmake function that calls the ros2cli command? The dependency graph for that cmake function will be quite long, but maybe not that much longer than launch_ros

@hidmic
Copy link
Contributor

hidmic commented May 30, 2019

@pbaughman there is one already, add_ros_test().

@pbaughman
Copy link
Contributor

@hidmic I want to refactor the domain coordinator into its own package so I can do this ament/ament_cmake#172 so lets's make sure we coordinate on this.

@ivanpauno
Copy link
Member

ivanpauno commented Jun 4, 2019

@hidmic I want to refactor the domain coordinator into its own package so I can do this ament/ament_cmake#172 so lets's make sure we coordinate on this.

Sounds good. Where should the domain coordinator live?
If it doesn't fit well in an existing repo, I will need a new one (I can't create a new one).

@ivanpauno ivanpauno self-assigned this Jun 4, 2019
@pbaughman
Copy link
Contributor

@ivanpauno In the WIP I've got now, it lives in ament_cmake_ros along with a new ament_cmake_pytest_isolated and an ament_cmake_gtest_isolated package

@ivanpauno
Copy link
Member

@ivanpauno In the WIP I've got now, it lives in ament_cmake_ros along with a new ament_cmake_pytest_isolated and an ament_cmake_gtest_isolated package

I think ament_cmake_pytest_isolated and ament_cmake_gtest_isolated should live in ament_cmake_ros. But the code of the domain coordinator could be in a separated repo, as it is really a python package (and not cmake stuff). It's ok for me having it there though.

@pbaughman
Copy link
Contributor

@ivanpauno Let's talk about it in the MR once I open it, but there's one cmake thing that I jammed into the domain_coordinator package - it might make sense to move it, or it might not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants