Add fault injection macro unit tests #27

brawner · 2020-07-29T01:25:24Z

Adding fault injection unit tests to rmw_dds_common. This should get coverage to about 99%. I'll post CI results soon.

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

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

brawner · 2020-08-12T23:07:55Z

Builds and tests of the following packages rcutils, rcl_logging_spdlog, rosidl_typesupport_c, rosidl_typesupport_cpp, rcl, rcl_action, rcl_lifecycle, rosidl_runtime_c, rmw_dds_common, rmw

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-08-24T19:18:49Z

Testing this PR branch against current ros 2 branches. Testing --packages-select rmw_dds_common

Linux
Linux-aarch64
macOS
Windows

hidmic · 2020-08-24T21:23:25Z

rmw_dds_common/test/test_graph_cache.cpp

@@ -1550,3 +1551,64 @@ TEST(test_graph_cache, bad_arguments)
    rcutils_reset_error();
  }
 }
+
+TEST(test_graph_cache, get_writers_info_by_topic_maybe_fail)


@brawner would it make sense to split this test case into three separate ones (i.e. one for each method being fault injected).

Yes, done! I think I originally intended for that given the test name.

hidmic · 2020-08-24T21:24:13Z

rmw_dds_common/test/test_graph_cache.cpp

+      graph_cache, {
+      {"participant1", "ns1", "node1"},
+      {"participant1", "ns1", "node2"},
+      {"participant1", "ns2", "node1"}});


@brawner perhaps not an issue, but should the test fixture be setup outside the fault injection loop?

Yah, created a test fixture class.

hidmic · 2020-08-24T21:25:29Z

rmw_dds_common/test/test_graph_cache.cpp

