Remove unncecessary dependencies in tests

[cwecht]

This PR removes AMENT_DEPENDENCIES to the respective RMW implementation in the RMW specific tests. I don't quite get why they have been there in the first place: having the RMW implementation mentioned as ament dependency should only result in linking against the RMW implementation, shouldn't it? For me this doesn't make sense, as the RMW implementation sould be loaded dynamically based on the RMW_IMPLEMENTATION environment variable.

With this PR the CMakeLists.txt gets quite a bit simple, too.

[fujitatomoya]

i would like to have a couple of confirmation for this, for me it seems to be better to keep these.

CC: @clalancette @cottsay

[clalancette]

I think we should do something like this, but it will have to be more in-depth.

In particular, we currently compile the same test multiple times, one for each RMW implementation we've found. But as you've noted, we don't actually need to do that; we just need to set RMW_IMPLEMENTATION appropriately.

So I think we should go further here and refactor this whole thing to only compile each of the tests once. There are some examples on how to do this in https://github.com/ros2/system_tests/blob/rolling/test_rclcpp/CMakeLists.txt . Not only will this improve our compilation times, it will also allow us to remove the last use of ament_target_dependencies from this package.

@cwecht are you willing to look at doing what I outlined above?

[cwecht]

@cwecht are you willing to look at doing what I outlined above?

Yes, done :)

[clalancette]

@cwecht Thanks for getting this going.

First of all, with these changes I'm seeing my build times cut in half. So that is a great start.

Second, since we are removing the RMW_IMPLEMENTATION at build-time, we can also remove a whole slew of code inside the tests themselves that deal with all of that. I've pushed a commit which makes all of those changes (9c5363e).

Third, I found that some of the tests that were being run before weren't being run in this implementation. So I fixed that as well (ec44471).

Finally, the one real problem I have with this change is the rcl_add_custom_gtest_test. That is confusingly similar to the already existing rcl_add_custom_gtest. Also, it doesn't seem to be gtest-specific; it ends up calling ament_add_test, which doesn't really seem gtest-specific. And that cmake function has a lot of options that seem to be unused (I only see ENV and TIMEOUT being used anywhere). So I'd like to rethink that function a bit, either by using existing functionality, or by simplifying it and calling it something else. Thoughts?

[cwecht]

@clalancette

Finally, the one real problem I have with this change is the rcl_add_custom_gtest_test. That is confusingly similar to the already existing rcl_add_custom_gtest. Also, it doesn't seem to be gtest-specific; it ends up calling ament_add_test, which doesn't really seem gtest-specific. And that cmake function has a lot of options that seem to be unused (I only see ENV and TIMEOUT being used anywhere). So I'd like to rethink that function a bit, either by using existing functionality, or by simplifying it and calling it something else. Thoughts?

rcl_add_custom_gtest_test is a copy of ament_add_gtest_test. With ament_add_gtest_test the name of the test and the executable must be the same. With rcl_add_custom_gtest_test the can diverge. With respect of its current usage scenario rcl_add_custom_gtest_test is a bit to complex, yes.

[clalancette]

rcl_add_custom_gtest_test is a copy of ament_add_gtest_test. With ament_add_gtest_test the name of the test and the executable must be the same. With rcl_add_custom_gtest_test the can diverge. With respect of its current usage scenario rcl_add_custom_gtest_test is a bit to complex, yes.

OK, I see. Thanks for the context.

I think what I'd rather do here is to add a new optional argument (EXECUTABLE) to ament_add_gtest_test. Then we can just use ament_add_gtest_test here instead of basically duplicating it. Does that make sense?

If you are willing to do that, please open up a new PR to ament_cmake and I'm happy to review that one as well.

[cwecht]

@clalancette I opted for adding a parameter TEST_NAME in ament/ament_cmake#492 instead of EXECUTABLE as seems to fit a bit better.

[clalancette]

@cwecht Can you please add a Signed-off-by line to the latest commit? Once that is done, I think this is ready for CI. Thanks.

[cwecht]

@clalancette done.

@cwecht Can you please add a Signed-off-by line to the latest commit? Once that is done, I think this is ready for CI. Thanks.

done.

[clalancette]

Thanks. Here is CI:

• Linux
• Linux-aarch64
• Windows

[cwecht]

That last build failed because of an compilatin error in rclpy?

[clalancette]

That last build failed because of an compilatin error in rclpy?

Oh, right. This PR needs to be rebased due to the latest changes. I've gone ahead and done that.

[clalancette]

New CI:

• Linux
• Linux-aarch64
• Windows