Add `rclrs_test` package for unit/integration tests

[christopherjreid]

This package is meant to test behaviors with dependencies that don't belong in the rclrs crate - namely, msgs

Addresses #237

[christopherjreid]

@nnmm Is this package structure what you envisioned for #237?

[christopherjreid]

Based on previous feedback, I'm wondering about structuring this as

• Setup a graph with a couple of nodes, publishers, subscribers, services and clients
• Run tests for all the graph.rs stuff

Unfortunately it doesn't look like Rust supports a gtest-like SetUp() and TearDown(), so that means (I think) that this would all go into a single test. Does that make sense to do?

[nnmm]

Thanks for tackling this!

@nnmm Is this package structure what you envisioned for #237?

Yep. A few early-stage comments:

• We probably want test_msgs rather than std_msg for this package. Though I just compiled it and noticed that we have a bug in rosidl_runtime_rs or rosidl_generator_rs that we need to fix first :/
• Returning Result from the tests makes them a little more concise.
• You don't need mod tests inside graph_tests.rs. You can just put #[cfg(test)] directly on the modules in lib.rs. Maybe it's even possible to just add a single #![cfg(test)] (with an exclamation mark) to the top lib.rs instead.

Unfortunately it doesn't look like Rust supports a gtest-like SetUp() and TearDown(), so that means (I think) that this would all go into a single test. Does that make sense to do?

I think it would also be fine to have a freestanding setup function that you call in each test. Something like run_with_test_nodes<F>(test_fn: F) perhaps, which spins up the test nodes, runs the provided function, and then cleans up. I'd try that approach, and if spinning up the test nodes is too expensive, we can still group them into fewer tests.

[christopherjreid]

Hrm, looks like adding the test_msgs dependency also pulls lots of other depedencies in... nav_msgs, sensor_msgs, diagnostic_msgs, and more

error: failed to load source for dependency `diagnostic_msgs`

Caused by:
  Unable to update /workspace/install/diagnostic_msgs/share/diagnostic_msgs/rust

Caused by:
  failed to read `/workspace/install/diagnostic_msgs/share/diagnostic_msgs/rust/Cargo.toml`

Caused by:
  No such file or directory (os error 2)

Not sure what's going on with that....

[christopherjreid]

After rebasing and adding all the msg dependencies, it's building and running now. I'm getting a spurious failure with count_publishers and count_subscribers - after constructing 2 publishers/subscribers, they occasionally return 3 instead. This does not seem to happen when I run a single test individually (e.g. cargo test test_publishers), so seems to be concurrency related. I'd say it's quite likely they're conflicting with test_topic_names_and_types, which creates a single publisher and subscriber. I might just distribute that functionality into test_publishers and test_subscribers

[nnmm]

After rebasing and adding all the msg dependencies, it's building and running now. I'm getting a spurious failure with count_publishers and count_subscribers - after constructing 2 publishers/subscribers, they occasionally return 3 instead. This does not seem to happen when I run a single test individually (e.g. cargo test test_publishers), so seems to be concurrency related. I'd say it's quite likely they're conflicting with test_topic_names_and_types, which creates a single publisher and subscriber. I might just distribute that functionality into test_publishers and test_subscribers

Yes, that's annoying. I think it's also fine to run cargo test with --jobs 1 to avoid crosstalk.

[christopherjreid]

Updated to combine everything into a single test - constructing the graph, then calling extracted functions for checking publishers, subscriptions, etc. I know that's a bit of an anti-pattern, but I felt like it was the best way to handle the concurrency issues.

Looks like it still has a compilation failure on GH though, so gonna troubleshoot that later

[christopherjreid]

I can't reproduce that error locally - looks like a problem with the export dependencies of test_msgs? Not sure how to proceed...

[nnmm]

@christopherjreid You can try adding a line to the workflow that sources the install directory. Probably

. ${{ steps.build.outputs.ros-workspace-directory-name }}/install/setup.bash

