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

Completes integration tests for action client/server #331

Merged

apojomovsky merged 5 commits into master from hidmic/complete-action-tests

Nov 29, 2018

Contributor

hidmic commented Nov 15, 2018 •

edited by apojomovsky

Loading

Closes #328. It builds on top of the work done for #323. At the end, this PR must included tests for:

Action server/client send/take functions (pending on Action client implementation #319 and Action server implementation #323).
Action server/client waits (pending on Add function for waiting on multiple wait sets together #330).

hidmic added the in progress label

hidmic changed the title ~~[DO NOT MERGE] Improves test_action_communication~~ [DO NOT MERGE] Completes integration tests for action client/server

hidmic changed the title ~~[DO NOT MERGE] Completes integration tests for action client/server~~ [DO NOT MERGE NOR REVIEW] Completes integration tests for action client/server

hidmic force-pushed the hidmic/complete-action-tests branch from 33bc503 to 9ab994e Compare

November 16, 2018 19:13

hidmic and others added 2 commits

November 22, 2018 11:14


          [rcl action] Augments test_action_communication test.

bde76ab


          Updated action tests to match the updated API

4ee3182

Added action cancel test

Added action result communication test

Added action status communication test

Added action feedback communication test

Fix wrong return code in action client

apojomovsky force-pushed the hidmic/complete-action-tests branch from 9ab994e to 4ee3182 Compare

November 26, 2018 22:28

apojomovsky changed the title ~~[DO NOT MERGE NOR REVIEW] Completes integration tests for action client/server~~ Completes integration tests for action client/server

hidmic commented

View reviewed changes

rcl_action/test/rcl_action/test_action_communication.cpp Outdated

+                bool is_goal_request_ready(false);
+                bool is_cancel_request_ready(false);
+                bool is_result_request_ready(false);

Contributor Author

hidmic Nov 26, 2018

@apojomovsky here and everywhere else it applies, consider using {} instead.

Contributor

apojomovsky Nov 27, 2018

done!

rcl_action/test/rcl_action/test_action_communication.cpp Outdated

+                  num_services,          // number_of_services
+                  rcl_get_default_allocator());
+                EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
+                rcl_reset_error();

Contributor Author

hidmic Nov 26, 2018

@apojomovsky considering that above's pattern of wait set initialization repeats quite often for both server and client, I believe we can push out to SetUp() and Teardown() methods.

Contributor

apojomovsky Nov 27, 2018

Good point, just did as suggested. thanks!

Alexis Pojomovsky added 2 commits

November 27, 2018 14:11


          Addressed peer review comments

d03d560


          Added checks for wait set entities

10eebe3

jacobperron self-requested a review

November 27, 2018 17:31

jacobperron reviewed

View reviewed changes

rcl_action/test/rcl_action/test_action_communication.cpp Outdated

-                ret = rcl_wait_set_fini(&wait_set);
-                EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
-                rcl_reset_error();
+                EXPECT_EQ(this->is_goal_request_ready, true) << rcl_get_error_string().str;

Member

jacobperron Nov 27, 2018

Also, expect cancel and result requests are false.

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

    
                  &this->is_status_ready,

                  &this->is_goal_response_ready,

                  &this->is_cancel_response_ready,

                  &this->is_result_response_ready);

                EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;

Member

jacobperron Nov 27, 2018

Also:

EXPECT_FALSE(this->is_feedback_ready);
EXPECT_FALSE(this->is_status_ready);
EXPECT_TRUE(this->is_goal_response_ready);
EXPECT_FALSE(this->is_cancel_response_ready);
EXPECT_FALSE(this->is_result_response_ready);

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp Outdated

               }
-              TEST_F(CLASSNAME(TestActionCommunication, RMW_IMPLEMENTATION), test_valid_cancel_comm)
-              {
+              TEST_F(CLASSNAME(TestActionCommunication, RMW_IMPLEMENTATION), test_valid_cancel_comm) {

Member

jacobperron Nov 27, 2018

style nit: I think the brace should go on the next line, following the style guide for functions.

Contributor

apojomovsky Nov 27, 2018

Oops, got confused about that. done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

-                ret = rcl_wait_set_fini(&wait_set);
+                  &this->is_goal_request_ready,
+                  &this->is_cancel_request_ready,
+                  &this->is_result_request_ready);
                 EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;

Member

jacobperron Nov 27, 2018

Also,

EXPECT_FALSE(this->is_goal_request_ready);
EXPECT_TRUE(this->is_cancel_request_ready);
EXPECT_FALSE(this->is_result_request_ready);

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

+                  &this->is_status_ready,
+                  &this->is_goal_response_ready,
+                  &this->is_cancel_response_ready,
+                  &this->is_result_response_ready);
                 EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;

