Make use of ros_testing to test policy generation. #214

hidmic · 2020-05-14T21:03:00Z

This pull request combines #168 and #212 to increase test coverage and reduces test flakiness for sros2 generate_policy verb.

codecov · 2020-05-14T21:37:53Z

Codecov Report

Merging #214 into master will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   77.95%   78.12%   +0.17%     
==========================================
  Files          16       16              
  Lines         576      576              
  Branches       52       52              
==========================================
+ Hits          449      450       +1     
+ Misses        107      106       -1     
  Partials       20       20

Flag	Coverage Δ
#unittests	`78.12% <ø> (+0.17%)`	⬆️

Impacted Files	Coverage Δ
sros2/sros2/__init__.py	`77.77% <0.00%> (+11.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef62301...b2a8bcb. Read the comment docs.

hidmic · 2020-05-15T13:36:23Z

Both test_nightly CI jobs are failing because librmw_connext_cpp.so cannot be found. I can only guess why the focal PR job failed, it seems like discovery latency to me. Bumped spin times from 1.0 to 2.0 seconds.

mikaelarguedas · 2020-05-15T16:24:16Z

Both test_nightly CI jobs are failing because librmw_connext_cpp.so cannot be found.

Noticed this too and confirmed that using an image with connext installed will fix the failing test: https://github.com/ros2/sros2/runs/677227888.

mikaelarguedas

that's great improvement thanks!

Looks great, left a couple nitpick comments inline

sros2/test/sros2/commands/security/verbs/test_generate_policy.py

mikaelarguedas · 2020-05-15T16:34:01Z

sros2/test/sros2/commands/security/verbs/test_generate_policy_no_nodes.py

@@ -0,0 +1,63 @@
+# Copyright 2019 Canonical Ltd
+# Copyright 2020 Open Source Robotics Foundation, Inc.


File has multiple copyright statements but no history

That's a very good point. It is work derived from a source file that was originally developed by Canonical. Do tell how we should proceed with this.

Should it actually share git history with another file? Or is it more of a "I copied that file and turned it into something else" situation?

It's a modified copy of test_generate_policy.py.

sros2/test/sros2/commands/security/verbs/utilities/sros2_cli_test_case.py

sros2/package.xml

sros2/test/sros2/commands/security/verbs/test_generate_policy.py

kyrofa · 2020-05-15T16:50:39Z

sros2/test/sros2/commands/security/verbs/fixtures/client_service_node.py

+import test_msgs.srv
+
+
+class ClientServiceNode(Node):


Can this (and other similar constructs) be turned into an actual pytest fixture on a conftest.py somewhere?

Short answer is no, because test_generate_policy*.py is no longer a pytest, but a pytest-run launch test.

kyrofa · 2020-05-15T16:55:17Z

sros2/test/sros2/commands/security/verbs/utilities/sros2_cli_test_case.py

+    ])
+
+
+class SROS2CLITestCase(unittest.TestCase):


The existing tests in this project are all pytest. Why are we using unittest here?

Because test_generate_policy*.py is no longer a pytest, but a pytest-run launch test. And launch_testing is built on top of unittest. There's a desire to achieve a stronger integration between launch_testing and pytest (even turning the former into a plugin for the latter, see ros2/launch#405), but as of today, we can only include launch tests in pytest runs.

Alright, thank you for explaining. I really dislike mixing the two, here. It's complicated enough having both of them in existence and sharing space in my head-- having them both used in the same project will cause no end of confusion. Is there any other way to do this?

There's not, assuming that testing against multiple RMW implementations is a requirement (I understand it is based on #212 (comment)) and that we're not looking forward to recreate launch and launch_testing using subprocess and pytest just for this one test. I'd rather not do that.

hidmic · 2020-05-19T15:35:48Z

CI up to sros2 and test_security:

Linux
Linux-aarch64
macOS (unrelated test failures, see test_security tests failing with Connext on OS X build_farmer#269)
Windows

hidmic · 2020-05-29T20:38:24Z

I'm not happy with 3e1bc39, for a few different reasons. But I'm unable to reproduce Windows CI failures in a Windows VM. CC @jacobperron

Signed-off-by: Brian Ezequiel Marchi <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]>

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-06-01T16:27:52Z

Alright, I corrected the commit history to pass DCO and make sense. CI's green, though tests are being skipped on Windows. FYI @mikaelarguedas @kyrofa. @jacobperron how shall we proceed?

mikaelarguedas · 2020-06-03T20:48:37Z

Regarding the move away from pytest and mixing unittest and pytest in the same package, I'm not a fan but it wouldn't be a blocker for me.

However, the fact that this seems to have pretty consistently failed CI on this repo makes me doubt this is having the intended effect of reducing flakeyness. So IMO it can't be merged as is.

I was hoping that the flakeyness was due to adding connext into the mix but several failing tests seem to be related to fastrtps(static or dynamic) so it connext doesn't seem to be the culprit.

We could scale back this to just provide a single RMW with daemon and "long" discovery time. if that proves to reduce flakeyness I'm in favor of merging that as it'll keep covering the cases users will face when using the CLI while improving CI stability.

hidmic · 2020-06-03T21:18:39Z

I have to circle back and see what's happening with Fast-RTPS now. In all cases, however, the issue (it seems) remains to be discovery latency. Testing with and without a daemon and against multiple RMW implementations only made it more visible.

I agree that to merge as-is isn't viable. I'm not sure I'd test less either just to get jobs to pass. Maybe we need to start ensuring preconditions (in this case, ROS graph state) hold instead of hoping they will within an arbitrary timeout.

This reverts commit c68f0a0. Signed-off-by: Michel Hidalgo <[email protected]>

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-06-10T17:37:10Z

One last try with 1a15522.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-06-10T22:32:43Z

Let's see how this fares on Windows:

Windows

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-06-11T20:12:43Z

Had to bump up the timeout for ros2 security generate_policy execution. I've seen it taking ~13 seconds on a pretty much idle Windows Server VM.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-06-11T21:45:41Z

Ok, b2a8bcb is almost insane. Those that timeout are the ones that use the daemon, and so they have the added overhead of XMLRPC calls. Still, it's ridiculous.

hidmic · 2020-06-12T13:41:34Z

CI up to sros2 and test_security:

Linux
Linux-aarch64
macOS
Windows

@mikaelarguedas @kyrofa PTAL, I think I've managed to remove most flakiness sources. I'm a bit baffled though by the time it takes the verb to complete in Windows.

mikaelarguedas · 2020-06-15T20:43:43Z

Ok, b2a8bcb is almost insane. Those that timeout are the ones that use the daemon, and so they have the added overhead of XMLRPC calls. Still, it's ridiculous.

This seems indeed incredible that it takes more than 20seconds to get the accurate state of a graph with 1 node and one Publisher/Subscriber pair... Is the same happening for other tests doing similar things ? or is there something wrong with the way testing is done here ?

I think I've managed to remove most flakiness sources.

As far as CI goes on this repo the latest iterations of this PR seem to yield only green jobs which is a big improvement compared to last review 👍

hidmic · 2020-06-15T20:51:28Z

Is the same happening for other tests doing similar things ? or is there something wrong with the way testing is done here ?

I think almost all CLI tests are disabled on Windows due to ros2/build_farmer#248.

At any rate, as I mentioned in #214 (comment), I do see ros2 security generate_policy taking 10-15 seconds on a Windows VM by calling it manually. Not sure where those extra 10 seconds come from in CI

mikaelarguedas · 2020-06-15T21:07:09Z

I do see ros2 security generate_policy taking 10-15 seconds on a Windows VM by calling it manually

I missed that bit. This seems very long too.. On linux its more on the second scale.

I'm +1 for this change as it increases use cases covered, does not degrade CI here and (IIUC) decreases CI flakyness on ci.ros2.org.

I'll leave it up to @kyrofa for the second review

hidmic · 2020-06-18T13:56:23Z

@kyrofa friendly ping.

kyrofa

Alright, while this makes me a bit grumpy (in the nicest possible sense 😛), I definitely see the benefit and understand that there's not a lot we can do about the unittest usage. Thank you for this, @hidmic. We can try to clarify test frameworks in a contribution guide for new devs in a follow-up.

* Make use of ros_testing to test policy generation. Signed-off-by: Brian Ezequiel Marchi <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]> * Skip test_generate_policy.py tests on Windows. Signed-off-by: Michel Hidalgo <[email protected]> * Revert "Skip test_generate_policy.py tests on Windows." This reverts commit c68f0a0. Signed-off-by: Michel Hidalgo <[email protected]> * Wait for expected ROS graph state. Signed-off-by: Michel Hidalgo <[email protected]> * Boost discovery delay. Signed-off-by: Michel Hidalgo <[email protected]> * Please linter. Signed-off-by: Michel Hidalgo <[email protected]> * Skip tests using RTI Connext w/o a daemon. Signed-off-by: Michel Hidalgo <[email protected]> * Bump generate_policy execution timeout on Windows. Signed-off-by: Michel Hidalgo <[email protected]> * Bump generate_policy execution timeout on Windows again. Signed-off-by: Michel Hidalgo <[email protected]> Co-authored-by: Brian Ezequiel Marchi <[email protected]>

* Make use of ros_testing to test policy generation. Signed-off-by: Brian Ezequiel Marchi <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]> * Skip test_generate_policy.py tests on Windows. Signed-off-by: Michel Hidalgo <[email protected]> * Revert "Skip test_generate_policy.py tests on Windows." This reverts commit c68f0a0. Signed-off-by: Michel Hidalgo <[email protected]> * Wait for expected ROS graph state. Signed-off-by: Michel Hidalgo <[email protected]> * Boost discovery delay. Signed-off-by: Michel Hidalgo <[email protected]> * Please linter. Signed-off-by: Michel Hidalgo <[email protected]> * Skip tests using RTI Connext w/o a daemon. Signed-off-by: Michel Hidalgo <[email protected]> * Bump generate_policy execution timeout on Windows. Signed-off-by: Michel Hidalgo <[email protected]> * Bump generate_policy execution timeout on Windows again. Signed-off-by: Michel Hidalgo <[email protected]> Co-authored-by: Brian Ezequiel Marchi <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]> Co-authored-by: Brian Ezequiel Marchi <[email protected]>

hidmic added the enhancement New feature or request label May 14, 2020

hidmic requested a review from mikaelarguedas May 14, 2020 21:03

This was referenced May 14, 2020

Add delay after node creation to ensure discovery for other rmw implementations #168

Closed

Do not use ros2cli daemon in generate_policy tests. #212

Closed

hidmic requested a review from dirk-thomas May 15, 2020 13:56

mikaelarguedas mentioned this pull request May 15, 2020

use nightly image including all 3 RMWs #215

Merged

mikaelarguedas reviewed May 15, 2020

View reviewed changes

kyrofa reviewed May 15, 2020

View reviewed changes

hidmic requested review from mikaelarguedas and kyrofa May 19, 2020 15:33

BMarchi and others added 2 commits June 1, 2020 13:26

Make use of ros_testing to test policy generation.

1d62bb3

Signed-off-by: Brian Ezequiel Marchi <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]>

Skip test_generate_policy.py tests on Windows.

c68f0a0

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic force-pushed the hidmic/improve-sros2-generate-policy-tests branch from 09dcdf9 to c68f0a0 Compare June 1, 2020 16:26

hidmic added 3 commits June 10, 2020 13:29

Revert "Skip test_generate_policy.py tests on Windows."

02dd9cb

This reverts commit c68f0a0. Signed-off-by: Michel Hidalgo <[email protected]>

Wait for expected ROS graph state.

0ebd444

Signed-off-by: Michel Hidalgo <[email protected]>

Boost discovery delay.

1a15522

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic added 2 commits June 10, 2020 19:21

Please linter.

70c9bec

Signed-off-by: Michel Hidalgo <[email protected]>

Skip tests using RTI Connext w/o a daemon.

4a39ffb

Signed-off-by: Michel Hidalgo <[email protected]>

Bump generate_policy execution timeout on Windows.

1b3dc82

Signed-off-by: Michel Hidalgo <[email protected]>

Bump generate_policy execution timeout on Windows again.

b2a8bcb

Signed-off-by: Michel Hidalgo <[email protected]>

sloretz mentioned this pull request Jun 11, 2020

Regression on nightly builds sros2.src.ros2.sros2.sros2.test.sros2.commands.security.verbs.test_generate_policy.test_generate_policy_services #213

Closed

dirk-thomas removed their request for review June 15, 2020 05:56

kyrofa approved these changes Jun 18, 2020

View reviewed changes

kyrofa merged commit cd234c1 into master Jun 21, 2020

kyrofa deleted the hidmic/improve-sros2-generate-policy-tests branch June 21, 2020 23:01

mikaelarguedas mentioned this pull request Feb 28, 2021

👨‍🌾 test_generate_policy and test_generate_service flaky on macOS on foxy #251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make use of ros_testing to test policy generation. #214

Make use of ros_testing to test policy generation. #214

hidmic commented May 14, 2020

codecov bot commented May 14, 2020 •

edited

Loading

hidmic commented May 15, 2020

mikaelarguedas commented May 15, 2020

mikaelarguedas left a comment

mikaelarguedas May 15, 2020

hidmic May 15, 2020

kyrofa May 15, 2020

hidmic May 15, 2020

kyrofa May 15, 2020 •

edited

Loading

hidmic May 15, 2020

kyrofa May 15, 2020

hidmic May 15, 2020

kyrofa May 15, 2020

hidmic May 15, 2020

hidmic commented May 19, 2020 •

edited

Loading

hidmic commented May 29, 2020

hidmic commented Jun 1, 2020

mikaelarguedas commented Jun 3, 2020

hidmic commented Jun 3, 2020

hidmic commented Jun 10, 2020

hidmic commented Jun 10, 2020 •

edited

Loading

hidmic commented Jun 11, 2020

hidmic commented Jun 11, 2020

hidmic commented Jun 12, 2020

mikaelarguedas commented Jun 15, 2020

hidmic commented Jun 15, 2020 •

edited

Loading

mikaelarguedas commented Jun 15, 2020

hidmic commented Jun 18, 2020

kyrofa left a comment

		@@ -0,0 +1,63 @@
		# Copyright 2019 Canonical Ltd
		# Copyright 2020 Open Source Robotics Foundation, Inc.

		import test_msgs.srv


		class ClientServiceNode(Node):

		])


		class SROS2CLITestCase(unittest.TestCase):

Make use of ros_testing to test policy generation. #214

Make use of ros_testing to test policy generation. #214

Conversation

hidmic commented May 14, 2020

codecov bot commented May 14, 2020 • edited Loading

Codecov Report

hidmic commented May 15, 2020

mikaelarguedas commented May 15, 2020

mikaelarguedas 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

kyrofa May 15, 2020 • 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

hidmic commented May 19, 2020 • edited Loading

hidmic commented May 29, 2020

hidmic commented Jun 1, 2020

mikaelarguedas commented Jun 3, 2020

hidmic commented Jun 3, 2020

hidmic commented Jun 10, 2020

hidmic commented Jun 10, 2020 • edited Loading

hidmic commented Jun 11, 2020

hidmic commented Jun 11, 2020

hidmic commented Jun 12, 2020

mikaelarguedas commented Jun 15, 2020

hidmic commented Jun 15, 2020 • edited Loading

mikaelarguedas commented Jun 15, 2020

hidmic commented Jun 18, 2020

kyrofa left a comment

Choose a reason for hiding this comment

codecov bot commented May 14, 2020 •

edited

Loading

kyrofa May 15, 2020 •

edited

Loading

hidmic commented May 19, 2020 •

edited

Loading

hidmic commented Jun 10, 2020 •

edited

Loading

hidmic commented Jun 15, 2020 •

edited

Loading