+      &allocator,
+      &info);
+    if (RMW_RET_OK == ret) {
+      ret = rmw_topic_endpoint_info_array_fini(&info, &allocator);


@brawner should this be:

Suggested change

ret = rmw_topic_endpoint_info_array_fini(&info, &allocator);

EXPECT_EQ(RWM_RET_OK, rmw_topic_endpoint_info_array_fini(&info, &allocator));

instead? Also, what if fault injection occurs here?

Actually, it will fail during a fault injection test because there will be at least one hook in there. I added a check on the return of fini just in case it needs be done a second time.

Blast545

Minor question

Blast545 · 2020-08-24T21:39:52Z

rmw_dds_common/test/test_graph_cache.cpp

+  RCUTILS_FAULT_INJECTION_TEST(
+  {
+    GraphCache graph_cache;
+    add_participants(graph_cache, {"participant1"});


Can this init section be left out of the fault_injection test macro?

I think so. I created a test fixture class and split this test into three separate TEST_Fs

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

brawner · 2020-08-25T21:19:06Z

rmw_dds_common/test/test_graph_cache.cpp

@@ -1550,3 +1551,64 @@ TEST(test_graph_cache, bad_arguments)
    rcutils_reset_error();
  }
 }
+
+TEST(test_graph_cache, get_writers_info_by_topic_maybe_fail)


Yes, done! I think I originally intended for that given the test name.

brawner · 2020-08-25T21:19:28Z

rmw_dds_common/test/test_graph_cache.cpp

+  RCUTILS_FAULT_INJECTION_TEST(
+  {
+    GraphCache graph_cache;
+    add_participants(graph_cache, {"participant1"});


I think so. I created a test fixture class and split this test into three separate TEST_Fs

brawner · 2020-08-25T21:20:16Z

rmw_dds_common/test/test_graph_cache.cpp

+      graph_cache, {
+      {"participant1", "ns1", "node1"},
+      {"participant1", "ns1", "node2"},
+      {"participant1", "ns2", "node1"}});


Yah, created a test fixture class.

brawner · 2020-08-25T21:21:34Z

rmw_dds_common/test/test_graph_cache.cpp

+      &allocator,
+      &info);
+    if (RMW_RET_OK == ret) {
+      ret = rmw_topic_endpoint_info_array_fini(&info, &allocator);


Actually, it will fail during a fault injection test because there will be at least one hook in there. I added a check on the return of fini just in case it needs be done a second time.

brawner · 2020-08-25T21:23:31Z

rmw_dds_common/test/test_graph_cache.cpp

-      ret = rcutils_string_array_fini(&namespaces);
-      ret = rcutils_string_array_fini(&enclaves);
+      // rcutils_string_array_fini is not under test, so disable fault injection test here.
+      int64_t fault_injection_count = rcutils_fault_injection_get_count();


@hidmic This is the only location here that would make sense to disable fault injection testing because rcutils_string_array_fini is out of scope for this test. Do you think it would make sense to submit a PR to rcutils to add in the disable fault injection testing macros?

I think it's fine as-is for the time being. If it starts showing up more often, we can add a macro.

brawner · 2020-08-26T18:51:33Z

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-08-26T22:17:22Z

Trying again after merging ros2/rcutils#283

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-08-26T23:27:02Z

rpr build fails because it requires a new release to rcutils for the fault injection macros, but the CI builds all pass so this should be good.

dirk-thomas · 2020-08-26T23:29:56Z

rpr build fails because it requires a new release to rcutils for the fault injection macros, but the CI builds all pass so this should be good.

Why not first release rcutils and once retriggered CI passes merge?

brawner · 2020-08-26T23:57:08Z

That's reasonable. It will be my first release, so I was just unsure about the whole process. I'll work to figure it out.

* Add fault injection macro unit tests Signed-off-by: Stephen Brawner <brawner@gmail.com> * target definitions Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner added 2 commits July 28, 2020 16:52

Add fault injection macro unit tests

9a70107

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

target definitions

5d490c1

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

brawner mentioned this pull request Aug 17, 2020

Fault injection macros and functionality (plus example) ros2/rcutils#264

Merged

brawner marked this pull request as ready for review August 24, 2020 19:18

brawner requested review from hidmic and Blast545 August 24, 2020 19:18

hidmic reviewed Aug 24, 2020

View reviewed changes

Blast545 approved these changes Aug 24, 2020

View reviewed changes

PR Fixup

7143397

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

brawner commented Aug 25, 2020

View reviewed changes

brawner mentioned this pull request Aug 26, 2020

Change rcutils_fault_injection_set_count to use int64_t ros2/rcutils#283

Merged

hidmic approved these changes Aug 26, 2020

View reviewed changes

brawner merged commit 14e666d into master Aug 26, 2020

delete-merged-branch bot deleted the brawner/fault-injection-macros branch August 26, 2020 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fault injection macro unit tests #27

Add fault injection macro unit tests #27

brawner commented Jul 29, 2020

brawner commented Aug 12, 2020

brawner commented Aug 24, 2020

hidmic Aug 24, 2020

brawner Aug 25, 2020

hidmic Aug 24, 2020

brawner Aug 25, 2020

hidmic Aug 24, 2020

brawner Aug 25, 2020

Blast545 left a comment

Blast545 Aug 24, 2020

brawner Aug 25, 2020

brawner Aug 25, 2020

brawner Aug 25, 2020

brawner Aug 25, 2020

brawner Aug 25, 2020

brawner Aug 25, 2020

hidmic Aug 26, 2020

brawner commented Aug 26, 2020 •

edited

Loading

brawner commented Aug 26, 2020

brawner commented Aug 26, 2020

dirk-thomas commented Aug 26, 2020

brawner commented Aug 26, 2020

	ret = rmw_topic_endpoint_info_array_fini(&info, &allocator);
	EXPECT_EQ(RWM_RET_OK, rmw_topic_endpoint_info_array_fini(&info, &allocator));

Add fault injection macro unit tests #27

Add fault injection macro unit tests #27

Conversation

brawner commented Jul 29, 2020

brawner commented Aug 12, 2020

brawner commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blast545 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brawner commented Aug 26, 2020 • edited Loading

brawner commented Aug 26, 2020

brawner commented Aug 26, 2020

dirk-thomas commented Aug 26, 2020

brawner commented Aug 26, 2020

brawner commented Aug 26, 2020 •

edited

Loading