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

Increase rcl_lifecycle test coverage and add more safety checks #649

Merged

brawner merged 7 commits into master from brawner/lifecycle-tests

May 22, 2020

Contributor

brawner commented May 14, 2020

As part of the push to Quality Level 1, this adds more more tests of the rcl_lifecycle API. While writing these tests, I found a few instances where more checks were needed.

If I didn't understand parts of the API and added checks where they weren't desired, feel free to make comments below. Alternatively, if there are more thorough checks that people feel are necessary I can also add those in. These were pretty much what I found passing around bad parameters.

Linux
Linux-aarch64
macOS
Windows
Coverage


          Increase test coverage and add more safety checks

44dbbed

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner force-pushed the brawner/lifecycle-tests branch from c277cfe to 44dbbed Compare

May 14, 2020 01:02

brawner self-assigned this

Contributor Author

brawner commented May 14, 2020

It looks like there is a bit more I can do to boost coverage. I'll work on this more tomorrow.

brawner marked this pull request as draft

May 14, 2020 01:13

brawner force-pushed the brawner/lifecycle-tests branch 7 times, most recently from 479aa12 to db71eb1 Compare

May 14, 2020 23:57

Contributor Author

brawner commented May 15, 2020 •

edited

Loading

Linux
Linux-aarch64
macOS
Windows

brawner force-pushed the brawner/lifecycle-tests branch from db71eb1 to 66e036f Compare

May 15, 2020 01:08


          More coverage

70465b7

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner force-pushed the brawner/lifecycle-tests branch from 66e036f to 70465b7 Compare

May 15, 2020 01:17

brawner marked this pull request as ready for review

May 15, 2020 01:38

Contributor Author

brawner commented May 15, 2020

Much better coverage, but com_interface and default_state_machine are dragging it down.

Name ↓	Classes	Lines	Conditionals
com_interface.c	100% 1/1	77% 66/86	66/86
default_state_machine.c	100% 1/1	80% 147/184	147/184
rcl_lifecycle.c	100% 1/1	92% 167/181	167/181
transition_map.c	100% 1/1	97% 77/79	77/79

ahcorde requested changes

View reviewed changes

Contributor

ahcorde left a comment

Some of the errors are better defined as RCL_RET INVALID_ARGUMENT. If you decide to change this you should change some error in the tests.

rcl_lifecycle/src/rcl_lifecycle.c

@@ @@ -58,6 +60,10 @@ rcl_lifecycle_state_init( @@
                   RCL_SET_ERROR_MSG("state pointer is null\n");
                   return RCL_RET_ERROR;
                 }