Member

jacobperron Nov 27, 2018

Also,

EXPECT_FALSE(this->is_feedback_ready);
EXPECT_FALSE(this->is_status_ready);
EXPECT_FALSE(this->is_goal_response_ready);
EXPECT_TRUE(this->is_cancel_response_ready);
EXPECT_FALSE(this->is_result_response_ready);

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

                 EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
                 rcl_reset_error();
                 ret = rcl_action_server_wait_set_get_entities_ready(
-                  &wait_set,
+                  &this->wait_set_server,
                   &this->action_server,
                   &is_goal_request_ready,
                   &is_cancel_request_ready,
                   &is_result_request_ready);
                 EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;

Member

jacobperron Nov 27, 2018

Check ready flags for expected values.

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

+                  &this->is_status_ready,
+                  &this->is_goal_response_ready,
+                  &this->is_cancel_response_ready,
+                  &this->is_result_response_ready);

Member

jacobperron Nov 27, 2018

Check ready flags for expected values.

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

+                  &this->is_status_ready,
+                  &this->is_goal_response_ready,
+                  &this->is_cancel_response_ready,
+                  &this->is_result_response_ready);

Member

jacobperron Nov 27, 2018

ditto

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

+                  &this->is_status_ready,
+                  &this->is_goal_response_ready,
+                  &this->is_cancel_response_ready,
+                  &this->is_result_response_ready);

Member

jacobperron Nov 27, 2018

ditto

Contributor

apojomovsky Nov 27, 2018

done, thanks!

rcl_action/test/rcl_action/test_action_communication.cpp

+                    &this->num_clients_client,
+                    &this->num_services_client);
+                  ASSERT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
+                  ret = rcl_wait_set_init(

Member

jacobperron Nov 27, 2018

Alternatively, the action wait set functions should support multiple clients/servers per wait set. i.e. we can put the action client and server into a single wait set. I'm fine leaving these tests using two wait sets though, I'll leave it up to you.

Contributor

apojomovsky Nov 27, 2018

That's a good detail, I thought I needed individual wait sets here. Using a single one looks a lot cleaner, will simplify that now.

Contributor

apojomovsky Nov 28, 2018

done!


          Addressed peer review comments v2

aead129

apojomovsky force-pushed the hidmic/complete-action-tests branch from f21c3d8 to aead129 Compare

November 28, 2018 18:08

Contributor

apojomovsky commented Nov 28, 2018

Thanks for the review guys, just finished addressing your comments.
I'm working on tests to exercise the examples found here now.

jacobperron approved these changes

View reviewed changes

Member

jacobperron left a comment

LGTM pending green CI

Contributor

apojomovsky commented Nov 29, 2018 •

edited

Loading

CI:

Linux:
Linux aarch64:
MacOS:
Windows:

Contributor

apojomovsky commented Nov 29, 2018

CI is passing so I'll merge this one and send a separate PR with the tests that exercise the examples

apojomovsky merged commit dfaa412 into master

tfoote removed the in progress label

Member

dirk-thomas commented Nov 30, 2018

The new test failed on one of the last nightly jobs: https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_debug/651/testReport/

Also do you still need the branch or can it be deleted?

Contributor

apojomovsky commented Nov 30, 2018

Sorry for that @dirk-thomas , this seems to be related to using tight timeouts in the wait_sets, will send a fix for that shortly.
Regarding this branch, I'll take care of removing it after the fix gets merged.

apojomovsky mentioned this pull request

Fix action tests failing in CI #344

Merged

Member

dirk-thomas commented Nov 30, 2018

Please update these tests to actually check if they work for each rmw implementation. Currently they only test the default and won't allow is to catch regressions in any other typesupport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet