Add ament_add_pytest_isolated and ament_add_gtest_isolated by pbaughman · Pull Request #3 · ros2/ament_cmake_ros

pbaughman · 2019-06-05T18:38:53Z

This PR depends on:

~~Add runner option to ament_add_test ament/ament_cmake#174~~ Already Merged

This PR blocks

Use the new 'RUNNER' argument provided in ament_cmake PR 174 to create new versions of ament_add_gtest and ament_add_pytest_test that runs with ROS_DOMAIN_ID isolation if no ROS_DOMAIN_ID is already set.

This will also coordinate domain IDs with the ros2test tool

Testing

I've tested this PR in conjunction with ament_cmake 174 by changing rcl and rclpy to use the 'isolated' version of these macros, then running

terminal 1$ colcon test --packages-select rcl
terminal 2$ colcon test --packages-select rclpy

And verifying in the test logs that we see tests using ROS_DOMAIN_ID 1 as well as ROS_DOMAIN_ID 2

Test Cases:

Run without a ROS_DOMAIN_ID and verify that ROS_DOMAIN_ID is selected by looking at the latest test logs
Run to colcon test commands in parallel and verify that different ROS_DOMAIN_IDs are chosen
Set ROS_DOMAIN_ID and verify that it's not overridden in the log
Unset ROS_DOMAIN_ID and set DISABLE_ROS_ISOLATION and verify that ROS_DOMAIN_ID is not overridden in the log

Example usage

In rclpy:

--- a/ws/src/rclpy/rclpy/CMakeLists.txt                       
+++ b/ws/src/rclpy/rclpy/CMakeLists.txt                       
@@ -12,6 +12,7 @@ endif()                                                       
                                                                                
 find_package(ament_cmake REQUIRED)                                             
 find_package(ament_cmake_python REQUIRED)                                      
+find_package(ament_cmake_pytest_isolated REQUIRED)                             
 find_package(rcl REQUIRED)                                                     
 find_package(rcl_yaml_param_parser REQUIRED)                                   
 find_package(rcutils REQUIRED)                                                 
@@ -113,7 +114,7 @@ if(BUILD_TESTING)                                           
     string(REPLACE "\\" "/" ament_index_build_path "${ament_index_build_path}")
   endif()                                                                      
   if(NOT _typesupport_impls STREQUAL "")                                       
-    ament_add_pytest_test(rclpytests test                                      
+    ament_add_pytest_isolated_test(rclpytests test                             
       PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}"                                 
       APPEND_ENV AMENT_PREFIX_PATH=${ament_index_build_path}                   
         PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR}

In rcl:

--- a/ws/src/rcl/rcl/test/CMakeLists.txt                                                                                                  
+++ b/ws/src/rcl/rcl/test/CMakeLists.txt                                                                                                  
@@ -1,4 +1,5 @@                                                                                                                                             
 find_package(ament_cmake_gtest REQUIRED)                                                                                                                   
+find_package(ament_cmake_gtest_isolated REQUIRED)                                                                                                          
 find_package(ament_cmake_pytest REQUIRED)                                                                                                                  
                                                                                                                                                            
 find_package(test_msgs REQUIRED)                                                                                                                           
                                                                                                         
--- a/ws/src/rcl/rcl/test/cmake/rcl_add_custom_gtest.cmake                                                                                
+++ b/ws/src/rcl/rcl/test/cmake/rcl_add_custom_gtest.cmake                                                                                
@@ -74,7 +74,7 @@ macro(rcl_add_custom_gtest target)                                                                                                        
   endif()                                                                                                                                                  
                                                                                                                                                            
   # Pass args along to ament_add_gtest().                                                                                                                  
