Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

QoS System Tests #347

Merged
merged 13 commits into from
May 21, 2019
Merged

QoS System Tests #347

merged 13 commits into from
May 21, 2019

Conversation

dabonnie
Copy link
Contributor

@dabonnie dabonnie commented Apr 22, 2019 •
edited
Loading

Added basic system tests for the rclcpp QoS features

  • liveliness
  • deadline
  • lifespan

Relates to ros2/design#212

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Apr 22, 2019
@dabonnie dabonnie changed the title Qos system tests QoS System Tests Apr 22, 2019
@dabonnie
Copy link
Contributor Author

@thomas-moulard - please run the following CI job:

@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

thomas-moulard
thomas-moulard reviewed Apr 22, 2019
View reviewed changes
test_quality_of_service/include/test_quality_of_service/qos_utilities.hpp Outdated Show resolved Hide resolved
test_quality_of_service/include/test_quality_of_service/qos_utilities.hpp Outdated Show resolved Hide resolved
test_quality_of_service/include/test_quality_of_service/qos_utilities.hpp Outdated Show resolved Hide resolved
test_quality_of_service/include/test_quality_of_service/qos_utilities.hpp Outdated Show resolved Hide resolved
test_quality_of_service/include/test_quality_of_service/qos_utilities.hpp Outdated Show resolved Hide resolved
test_quality_of_service/test/test_deadline.cpp Show resolved Hide resolved
test_quality_of_service/test/test_deadline.cpp Outdated Show resolved Hide resolved
test_quality_of_service/test/test_liveliness.cpp Outdated Show resolved Hide resolved
test_quality_of_service/include/test_quality_of_service/qos_utilities.hpp Show resolved Hide resolved
test_quality_of_service/CMakeLists.txt Outdated Show resolved Hide resolved
@dabonnie dabonnie mentioned this pull request Apr 24, 2019
Closed
@nburek nburek mentioned this pull request May 6, 2019
Closed
29 tasks
@mm318
Copy link
Member

mm318 commented May 6, 2019 •
edited
Loading

Can we move this from "In Progress" to "In Review"?

@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 6, 2019
@mm318 mm318 force-pushed the qos_system_tests branch 3 times, most recently from 7c7b0e4 to 3cec879 Compare May 9, 2019 17:46
@mm318 mm318 force-pushed the qos_system_tests branch from d1fc46d to fd8edc4 Compare May 13, 2019 21:26
@dirk-thomas
Copy link
Member

@thomas-moulard Please run CI for this.

@mm318 mm318 force-pushed the qos_system_tests branch from fd8edc4 to 9925d30 Compare May 16, 2019 21:51
@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
    T

@mm318
Copy link
Member

mm318 commented May 16, 2019

@thomas-moulard - please run the following CI job:

@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@nburek
Copy link

nburek commented May 20, 2019

It looks like this was failing because it was trying to find_package on pytest, which was not used and not included in the package.xml.

@thomas-moulard Could you please restart the tests with the latest commit. Thank you

@nuclearsandwich
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mm318 mm318 force-pushed the qos_system_tests branch from a8429b7 to a3e299d Compare May 20, 2019 23:28
@mm318
Copy link
Member

mm318 commented May 21, 2019

@thomas-moulard can you please re-run the CI?

@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mm318
Copy link
Member

mm318 commented May 21, 2019

@nuclearsandwich @dirk-thomas this should be ready to merge. Thanks!

@dirk-thomas
Copy link
Member

First this needs to be reviewed by someone. @wjwwood Can you maybe take a look at this?

wjwwood
wjwwood approved these changes May 21, 2019
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall lgtm.

It already has reviews from @thomas-moulard. I just had a inconsequential comment about which logger is being used.

test_quality_of_service/test/test_deadline.cpp Outdated Show resolved Hide resolved
test_quality_of_service/test/test_liveliness.cpp Outdated Show resolved Hide resolved
dabonnie and others added 13 commits May 21, 2019 14:49
@dabonnie @mm318
Added new Quality of Service system test package
f0e7aac 
Added simple test for liveliness

Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Added deadline system test
25bab25 
Added lifespan system test

Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Added linting fixes
cf39d33 
Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Added documentation
6b9cfd3 
Added minor liveliness test cleanup

Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Fix CMake warnings
0e6d1a7 
Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Added base class for test nodes
7cbd09e 
Added implementation headers and files
Addressed review comments

Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Added simple deadline test
364cb8d 
Renamed implementing classes to avoid namespace confusion

Signed-off-by: Devin Bonnie <[email protected]>
@dabonnie @mm318
Addressed review comments
50fe94e 
Fixed bugs in fixture bringup and teardown
Fixed base class issues

Signed-off-by: Devin Bonnie <[email protected]>
@mm318
fix compiler errors
aa7fb5f 
Signed-off-by: Miaofei <[email protected]>
@mm318
update to be compatible with latest rclcpp API
adac364 
Signed-off-by: Miaofei <[email protected]>
@nburek @mm318
Removes unused find_package for pytest.
3369b93 
Signed-off-by: burekn <[email protected]>
@mm318
fix windows compiler warnings
4b81dc7 
Signed-off-by: Miaofei <[email protected]>
@mm318
replace rclcpp::get_logger() with Node::get_logger()
3314b3b 
Signed-off-by: Miaofei <[email protected]>
@mm318 mm318 force-pushed the qos_system_tests branch from a3e299d to 3314b3b Compare May 21, 2019 21:50
@mm318
Copy link
Member

mm318 commented May 21, 2019

@thomas-moulard can you please re-run the CI for this PR (#347 (comment))?

@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mm318
Copy link
Member

mm318 commented May 21, 2019

Hi @dirk-thomas and @wjwwood, this should be ready to merge. The instability of the macOS build is already being tracked at ros2/build_farmer#199.

@wjwwood wjwwood merged commit d82720e into ros2:master May 21, 2019
@wjwwood
Copy link
Member

wjwwood commented May 21, 2019

I didn't see any, but just to confirm, there are not any other pr's that need to be merged with this one right?

@mm318
Copy link
Member

mm318 commented May 21, 2019

This one is a standalone PR.

Thank you!

@dirk-thomas dirk-thomas mentioned this pull request Jul 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-moulard thomas-moulard thomas-moulard left review comments

@mm318 mm318 mm318 left review comments

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@dabonnie @thomas-moulard @mm318 @dirk-thomas @nburek @nuclearsandwich @wjwwood @tfoote @sloretz