+                if (!label) {
+                  RCL_SET_ERROR_MSG("State label is null\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

@@ @@ -118,7 +124,22 @@ rcl_lifecycle_transition_init( @@
                 if (!transition) {
                   RCL_SET_ERROR_MSG("transition pointer is null\n");
-                  return RCL_RET_OK;
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

+                if (!label) {
+                  RCL_SET_ERROR_MSG("label pointer is null\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

+                if (!start) {
+                  RCL_SET_ERROR_MSG("start state pointer is null\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

+                if (!goal) {
+                  RCL_SET_ERROR_MSG("goal state pointer is null\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

@@ @@ -140,7 +161,7 @@ rcl_lifecycle_transition_fini( @@
                 const rcl_allocator_t * allocator)
               {
                 if (!allocator) {
-                  RCL_SET_ERROR_MSG("can't initialize transition, no allocator given\n");
+                  RCL_SET_ERROR_MSG("can't finalize transition, no allocator given\n");
                   return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

@@ @@ -192,6 +213,14 @@ rcl_lifecycle_state_machine_init( @@
                 bool default_states,
                 const rcl_allocator_t * allocator)
               {
+                if (!state_machine) {
+                  RCL_SET_ERROR_MSG("State machine is null\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

+                }
+                if (!node_handle) {
+                  RCL_SET_ERROR_MSG("Node handle is null\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/rcl_lifecycle.c

+                if (!node_handle) {
+                  RCL_SET_ERROR_MSG("Node handle is null\n");
+                  return RCL_RET_ERROR;
+                }
                 if (!allocator) {
                   RCL_SET_ERROR_MSG("can't initialize state machine, no allocator given\n");
                   return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

rcl_lifecycle/src/transition_map.c

@@ @@ -53,6 +53,11 @@ rcl_lifecycle_transition_map_fini( @@
                 rcl_lifecycle_transition_map_t * transition_map,
                 const rcutils_allocator_t * allocator)
               {
+                if (!allocator) {
+                  RCL_SET_ERROR_MSG("can't free transition map, no allocator given\n");
+                  return RCL_RET_ERROR;

Contributor

ahcorde May 15, 2020

Suggested change

      
                return RCL_RET_ERROR;
          
                return RCL_RET_INVALID_ARGUMENT;

Contributor Author

brawner commented May 15, 2020

I'm not sure I can change the error codes of already existing functions this late in the game. While, I agree invalid_argument is better than error, I chose RCL_RET_ERROR to match the rest of the code.

brawner requested a review from Karsten1987

May 15, 2020 18:21

ahcorde approved these changes

View reviewed changes

Contributor

ahcorde left a comment

I added an issue to follow up with the error messages #655

Karsten1987 approved these changes

View reviewed changes

Contributor

Karsten1987 left a comment

Looks good to me besides one comment.

rcl_lifecycle/src/default_state_machine.c Outdated

@@ @@ -691,6 +691,7 @@ rcl_lifecycle_init_default_state_machine( @@
                 return ret;
               fail:
+                // if rcl_lifecycle_transition_map_fini() fails, it will clobber the error string here, twice

Contributor

Karsten1987 May 18, 2020

is that meant as a TODO here? If fini() fails and sets its own error message, I think we can either reset it here or concatenate. I think overwriting the error message actually yields a warning.

Contributor Author

brawner May 18, 2020

No it was an issue I had found, and didn't have a great way of resolving it. I didn't know concatenation would work, I'll add that in.

It will indeed print a warning if the error message is set and then it proceeds to set another error message. However, I didn't want to lose the trace of errors messages as they fall from one failure to the next (so it would have been impossible to see the original failure case).

brawner mentioned this pull request

Improve error messages in rcl_lifecycle #655

Closed


          Concatenating error messages

152aaa3

Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com>

Karsten1987 reviewed

View reviewed changes

rcl_lifecycle/src/default_state_machine.c Outdated Show resolved Hide resolved

brawner force-pushed the brawner/lifecycle-tests branch from 7a3b0b9 to 93ca2ec Compare

May 19, 2020 21:56


          PR Fixup

71607bc

Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com>

brawner force-pushed the brawner/lifecycle-tests branch from 93ca2ec to 71607bc Compare

May 19, 2020 21:58

Contributor Author

brawner commented May 19, 2020 •

edited

Loading

Checking one last time

Linux
Linux-aarch64
macOS
Windows

Karsten1987 reviewed

View reviewed changes

rcl_lifecycle/src/default_state_machine.c Outdated

@@ @@ -690,10 +690,27 @@ rcl_lifecycle_init_default_state_machine( @@
                 return ret;
-              fail:
+              // Semicolon handles the "a label can only be part of a statement..." error
+              fail:;

Contributor

Karsten1987 May 20, 2020

Suggested change

      
            fail:;
          
            fail:

Or was that semicolon intended?

Contributor Author

brawner May 22, 2020

The compiler wouldn't let me declare current_error directly after fail: without it. The semicolon effectively creates a new statement. Should I move the variable declarations to the top?

rcl_lifecycle/src/default_state_machine.c Outdated

                 if (rcl_lifecycle_transition_map_fini(&state_machine->transition_map, allocator) != RCL_RET_OK) {
-                  RCL_SET_ERROR_MSG("could not free lifecycle transition map. Leaking memory!\n");
+                  const char * fini_error = (rcl_error_is_set()) ? rcl_get_error_string().str : "";

Contributor

Karsten1987 May 20, 2020

if I interpret this correctly this line will always result in an empty string, given that you reset the error message just above.

rcl_lifecycle/src/default_state_machine.c Outdated

Comment on lines 708 to 709

		} else if (strcmp(current_error, "") != 0) {
		RCL_SET_ERROR_MSG(current_error);

Contributor

Karsten1987 May 20, 2020

Sorry to iterate again over this, but I think that else if branch is not needed. If I see this correctly, we'll fetch the error message before just to reset it again in this branch.
Why not just retrieving the existing error message within the first if branch?

something like this:

if (rcl_lifecycle_transition_map_fini(&state_machine->transition_map, allocator) != RCL_RET_OK) {
  const char * fini_error = "";
  if (rcl_error_is_set()) {
     fini_error = rcl_get_error_string().str;
     rcl_reset_error();
  }
  RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
  "Freeing transition map failed while handling a previous error. Leaking memory!"
  "\nOriginal error:\n\t%s\nError encountered in rcl_lifecycle_transition_map_fini():\n\t%s\n",
  current_error, fini_error);
}

if (!rcl_error_is_set()) {
  RCL_SET_ERROR_MSG("Unspecified error ..");
}

Contributor Author

brawner May 20, 2020

The problem originally was that the error message was already set. But if rcl_lifecycle_transition_map_fini failed, it would set its own message, and then the message would get set a third time inside this first if loop. So the goal was to keep track of current_message, and append fini_error if it existed and throw a combined error that was hopefully more helpful than either of them individually.

However, if rcl_lifecycle_transition_map_fini succeeds, we still want the user to know what caused the code to reach fail: in the first place, which is in current_message , but only if rcl_error_is_set() was true in line 697.

Originally when I submitted this, I just relied on rcutils to log the transition of error messages, since it warns if a new error message is set while a previous one had already existed. However, since I'm pretty new to the ROS 2 C codebase, I'm really not confident about any particular approach here 🤷‍♂️

Stephen Brawner added 2 commits

May 19, 2020 21:22


          Forgot a

f269c23

Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com>


          PR Fixup

a4961f1

Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com>

Contributor Author

brawner commented May 22, 2020

Address @Karsten1987's feedback and am running CI again.

Linux
Linux-aarch64
macOS
Windows


          Moving var to top

661b4b7

Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com>

brawner force-pushed the brawner/lifecycle-tests branch from be69602 to 661b4b7 Compare

May 22, 2020 19:11

Karsten1987 approved these changes

View reviewed changes

Contributor

Karsten1987 left a comment

this looks good to me now. Thanks for iterating with me over this.

brawner merged commit 7146919 into master

delete-merged-branch bot deleted the brawner/lifecycle-tests branch

May 22, 2020 21:44

Contributor Author

brawner commented May 22, 2020

Thank you for your reviews and help!

Contributor

hidmic commented May 27, 2020

@brawner @Karsten1987 this patch is causing test failures in rclcpp_lifecycle tests. See https://ci.ros2.org/view/nightly/job/nightly_linux_release/1555/. It seems these additional checks in rcl_lifecycle_transition_init() are not consistent with what rclcpp_lifecycle expects of this API.

Contributor

Karsten1987 commented May 27, 2020

Looks like a transition can be lazy initialized where start and goal state don't have to be present. Which means we should remove the nullptr checks for these two.

Karsten1987 mentioned this pull request

transition start and goal states can be null #662

Merged

brawner mentioned this pull request

Remove std::cout line from test_rcl_lifecycle.cpp #773

Merged

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