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.

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

add Fibonacci test for actions #316

Merged
merged 5 commits into from
Dec 7, 2018
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Dec 5, 2018

Requires ros2/rclcpp#593
Requires ros2/rclcpp#594

@wjwwood wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Dec 5, 2018
@wjwwood wjwwood self-assigned this Dec 5, 2018
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood force-pushed the wjwwood/actions_test_communication branch from a1e02ff to 013f1fe Compare December 5, 2018 22:53
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 5, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Dec 5, 2018

This is ready for review.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Two small comments. Will the test generators output more than one element eventually? If not, why an std::vector?


{
ActionClientTest<test_msgs::action::Fibonacci> test;
size_t order = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

@wjwwood hiding above's variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that was intentional, the idea being each scoped section would make their own order, but I guess I could use different names here.

}
return true;
};
result.push_back(test);
Copy link
Contributor

Choose a reason for hiding this comment

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

@wjwwood only one test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@wjwwood
Copy link
Member Author

wjwwood commented Dec 6, 2018

Will the test generators output more than one element eventually?

That's the plan. I based this on the tests from services, which have a "add two int" test and a more complete one that tests all IDL types in a single service call. I didn't do that yet for actions because of the time and because it's somewhat redundant given that the tests for topics and services already test a complex message like that and actions derive from services and topics.

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM with green CI and a small cmake fix

set_tests_properties(
test_action_client_server${test_suffix}
PROPERTIES DEPENDS
"test_requester_cpp__${rmw_implementation1};test_replier_cpp__${rmw_implementation2}"
Copy link
Contributor

Choose a reason for hiding this comment

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

"test_action_client_cpp__${rmw_implementation1};test_action_server_cpp__${rmw_implementation2}" I think

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

lgtm thanks!

// on feedback, check the feedback is valid
auto feedback_callback =
[&](auto, const auto & feedback) {
RCLCPP_INFO(logger, "received feedback");
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the test pass if no feedback is received? I guess it has to, no way to make sure feedback is received before the result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't asserting that at all. I just wanted to make sure that if feedback is sent it is valid. I could keep a counter, but I'm not sure if all the feedback would be received before the result or not, and if not how I could wait for all the feedback even after the result.

I'd consider this a good improvement on the test if we could find a good way to do it.

[logger](auto /* goal_handle */) {
RCLCPP_ERROR(logger, "received unexpected request to cancel goal");
return rclcpp_action::CancelResponse::ACCEPT;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

should_fail = true if cancel request received?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably.

@hidmic
Copy link
Contributor

hidmic commented Dec 6, 2018

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

@sloretz
Copy link
Contributor

sloretz commented Dec 7, 2018

CI with fix for test failure when using connext

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

@sloretz
Copy link
Contributor

sloretz commented Dec 7, 2018

CI post ros2/rosidl#338

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

@sloretz
Copy link
Contributor

sloretz commented Dec 7, 2018

Rebuilding just windows to check if warning is gone Build Status

@sloretz sloretz merged commit 9147079 into master Dec 7, 2018
@sloretz sloretz deleted the wjwwood/actions_test_communication branch December 7, 2018 02:57
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Dec 7, 2018
@sloretz sloretz mentioned this pull request Jan 10, 2019
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants