Increase test coverage and add more safety checks

Signed-off-by: Stephen Brawner <brawner@gmail.com>
ros2 · May 14, 2020 · 44dbbed · 44dbbed
1 parent d321751
commit 44dbbed
Show file tree

Hide file tree

Showing 7 changed files with 480 additions and 4 deletions.
diff --git a/rcl_lifecycle/CMakeLists.txt b/rcl_lifecycle/CMakeLists.txt
@@ -89,6 +89,16 @@ if(BUILD_TESTING)
     )
     target_link_libraries(test_multiple_instances ${PROJECT_NAME})
   endif()
+  ament_add_gtest(test_rcl_lifecycle
+    test/test_rcl_lifecycle.cpp
+  )
+  if(TARGET test_rcl_lifecycle)
+    ament_target_dependencies(test_rcl_lifecycle
+      "rcl"
+      "osrf_testing_tools_cpp"
+    )
+    target_link_libraries(test_rcl_lifecycle ${PROJECT_NAME})
+  endif()
   ament_add_gtest(test_transition_map
     test/test_transition_map.cpp
   )

diff --git a/rcl_lifecycle/src/default_state_machine.c b/rcl_lifecycle/src/default_state_machine.c
@@ -650,7 +650,7 @@ _register_transitions(
   return ret;
 }
 
-// default implementation as despicted on
+// default implementation as depicted on
 // design.ros2.org
 rcl_ret_t
 rcl_lifecycle_init_default_state_machine(

diff --git a/rcl_lifecycle/src/rcl_lifecycle.c b/rcl_lifecycle/src/rcl_lifecycle.c
@@ -40,6 +40,8 @@ rcl_lifecycle_get_zero_initialized_state()
   rcl_lifecycle_state_t state;
   state.id = 0;
   state.label = NULL;
+  state.valid_transitions = NULL;
+  state.valid_transition_size = 0;
   return state;
 }
 
@@ -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;
+  }
 
   state->id = id;
   state->label = rcutils_strndup(label, strlen(label), *allocator);
@@ -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;
+  }
+
+  if (!label) {
+    RCL_SET_ERROR_MSG("label pointer is null\n");
+    return RCL_RET_ERROR;
+  }
+
+  if (!start) {
+    RCL_SET_ERROR_MSG("start state pointer is null\n");
+    return RCL_RET_ERROR;
+  }
+
+  if (!goal) {
+    RCL_SET_ERROR_MSG("goal state pointer is null\n");
+    return RCL_RET_ERROR;
   }
 
   transition->start = start;
@@ -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;
   }
   // it is already NULL
@@ -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;
+  }
+  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;

diff --git a/rcl_lifecycle/src/transition_map.c b/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;
+  }
+
   rcl_ret_t fcn_ret = RCL_RET_OK;
 
   // free valid transitions for all states
@@ -117,6 +122,11 @@ rcl_lifecycle_register_transition(
     return RCL_RET_ERROR;
   }
 
+  rcl_lifecycle_state_t * goal = rcl_lifecycle_get_state(transition_map, transition.goal->id);
+  if (!goal) {
+    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("state %u is not registered\n", transition.goal->id);
+    return RCL_RET_ERROR;
+  }
   // we add a new transition, so increase the size
   transition_map->transitions_size += 1;
   rcl_lifecycle_transition_t * new_transitions = allocator->reallocate(

diff --git a/rcl_lifecycle/test/test_default_state_machine.cpp b/rcl_lifecycle/test/test_default_state_machine.cpp
@@ -113,7 +113,11 @@ TEST_F(TestDefaultStateMachine, zero_init) {
 TEST_F(TestDefaultStateMachine, default_init) {
   rcl_lifecycle_state_machine_t state_machine = rcl_lifecycle_get_zero_initialized_state_machine();
 
-  auto ret = rcl_lifecycle_init_default_state_machine(&state_machine, this->allocator);
+  auto ret = rcl_lifecycle_init_default_state_machine(&state_machine, nullptr);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcutils_reset_error();
+
+  ret = rcl_lifecycle_init_default_state_machine(&state_machine, this->allocator);
   EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 
   ret = rcl_lifecycle_state_machine_fini(&state_machine, this->node_ptr, this->allocator);