Add in tests for the static executor by clalancette · Pull Request #1331 · ros2/rclcpp

clalancette · 2020-09-24T18:06:44Z

This PR adds in tests for the StaticSingleThreadedExecutor and a helper class, StaticExecutorEntitiesCollector. With this PR in place, I see 97% line coverage on static_single_threaded_executor.cpp and 97% line coverage on static_executor_entities_collector.cpp.

Along the way, I found a few bugs and other problems that I've fixed:

There were some typos in comments.
There were a bunch of places where we could add const to variables to give the compiler a bit more of a hint.
There were a couple of places where we were calling the copy constructor when passing a map into a method.
There was a bug when calculating whether to add a guard condition after adding a new node.

Note that this is still in draft because I want to run CI on it (particularly for Windows). I'll pull it out of draft once CI comes back clean.

clalancette · 2020-09-24T18:07:47Z

CI:

Linux
Linux-aarch64
macOS
Windows

brawner

Most of my comments are just due to some formatting that seems a bit different then I've seen elsewhere in rclcpp. Also, I'm curious whether this makes sense as several PRs rather than just one larger one.

rclcpp/include/rclcpp/executors/static_executor_entities_collector.hpp

rclcpp/src/rclcpp/executors/static_executor_entities_collector.cpp

brawner · 2020-09-24T19:04:26Z

rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp

+
+TEST_F(TestStaticExecutorEntitiesCollector, add_callback_group) {
+  auto node = std::make_shared<rclcpp::Node>("node1", "ns");
+  rclcpp::CallbackGroup::SharedPtr cb_group = node->create_callback_group(


A bit of a nitpick here, but I think using auto would let you put these two lines onto one, which would help readability a small bit with all these new tests. Likewise, I would recommend to avoid abbreviations like 'cb_group' if possible (either group or callback_group). Fix here and below

Would that actually work, though? If I do that, then the node pointer becomes a temporary, and then wouldn't it be destroyed immediately after this line (since there are no more handles to it)?

We discussed this offline. @brawner was suggesting that I combine this line with the next one by using auto. My personal preference is to not use auto, so I think we'll just leave this here for now. @brawner Let me know if you disagree.

Disagreements about the awesomeness and power of auto in a unit test aside, I'm fine leaving these as is.

rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp

brawner · 2020-09-24T19:16:37Z

rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp

+  auto subscription =
+    node->create_subscription<test_msgs::msg::Empty>(
+    "topic", rclcpp::QoS(10), [](test_msgs::msg::Empty::SharedPtr) {});
+  auto timer =


Nit: was this line break intentional?

Not sure about this one. uncrustify definitely told me to make this change, though you are right that it can probably be collapsed to one line.

Strange, uncrustify is happy with this on my machine,

auto timer = node->create_wall_timer(std::chrono::seconds(60), []() {});

rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp

rclcpp/test/rclcpp/executors/test_static_single_threaded_executor.cpp

clalancette · 2020-09-24T19:43:08Z

Most of my comments are just due to some formatting that seems a bit different then I've seen elsewhere in rclcpp.

I honestly just write code in my own style, and then do what uncrustify tells me to make it pass the linters. It is often the case that several things would satisfy uncrustify, but I just always take the one it tells me to do.

Also, I'm curious whether this makes sense as several PRs rather than just one larger one.

Yeah, I debated about that. I decided to put it into a single PR for a few reasons:

These changes did come up while writing the tests here, so they are mostly related.
My plan was to "Rebase and Merge" this set so that they are individual commits.
Expediency.

If you'd still like me to split it up, how many more PRs would you suggest? I can see arguments for 1, 2, 3, 4, or 5 :).

brawner · 2020-09-24T20:13:58Z

how many more PRs would you suggest?

I waffled myself on how many would make sense. I think separating out the API changes would be good and then you can tag some reviewers that might have more insight into the intended use of these classes than myself. Otherwise, it's just a matter of backportability.

When adding coverage our general guiding principle has been that bugs should be back portable to whatever ros distro's make sense and tests should be backportable to foxy. However, I was under the impression you were adding coverage for stuff mostly added only in rolling, so separating out into more PRs might not provide much benefit.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

We need to determine if this is a new node *before* adding it to the list; otherwise, it will always be treated as an old node. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

We get to 97% code coverage with these tests in place. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2020-09-25T14:03:24Z

All right, I've rebased and left only the changes that don't affect API here. I'll open a separate PR for the API changes presently. I'm going to mark this as an in-progress PR now, and run another round of CI here.

clalancette · 2020-09-25T14:06:02Z

CI:

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-09-25T17:26:07Z

rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp

+  auto subscription =
+    node->create_subscription<test_msgs::msg::Empty>(
+    "topic", rclcpp::QoS(10), [](test_msgs::msg::Empty::SharedPtr) {});
+  auto timer =


Strange, uncrustify is happy with this on my machine,

auto timer = node->create_wall_timer(std::chrono::seconds(60), []() {});

brawner · 2020-09-25T17:27:00Z

rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp

+
+TEST_F(TestStaticExecutorEntitiesCollector, add_callback_group) {
+  auto node = std::make_shared<rclcpp::Node>("node1", "ns");
+  rclcpp::CallbackGroup::SharedPtr cb_group = node->create_callback_group(


Disagreements about the awesomeness and power of auto in a unit test aside, I'm fine leaving these as is.

clalancette · 2020-09-25T19:42:52Z

Thanks for the reviews!

clalancette requested review from Blast545 and brawner September 24, 2020 18:06

brawner reviewed Sep 24, 2020

View reviewed changes

clalancette force-pushed the clalancette/test-static-executor branch from 83c1414 to cc6ffde Compare September 24, 2020 20:13

clalancette added 5 commits September 25, 2020 13:37

Fix typos in comments.

5b54fae

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Bugfix when adding callback group.

10ddcfe

We need to determine if this is a new node *before* adding it to the list; otherwise, it will always be treated as an old node. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Get one more line of code coverage in memory_strategy.cpp

af1e1a1

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Many more tests for the static_executor_entities_collector.

5563bd1

We get to 97% code coverage with these tests in place. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add more tests for the static single threaded executor.

bb4cb74

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette force-pushed the clalancette/test-static-executor branch from cc6ffde to bb4cb74 Compare September 25, 2020 14:01

clalancette marked this pull request as ready for review September 25, 2020 14:03

clalancette mentioned this pull request Sep 25, 2020

Cleanup the static executor API #1334

Merged

brawner approved these changes Sep 25, 2020

View reviewed changes

clalancette merged commit 148d295 into master Sep 25, 2020

delete-merged-branch bot deleted the clalancette/test-static-executor branch September 25, 2020 19:42

clalancette mentioned this pull request Oct 7, 2020

Clear members for StaticExecutorEntitiesCollector to avoid shared_ptr dependency #1303

Merged

brawner mentioned this pull request Oct 8, 2020

[foxy backport] Add executor unit tests #1336 #1395

Merged

Conversation

clalancette commented Sep 24, 2020

Uh oh!

clalancette commented Sep 24, 2020

Uh oh!

brawner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette Sep 24, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clalancette commented Sep 24, 2020

Uh oh!

brawner commented Sep 24, 2020

Uh oh!

clalancette commented Sep 25, 2020

Uh oh!

clalancette commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette commented Sep 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clalancette Sep 24, 2020 •

edited

Loading