-  ament_add_gtest(${target} ${_ARG_SRCS} ${_ARG_ENV} ${_ARG_APPEND_ENV} ${_ARG_APPEND_LIBRARY_DIRS}                                                        
+  ament_add_gtest_isolated(${target} ${_ARG_SRCS} ${_ARG_ENV} ${_ARG_APPEND_ENV} ${_ARG_APPEND_LIBRARY_DIRS}                                               
                   ${_ARG_SKIP_TEST} ${_ARG_TIMEOUT})                                                                                                       
   # Check if the target was actually created.                                                                                                              
   if(TARGET ${target})

- Add a domain_coordinator package containing the functionality from launch_testing and a run_test_isolated.py script that can be used by the isolated cmake test functions instead of the default run_test.py - Add ament_cmake_gtest_isolated to run gtests with ROS_DOMAIN_ID isolation - Add ament_cmake_pytest_isolated to run pytests with ROS_DOMAIN_ID isolation Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-06-05T18:48:38Z

@tfoote This PR should resolve the discussion we started a few weeks ago over here

ivanpauno

I let a few comments about domain_coordinator package.
I didn't review the rest.

pbaughman · 2019-06-05T20:43:24Z

@ivanpauno what if I add one more package - ament_cmake_test_isolated, which would provide run_test_isolated.py in a way that its path could be referenced by cmake. Then domain_coordinator could indeed be made into an ament_python package

ivanpauno · 2019-06-05T21:01:56Z

Maybe, but then I'm not sure how I would look up the path here and here

mmm, I don't know either (without creating a FindDomainCoordinator.cmake and installing it from setup.py).

@ivanpauno what if I add one more package - ament_cmake_test_isolated, which would provide run_test_isolated.py in a way that its path could be referenced by cmake. Then domain_coordinator could indeed be made into an ament_python package

If it's not way to looking up the path when changing the package to be ament_python, I think it's ok letting it as an ament_cmake package. In that case, setup.py file is not being used and should be deleted.

Your option of adding an extra package is also good.

- Make domain_coordinator an ament_python package - remove run_test_isolated.py from domain_coordinator - Add a new cmake package that provides run_test_isolated.py Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-06-05T21:46:26Z

@ivanpauno Ok - domain_coordinator is an ament_python package now. It can be moved to another repo if you wish. Package dependencies are not my strongest skill, so I hope I got the transitive dependency on domain_coordinator from ament_cmake_test_isolated right (see comment above)

ivanpauno

LGTM, I let some minor comments.

It's fine for me to leave the domain_coordinator package here.

ivanpauno · 2019-06-06T13:44:52Z

+
+    author='Pete Baughman',
+    author_email='pete.baughman@apex.ai',
+


I think that it would be good to add url, download_url, long_description, license, etc. See https://github.com/ros2/launch/blob/master/launch/setup.py as an example.

Sorry for my lazy "etc". keywords, classifiers and install_requires are missing. The three can be copied from the example.

- Fix copyright date from 2017->2019 - Simplify dependencies of isolated test packages - Put more information in domain_coordinator setup.py Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-06-06T15:47:02Z

@ivanpauno Thanks for your help with the dependency stuff. I believe I've done all of your feedback. I also updated the version element in all of the package.xml files to match the other package.xml files in ament_cmake_ros - I hope this was the right thing to do.

pbaughman · 2019-06-06T18:27:30Z

@ivanpauno I'm having some trouble with this following the merge of the PR in ament_cmake. . . investigating

pbaughman · 2019-06-06T18:43:28Z

Ok, False alarm above - a few files got deleted from my local workspace when I did a clean build. I've now successfully re-tested this with the merged ament_cmake changes. This is now free of blockers and ready to go!

ivanpauno

LGTM. One extra comment.

ivanpauno · 2019-06-06T19:22:30Z

+
+    author='Pete Baughman',
+    author_email='pete.baughman@apex.ai',
+


Sorry for my lazy "etc". keywords, classifiers and install_requires are missing. The three can be copied from the example.

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-06-06T19:43:46Z

@ivanpauno Oh, whoops - sorry - it has all the same fields as launch setup.py now

ivanpauno · 2019-06-06T20:10:01Z

Running CI (fastrtps, up to ament_cmake_gtest_isolated ament_cmake_pytest_isolated ament_cmake_test_isolated domain_coordinator):

Linux
Linux-aarch64
macOS
Windows

wjwwood

lgtm

Just some thoughts on the names (I don't think we need to change them but I wanted to note my thoughts in case others agree and want to change them):

ament_cmake_*_isolated strikes me as a bit broad, since it really only does isolation for ROS stuff, and I was expecting either ament_cmake_ros_*_isolated or maybe ament_cmake_*_ros_isolated. My very slight concern is that people will assume isolated means what it means for our build tools (merged vs isolated install for example) or that it has something to do with threading or test result output, rather than specifically being about ROS isolation. The first time you see anything about ROS is in the doc block of the cmake functions of similar name.

Also, domain_coordinator could similarly be more specific as ros_domain_coordinator, but again it's not that critical.

Just some food for thought, but I think the pr as-is is ok (aside from perhaps moving the documentation as I suggested in my comment).

dirk-thomas · 2019-06-06T21:20:00Z

Just to clarify the intended follow up: do you plan to update any / all existing calls to the non-isolated version of the CMake functions? If yes, I think we should be very confident in the naming of the function. Changing it later will be difficult. Maybe using a suffix like _with_unique_domain_id would be more informative to avoid confusion (obviously not a very short name / sentence / paragraph).

pbaughman · 2019-06-06T21:28:29Z

@wjwwood

I will rename the functions to be ament_cmake_*_ros_isolated
I will put the explanation of how ROS_DOMAIN_ID is either used or generated automatically or skipped in the doc_block for ament_cmake_test_isolated

@dirk-thomas
I wasn't planning to change every ament_cmake_gtest into ament_cmake_gtest_ros_isolated - just the ones that are causing problems. For example, I know that some of the tests in rcl that look up node names don't play well with other tests that start nodes. Likewise, the tests in rosbag2 that try to record information from all topics don't play well with other tests that publish on topics.

dirk-thomas · 2019-06-06T22:58:59Z

Maybe for some of those cases it would make sense to actually update the tests instead?

pbaughman · 2019-06-06T23:42:57Z

@dirk-thomas Maybe - that's the route I initially started to go down but after fixing up a few tests, I determined that that solving the more general problem of how to isolate ROS tests from one-another would be a cheaper way to accomplish the same thing.

I will keep my eyes peeled for tests with 'easy' fixes - but the rosbag2 tests in particular do not like to have any other publishers running on the system.

- Give a better name so this doesn't seem like the same thing as build isolated vs build merge install - Add the explanation of how ROS_DOMAIN_ID is selected in the cmake function documentation Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-06-07T16:34:59Z

Ok, this is ready to go again. We can squash the commits when we merge, right?

wjwwood · 2019-06-07T22:26:20Z

I'll re-review and run a new set of CI before merging but yes we squash when we merge.

wjwwood · 2019-06-07T22:35:03Z

CI:

Linux
Linux-aarch64
macOS
Windows

With settings:

build: --packages-up-to ament_cmake_ros_isolated_gtest ament_cmake_ros_isolated_pytest ament_cmake_ros_isolated_test domain_coordinator
test: --packages-select ament_cmake_ros_isolated_gtest ament_cmake_ros_isolated_pytest ament_cmake_ros_isolated_test domain_coordinator

pbaughman · 2019-06-10T18:06:50Z

sigh
~~I think the dependencies weren't right on this. I'm getting~~

14:   File "/path/ament_cmake_ros_isolated_test/share/ament_cmake_ros_isolated_test/cmake/run_test_isolated.py", line 21, in <module>
14:     import domain_coordinator
14: ImportError: No module named 'domain_coordinator'

~~When I do a non-merge install. Investigating~~

pbaughman · 2019-06-10T18:59:06Z

Never mind about the above - I'm a big dummy who typo'd the name of this package when I tried to use it.

dirk-thomas · 2019-06-11T17:07:25Z

Do we really need separate packages for each of these functions? And another one with #4?

I would suggest to rather move them into ament_cmake_ros.

dirk-thomas · 2020-08-28T16:00:10Z

+    use ROS2 without unexpected cross-talk between processes.
+    This is similar to the ROS1 rostest behavior of putting the ROS master on a unique port.
+
+    Users of get_coordintaed_gomain_id must keep the returned object alive.  If the returned


This is conceptionally really broken. In Python you can't rely on when an instance which isn't referenced anymore is being garbage collected. As a result the tests even warn about the sockets never being closed: https://ci.ros2.org/view/All/job/test_ci_windows/196/pytest-warnings/

See #9 for the follow up ticket.

@dirk-thomas The open port shouldn't outlive the run_test_isolated.py wrapper. The port will be returned to the pool of available ports when run_test_isolated.py is terminated

I agree that another use of the domain_coordinator object might consider a ROS_DOMAIN_ID "taken" for longer than you'd otherwise expect though. If someone changed how run_test_isolated.py works, or tries to re-use the domain_coordinator elsewhere they might be surprised.

Obviously the resource doesn't outlive the process lifetime. But that doesn't solve the API problem which in its current form results in warnings about unclosed handles.

Yeah - sorry, I edited at the same time you replied. The API documentation does kind of write a check that the implementation can't cash right now. I also agree that useless warnings are bad noise.

Pete Baughman added 2 commits June 5, 2019 11:39

ament_cmake_ros Add doc strings for cmake test functions

973a713

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman force-pushed the add_isolated_test branch from 1ea6217 to 973a713 Compare June 5, 2019 18:40

This was referenced Jun 5, 2019

Add runner option to ament_add_test ament/ament_cmake#174

Merged

Remove launch_testing dependency on ros domain ID ros2/launch#256

Merged

Move --disable-isolation argument from launch_testing to ros2test ros2/ros_testing#2

Merged

ivanpauno reviewed Jun 5, 2019

View reviewed changes

Comment thread domain_coordinator/cmake/run_test_isolated.py

Comment thread domain_coordinator/package.xml Outdated

pbaughman mentioned this pull request Jun 5, 2019

I would like to extend run_test.py to provide ROS_DOMAIN_ID isolation ament/ament_cmake#172

Closed

pbaughman commented Jun 5, 2019

View reviewed changes

Comment thread ament_cmake_test_isolated/CMakeLists.txt

ivanpauno reviewed Jun 6, 2019

View reviewed changes

Address PR comments

1566d97

- Fix copyright date from 2017->2019 - Simplify dependencies of isolated test packages - Put more information in domain_coordinator setup.py Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

ivanpauno approved these changes Jun 6, 2019

View reviewed changes

Add missing fields in setup.py

7bebefa

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

wjwwood approved these changes Jun 6, 2019

View reviewed changes

Comment thread ament_cmake_test_isolated/cmake/run_test_isolated.py

pbaughman force-pushed the add_isolated_test branch from b07a4d9 to 2b1af8d Compare June 7, 2019 16:34

wjwwood approved these changes Jun 7, 2019

View reviewed changes

wjwwood merged commit d699ccf into ros2:master Jun 7, 2019

pbaughman deleted the add_isolated_test branch June 8, 2019 03:50

dirk-thomas mentioned this pull request Jun 11, 2019

Add isolated gmock test #4

Merged

dirk-thomas mentioned this pull request Aug 28, 2020

fix domain coordinator API #9

Closed

dirk-thomas reviewed Aug 28, 2020

View reviewed changes


		author='Pete Baughman',
		author_email='pete.baughman@apex.ai',

Conversation

pbaughman commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Example usage

Uh oh!

pbaughman commented Jun 5, 2019

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pbaughman commented Jun 5, 2019

Uh oh!

ivanpauno commented Jun 5, 2019

Uh oh!

Uh oh!

pbaughman commented Jun 5, 2019

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanpauno Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

ivanpauno Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

pbaughman commented Jun 6, 2019

Uh oh!

pbaughman commented Jun 6, 2019

Uh oh!

pbaughman commented Jun 6, 2019

Uh oh!

ivanpauno left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanpauno Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

pbaughman commented Jun 6, 2019

Uh oh!

ivanpauno commented Jun 6, 2019

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dirk-thomas commented Jun 6, 2019

Uh oh!

pbaughman commented Jun 6, 2019

Uh oh!

dirk-thomas commented Jun 6, 2019

Uh oh!

pbaughman commented Jun 6, 2019

Uh oh!

pbaughman commented Jun 7, 2019

Uh oh!

wjwwood commented Jun 7, 2019

Uh oh!

wjwwood commented Jun 7, 2019

Uh oh!

pbaughman commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbaughman commented Jun 10, 2019

Uh oh!

dirk-thomas commented Jun 11, 2019

Uh oh!

pbaughman commented Jun 5, 2019 •

edited

Loading

ivanpauno left a comment •

edited

Loading

pbaughman commented Jun 10, 2019 •

edited

Loading

pbaughman Aug 28, 2020 •

edited

Loading