Run QoS tests. #441

hidmic · 2020-07-08T15:54:46Z

As a result of the investigation for ros2/rmw_fastrtps#384, it was apparent that test_quality_of_service tests were actually not being run. This patch corrects the situation.

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

hidmic · 2020-07-08T15:56:23Z

Several tests fail on master for different RMW implementations. We may have to delay merging this PR until that is resolved, considering ongoing efforts to go green. FYI @dirk-thomas.

dirk-thomas · 2020-07-08T16:01:15Z

@dabonnie FYI since you added this package in #347.

clalancette · 2020-07-08T16:05:49Z

Several tests fail on master for different RMW implementations. We may have to delay merging this PR until that is resolved, considering ongoing efforts to go green. FYI @dirk-thomas.

One suggestion; enable the tests that actually pass right now, then do a follow-up issue/PR to fix and enable the ones that don't. That way we at least will get some of the test coverage here.

dabonnie · 2020-07-08T17:01:42Z

@dabonnie FYI since you added this package in #347.

Thanks for the visibility, I'll follow up.

dabonnie · 2020-07-09T17:16:47Z

Several tests fail on master for different RMW implementations. We may have to delay merging this PR until that is resolved, considering ongoing efforts to go green. FYI @dirk-thomas.

Do you happen to have a link to the failures?

hidmic · 2020-07-13T17:51:30Z

@dabonnie I've only reproduced locally. Here's a CI run for reference:

Linux

I wonder why there's no Rolling PR job though. @nuclearsandwich ?

jacobperron · 2020-07-13T18:24:05Z

I wonder why there's no Rolling PR job though.

The job is not enabled in the rosdistro distribution.yaml. I'm not sure if it can be enabled without a binary job though.

dabonnie · 2020-07-13T18:58:09Z

I wonder why there's no Rolling PR job though.

The job is not enabled in the rosdistro distribution.yaml. I'm not sure if it can be enabled without a binary job though.

I think it's worthwhile to run / test via actions-ros-ci: I'll be adding an issue / PR.

dirk-thomas · 2020-07-13T19:13:59Z

The job is not enabled in the rosdistro distribution.yaml. I'm not sure if it can be enabled without a binary job though.

A repository can have dev and PR jobs if all its dependencies are released. It doesn't have to be release itself.

I think it's worthwhile to run / test via actions-ros-ci: I'll be adding an issue / PR.

For these repositories please continue due use the Jenkins-based PR / CI builds since they provide a lot of benefits over the in-development actions atm.

dabonnie · 2020-07-14T05:40:25Z

For these repositories please continue due use the Jenkins-based PR / CI builds since they provide a lot of benefits over the in-development actions atm.

Can't we do both? What's the downside to having scheduled actions running at a 1h or 1day interval exposing these kinds of issues? It may be that I'm not aware that adding a job on Jenkins is a fairly easy task, so in that case I would agree we only need your approach.

dirk-thomas · 2020-07-14T15:26:53Z

Can't we do both?

Sure, we can have both. But not only actions since the Jenkins based jobs provide more functionality. So please also enable pull request jobs for this repository.

dirk-thomas · 2020-07-14T15:28:57Z

It may be that I'm not aware that adding a job on Jenkins is a fairly easy task, so in that case I would agree we only need your approach.

The Jenkins jobs in build.ros.org and build.ros2.org are fully automated. All it takes is seeing the key test_pull_requests in the rosdistro for this repository.

nuclearsandwich · 2020-07-14T15:36:50Z

PR to enable pull request testing for Rolling on build.ros2.org ros/rosdistro#25806

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

hidmic · 2020-07-16T17:48:34Z

Alright, based on ros2/rmw_fastrtps#384 (comment) I believe I got the tests right. @dabonnie PTAL.

dabonnie · 2020-07-16T17:53:20Z

test_quality_of_service/test/test_deadline.cpp

@@ -30,57 +30,13 @@

 using namespace std::chrono_literals;

