[ros2lifecycle] Add test coverage for CLI #391

hidmic · 2019-11-05T20:17:36Z

Precisely what the title says. This pull requests adds integration for each and every ros2 lifecycle verb.

It current depends on the lifecycle package in the ros2/demos repository, creating a circular repo dependency. Fixture nodes must be duplicated and relocated, maybe simplifying these in the process.

hidmic · 2019-11-05T20:52:27Z

CI up to ros2lifecycle:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2019-11-06T14:55:42Z

Well, tests failed because I didn't push a local fix in launch. See ros2/launch#353.

hidmic · 2019-11-06T19:06:37Z

CI up to ros2lifecycle:

Linux
Linux-aarch64
macOS
Windows

BMarchi · 2019-11-11T11:38:52Z

ros2lifecycle/test/test_cli.py

+            assert lifecycle_command.wait_for_shutdown(timeout=10)
+        assert lifecycle_command.exit_code == launch_testing.asserts.EXIT_OK
+        assert launch_testing.tools.expect_output(
+            expected_lines=ALL_LIFECYCLE_NODE_TRANSITIONS,


This should not return any transition since you didn't give the --include-hidden-nodes argument

That's true, which makes me wonder how's this test passing. Looks like ros2 lifecycle list is simply ignoring the --include-hidden-nodes flag:

ros2cli/ros2lifecycle/ros2lifecycle/verb/list.py

Lines 43 to 64 in 9ce402d

def main(self, *, args): # noqa: D102

with NodeStrategy(args) as node:

node_names = get_node_names(

node=node, include_hidden_nodes=args.include_hidden_nodes)

with DirectNode(args) as node:

if args.all:

transitions = call_get_transition_graph(

node=node, states={args.node_name: None})

else:

transitions = call_get_available_transitions(

node=node, states={args.node_name: None})

transitions = transitions[args.node_name]

if isinstance(transitions, Exception):

return 'Exception while calling service of node ' \

"'{args.node_name}': {transitions}" \

.format_map(locals())

for t in transitions:

print(

'- {t.transition.label} [{t.transition.id}]\n'

'\tStart: {t.start_state.label}\n'

'\tGoal: {t.goal_state.label}'.format_map(locals()))

Test fixed in fc5977c.

Can you make the same tests but without the hidden flag? To ensure the proper functionality of the flag (I'm seeing that it will work, but just in case)

Done in eb2f8d3

BMarchi · 2019-11-11T11:41:43Z

ros2lifecycle/test/test_cli.py

+
+    @launch_testing.markers.retry_on_failure(times=5)
+    def test_count_all_lifecycle_nodes(self):
+        with self.launch_lifecycle_command(arguments=['nodes', '-a', '-c']) as lifecycle_command:


Add a test counting without the -a flag checking that it doesn't count the hidden node

Added in fc5977c

BMarchi · 2019-11-11T11:43:17Z

ros2lifecycle/test/test_cli.py

+    @launch_testing.markers.retry_on_failure(times=5)
+    def test_set_lifecycle_node_invalid_transition(self):
+        with self.launch_lifecycle_command(
+            arguments=['set', '/lc_node', 'noop']


Add another test by checking a valid set without the hidden flag with the hidden node

Added in fc5977c.

BMarchi · 2019-11-11T11:51:15Z

ros2lifecycle/test/test_cli.py

+        )
+
+    @launch_testing.markers.retry_on_failure(times=5)
+    def test_get_nonexistent_node_state(self):


There should be a get without the hidden flag checking for a hidden node test

Also, there's no get test valid. It's coupled with the test_node_lifecycle

Added in fc5977c. It's a bit annoying to not have proper fixture scoping.

BMarchi

There are some tests that need to be added

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

…kage. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2019-11-11T14:54:26Z

@BMarchi thank you for your review! PTAL and sorry for the rebase, I had to pull in CLI fixes.

hidmic · 2019-11-11T14:55:12Z

CI up to ros2lifecycle:

Linux
Linux-aarch64
macOS
Windows

BMarchi · 2019-11-11T15:18:28Z

ros2lifecycle/package.xml

@@ -20,7 +20,10 @@
  <test_depend>ament_flake8</test_depend>
  <test_depend>ament_pep257</test_depend>
  <test_depend>ament_xmllint</test_depend>
+


Any particular reason for this newline?

Not really, fixed in eb2f8d3

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2019-11-11T20:30:14Z

@Karsten1987 want to review? Otherwise, I'll merge

mjcarroll

More tests! 🎊

Karsten1987

just a nit, but looks good to me.

Karsten1987 · 2019-11-11T21:15:52Z

ros2lifecycle_test_fixtures/src/simple_lifecycle_node.cpp

+
+  rclcpp::init(argc, argv);
+
+  auto lc_node = std::make_shared<SimpleLifecycleNode>("lc_node");


Might be worth renaming the node to something more specific (and different from the lifecycle talker exec in the lifecycle demo package).

I use "simple_lc_node" on launch, does that sound reasonable? Or would you rather be more specific?

that's okay I guess. I personally like to prefix them with test or the name of the test package in this case, but either way is fine.

See 046d82c

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2019-11-12T16:46:24Z

One last sanity CI run up to ros2lifecycle:

Linux

hidmic · 2019-11-12T19:13:05Z

Alright, going in!

* Add test coverage for ros2lifecycle CLI. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Add ros2lifecycle_test_fixtures package. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Use ros2lifecycle_test_fixtures package instead of demo lifecycle package. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Add a few more ros2lifecycle test cases. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Address last few peer review comments. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Rename ros2lifecycle tests fixture node. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic changed the title ~~[DO NOT MERGE] Add test coverage for ros2lifecycle CLI~~ Add test coverage for ros2lifecycle CLI Nov 5, 2019

hidmic requested review from dirk-thomas, BMarchi and Karsten1987 November 5, 2019 20:52

hidmic changed the title ~~Add test coverage for ros2lifecycle CLI~~ [ros2lifecycle] Add test coverage for CLI Nov 6, 2019

BMarchi reviewed Nov 11, 2019

View reviewed changes

BMarchi suggested changes Nov 11, 2019

View reviewed changes

hidmic mentioned this pull request Nov 11, 2019

[ros2lifecycle] Misc fixes #395

Merged

hidmic added 4 commits November 11, 2019 11:34

Add test coverage for ros2lifecycle CLI.

baca022

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Add ros2lifecycle_test_fixtures package.

05c4e4e

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Use ros2lifecycle_test_fixtures package instead of demo lifecycle pac…

fd3badd

…kage. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Add a few more ros2lifecycle test cases.

fc5977c

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the hidmic/ros2lifecycle-tests branch from 67ea30a to fc5977c Compare November 11, 2019 14:52

BMarchi reviewed Nov 11, 2019

View reviewed changes

Address last few peer review comments.

eb2f8d3

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

BMarchi approved these changes Nov 11, 2019

View reviewed changes

mjcarroll approved these changes Nov 11, 2019

View reviewed changes

Karsten1987 approved these changes Nov 11, 2019

View reviewed changes

Rename ros2lifecycle tests fixture node.

046d82c

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Karsten1987 approved these changes Nov 11, 2019

View reviewed changes

hidmic merged commit 40a7d99 into master Nov 12, 2019

delete-merged-branch bot deleted the hidmic/ros2lifecycle-tests branch November 12, 2019 19:13

hidmic mentioned this pull request Nov 19, 2019

Release Testing Automation - Iteration #1 ros2/ros2#739

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ros2lifecycle] Add test coverage for CLI #391

[ros2lifecycle] Add test coverage for CLI #391

hidmic commented Nov 5, 2019 •

edited

Loading

hidmic commented Nov 5, 2019 •

edited

Loading

hidmic commented Nov 6, 2019

hidmic commented Nov 6, 2019 •

edited

Loading

BMarchi Nov 11, 2019

hidmic Nov 11, 2019 •

edited

Loading

hidmic Nov 11, 2019

hidmic Nov 11, 2019

BMarchi Nov 11, 2019

hidmic Nov 11, 2019

BMarchi Nov 11, 2019

hidmic Nov 11, 2019

BMarchi Nov 11, 2019 •

edited

Loading

hidmic Nov 11, 2019

BMarchi Nov 11, 2019

BMarchi Nov 11, 2019

hidmic Nov 11, 2019

BMarchi left a comment

hidmic commented Nov 11, 2019

hidmic commented Nov 11, 2019 •

edited

Loading

BMarchi Nov 11, 2019

hidmic Nov 11, 2019

hidmic commented Nov 11, 2019

mjcarroll left a comment

Karsten1987 left a comment

Karsten1987 Nov 11, 2019

hidmic Nov 11, 2019

Karsten1987 Nov 11, 2019

hidmic Nov 11, 2019

hidmic commented Nov 12, 2019

hidmic commented Nov 12, 2019

	def main(self, *, args): # noqa: D102
	with NodeStrategy(args) as node:
	node_names = get_node_names(
	node=node, include_hidden_nodes=args.include_hidden_nodes)

	with DirectNode(args) as node:
	if args.all:
	transitions = call_get_transition_graph(
	node=node, states={args.node_name: None})
	else:
	transitions = call_get_available_transitions(
	node=node, states={args.node_name: None})
	transitions = transitions[args.node_name]
	if isinstance(transitions, Exception):
	return 'Exception while calling service of node ' \
	"'{args.node_name}': {transitions}" \
	.format_map(locals())
	for t in transitions:
	print(
	'- {t.transition.label} [{t.transition.id}]\n'
	'\tStart: {t.start_state.label}\n'
	'\tGoal: {t.goal_state.label}'.format_map(locals()))


		rclcpp::init(argc, argv);

		auto lc_node = std::make_shared<SimpleLifecycleNode>("lc_node");

[ros2lifecycle] Add test coverage for CLI #391

[ros2lifecycle] Add test coverage for CLI #391

Conversation

hidmic commented Nov 5, 2019 • edited Loading

hidmic commented Nov 5, 2019 • edited Loading

hidmic commented Nov 6, 2019

hidmic commented Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

hidmic Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMarchi Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMarchi left a comment

Choose a reason for hiding this comment

hidmic commented Nov 11, 2019

hidmic commented Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Nov 11, 2019

mjcarroll left a comment

Choose a reason for hiding this comment

Karsten1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Nov 12, 2019

hidmic commented Nov 12, 2019

hidmic commented Nov 5, 2019 •

edited

Loading

hidmic commented Nov 5, 2019 •

edited

Loading

hidmic commented Nov 6, 2019 •

edited

Loading

hidmic Nov 11, 2019 •

edited

Loading

BMarchi Nov 11, 2019 •

edited

Loading

hidmic commented Nov 11, 2019 •

edited

Loading