but if that's not the right path, you might need to dig around a bit by adding ls commands etc.

[christopherjreid]

Thanks for the tips! That, and some test changes (apparently Galactic/cyclonedds do not allow publishing to the same topics with different types) got all the tests to pass!

So this is ready for official review now

[nnmm]

Looks good in general, I'm just wondering if it's really necessary to group everything in one test because of the interference problem. Would using separate namespaces for each test solve the problem too?

[christopherjreid]

I just added a new commit that creates a new test case for publishers, and one for subscribers. I feel like this is a lot noisier than the previous method, though there is definitely value in having "smaller" (or at least more targeted) test cases like this. What do you think, should I continue on this path for the remaining 3 test cases? (topics, services, and clients)?

[nnmm]

I just added a new commit that creates a new test case for publishers, and one for subscribers. I feel like this is a lot noisier than the previous method, though there is definitely value in having "smaller" (or at least more targeted) test cases like this. What do you think, should I continue on this path for the remaining 3 test cases? (topics, services, and clients)?

Hmm, I'd tend towards writing the tests in the most idiomatic way for now and just solve the interference problem by running them sequentially for now (i.e. adding --jobs 1 to CI for this package). Maybe we'll find a better approach for test isolation in the future, involving ROS_DOMAIN_ID or so.

Is that fine with you too @esteve?

[christopherjreid]

Haven't looked at this in a couple days, but noticed CI had a test failure. Running this locally, I notice there's about a 1/15 chance or so that it randomly fails one of the tests, because the publisher/subscription/service/client that -should- exist does not.

Nothing within the test file itself should have concurrency issues anymore - is there some under-the-hood race condition that I'm hitting? Should I sleep for a bit after constructing the graph?

[nnmm]

Haven't looked at this in a couple days, but noticed CI had a test failure. Running this locally, I notice there's about a 1/15 chance or so that it randomly fails one of the tests, because the publisher/subscription/service/client that -should- exist does not.

Nothing within the test file itself should have concurrency issues anymore - is there some under-the-hood race condition that I'm hitting? Should I sleep for a bit after constructing the graph?

It seems like nodes won't get immediately notified of other entities (topics, publishers etc.), but only asynchronously.

As far as I can see in https://github.com/ros2/rclcpp/blob/rolling/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpphttps://github.com/ros2/rclcpp/blob/rolling/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp, they use a wait_for_graph_change() function with a timeout. Maybe you can look into porting that function?

[nnmm]

Hi @christopherjreid, I've been quiet since I've been on vacation, but please let me know if there's anything I can help you with here :) If you prefer, I could also take a stab at porting wait_for_graph_change().

[christopherjreid]

I would like to do it, just have been very busy (renovating our house). Hopefully I'll have some time this week to take a stab at it!

[nnmm]

Okay, great – there's no rush at all.

[christopherjreid]

Not sure why CI is failing. I'm rebased on main...

[nnmm]

Not your fault. I just reran CI on another PR, and it failed too. We should report this error in https://github.com/ros-tooling/setup-ros or so, or check if someone already did:

  The following packages have unmet dependencies:
   libignition-math6-dev : Depends: libignition-cmake2-dev (>= 2.13.0) but 2.12.1-2~jammy is to be installed
  W: Target Packages (main/binary-amd64/Packages) is configured multiple times in /etc/apt/sources.list.d/ros-latest.list:1 and /etc/apt/sources.list.d/ros2-latest.list:1
  W: Target Packages (main/binary-all/Packages) is configured multiple times in /etc/apt/sources.list.d/ros-latest.list:1 and /etc/apt/sources.list.d/ros2-latest.list:1
  E: Unable to correct problems, you have held broken packages.

[nnmm]

The CI error should now be resolved – but you also need to rebase on main to get clippy fixes that were made necessary by upgrading the Rust compiler.

[christopherjreid]

Ready for merging now, I think!