-///// Test Deadline with a single subscriber node
-TEST_F(QosRclcppTestFixture, test_deadline_no_publisher) {


Curious why this test was removed, as it checks for spurious events.

Based on its current definition, deadline QoS establishes the maximum inter-arrival time for messages. If no message is ever published, no deadline event will be received. It is a bit counter-intuitive, but that's how it is for DDS and ROS.

hidmic · 2020-07-16T20:04:16Z

PR job's failing because FastDDS has to be re-released against rolling.

Running CI up to test_quality_of_service:

Linux
Linux-aarch64
macOS
Windows

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

To cope with longer discovery times. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

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

To cope with longer discovery times. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-07-17T20:28:46Z

Ready for another look, PTAL! I had to tweak the lifespan test a bit and increase timeouts and period for rmw_connext_cpp's ridiculous discovery latencies.

dabonnie · 2020-07-17T23:29:37Z

test_quality_of_service/test/test_deadline.cpp

@@ -33,7 +33,8 @@ using namespace std::chrono_literals;
 /// Test Deadline with a single publishing node and single subscriber node
 TEST_F(QosRclcppTestFixture, test_deadline) {
  int expected_number_of_events = 4;
-  const std::chrono::milliseconds deadline_duration = 1s;
+  const std::chrono::milliseconds deadline_duration =


nit: an explanation why this is different for connext (observed via CI, your testing, etc)

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

hidmic · 2020-07-30T14:02:39Z

I'm starting to wonder if it's even possible to write a QoS test like these that's not intrinsically flaky.

hidmic · 2020-07-30T20:59:30Z

The last few failing tests on Windows are due to Fast-RTPS hanging on subscription destruction.

@richiprosima @MiguelCompany I've tracked it down to ResourceEvent::unregister_timer during ~SubscriberImpl. Can you think of any recent changes that may explain the issue?

hidmic · 2020-08-10T13:38:56Z

@richiprosima @MiguelCompany friendly ping. Would a backtrace help you guys?

hidmic · 2020-12-22T18:40:17Z

@EduPonz @MiguelCompany ping

EduPonz · 2020-12-23T08:18:28Z

Sorry @hidmic , this flew completely under our radar. Some backtrace would indeed be most welcomed. Also, anything you think worth mentioning for replicating would be much appreciated. We did some refactoring on the ResourceEvent in eProsima/Fast-DDS#1597 (backported to 2.0.x in eProsima/Fast-DDS#1651). I do not think that it'll have any effect on this particular failure, but I'd like to try out.

Thanks!

hidmic · 2021-02-15T22:13:40Z

@ros-pull-request-builder retest this please

hidmic · 2021-02-15T22:15:41Z

It seems I can no longer reproduce. Re-running CI to confirm:

Linux
Linux-aarch64
macOS
Windows

clalancette

Seems reasonable to me and has green CI.

hidmic · 2021-02-16T13:45:46Z

Alright, let's get this in. Thanks @clalancette !

Run QoS tests.

e37b333

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

hidmic mentioned this pull request Jul 8, 2020

No callback when AUTOMATIC liveliness is lost ros2/rmw_fastrtps#384

Closed

hidmic requested review from clalancette, ivanpauno and jacobperron and removed request for ivanpauno July 8, 2020 15:59

nuclearsandwich mentioned this pull request Jul 14, 2020

Enable pull request testing for system_tests. ros/rosdistro#25806

Merged

Fix deadline QoS tests.

1810ac6

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

dabonnie approved these changes Jul 16, 2020

View reviewed changes

hidmic added 4 commits July 17, 2020 11:04

Instantiate publisher before subscriber.

9b1adff

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

Use longer deadlines for rmw_connext_cpp.

5864492

To cope with longer discovery times. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix lifespan QoS test.

6edbeb6

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

Use longer lifespans for rmw_connext_cpp.

6774584

To cope with longer discovery times. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested a review from dabonnie July 17, 2020 20:27

dabonnie approved these changes Jul 17, 2020

View reviewed changes

hidmic added 2 commits July 24, 2020 14:59

Explain different QoS durations.

9f65fb7

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

Cope with discovery noise in lifespan test.

1169fcb

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

dabonnie approved these changes Jul 24, 2020

View reviewed changes

Adjust lifespan test again to deal discovery.

d05182f

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

clalancette assigned hidmic Jan 7, 2021

clalancette approved these changes Feb 16, 2021

View reviewed changes

hidmic merged commit e443904 into master Feb 16, 2021

delete-merged-branch bot deleted the hidmic/run-qos-tests branch February 16, 2021 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run QoS tests. #441

Run QoS tests. #441

hidmic commented Jul 8, 2020

hidmic commented Jul 8, 2020

dirk-thomas commented Jul 8, 2020 •

edited

clalancette commented Jul 8, 2020

dabonnie commented Jul 8, 2020 •

edited

dabonnie commented Jul 9, 2020

hidmic commented Jul 13, 2020 •

edited

jacobperron commented Jul 13, 2020 •

edited

dabonnie commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

dabonnie commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

nuclearsandwich commented Jul 14, 2020

hidmic commented Jul 16, 2020

dabonnie Jul 16, 2020

hidmic Jul 16, 2020

hidmic commented Jul 16, 2020 •

edited

hidmic commented Jul 17, 2020

dabonnie Jul 17, 2020

hidmic commented Jul 30, 2020

hidmic commented Jul 30, 2020 •

edited

hidmic commented Aug 10, 2020

hidmic commented Dec 22, 2020

EduPonz commented Dec 23, 2020

hidmic commented Feb 15, 2021

hidmic commented Feb 15, 2021

clalancette left a comment

hidmic commented Feb 16, 2021

Run QoS tests. #441

Run QoS tests. #441

Conversation

hidmic commented Jul 8, 2020

hidmic commented Jul 8, 2020

dirk-thomas commented Jul 8, 2020 • edited

clalancette commented Jul 8, 2020

dabonnie commented Jul 8, 2020 • edited

dabonnie commented Jul 9, 2020

hidmic commented Jul 13, 2020 • edited

jacobperron commented Jul 13, 2020 • edited

dabonnie commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

dabonnie commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

nuclearsandwich commented Jul 14, 2020

hidmic commented Jul 16, 2020

dabonnie Jul 16, 2020

Choose a reason for hiding this comment

hidmic Jul 16, 2020

Choose a reason for hiding this comment

hidmic commented Jul 16, 2020 • edited

hidmic commented Jul 17, 2020

dabonnie Jul 17, 2020

Choose a reason for hiding this comment

hidmic commented Jul 30, 2020

hidmic commented Jul 30, 2020 • edited

hidmic commented Aug 10, 2020

hidmic commented Dec 22, 2020

EduPonz commented Dec 23, 2020

hidmic commented Feb 15, 2021

hidmic commented Feb 15, 2021

clalancette left a comment

Choose a reason for hiding this comment

hidmic commented Feb 16, 2021

dirk-thomas commented Jul 8, 2020 •

edited

dabonnie commented Jul 8, 2020 •

edited

hidmic commented Jul 13, 2020 •

edited

jacobperron commented Jul 13, 2020 •

edited

hidmic commented Jul 16, 2020 •

edited

hidmic commented Jul 30, 2020 •

edited