Complete rcutils API coverage by hidmic · Pull Request #272 · ros2/rcutils

hidmic · 2020-07-28T22:08:46Z

Precisely what the title says, filling the gaps with mocks. Depends on ros2/Mimick#7.

hidmic · 2020-07-28T22:13:13Z

This isn't PR material just yet, but I wanted to open it early enough for folks to take a look at how tests are looking like, and, ideally, compare it side-by-side with @brawner's fault injection tooling. There are cases that definitely suit mocking conceptually (e.g. dealing with bad system clocks), others not so much (e.g. failing a finalization).

hidmic · 2020-07-29T22:45:22Z

Total coverage is over 95%, but there're places where it is still below that mark. Those places were left out on purpose, and can be split into three (3) groups:

Those that can still be tested without mocking (e.g. using a bad allocator).
Those that I think are good candidates for fault injection (e.g. typical cascading errors during finalization).
Those that I think could use a revisit e.g. the use of an rcutils_string_map_t in logging.c to track logger severity levels adds failure points plus allocations/deallocation cycles on every log for no obvious reason.

src/char_array.c

test/mocking_utils.hpp

brawner · 2020-07-30T19:04:28Z

test/mocking_utils.hpp

+#if !defined(_WIN32)
+#if defined(__MACH__)
+
+class fake_clock : unique_in_scope<fake_clock>


fake_* isn't a very helpful naming style for all these classes. What about rcutils_mock_* or something that helps people get where it's coming from and what its use is?

Being an internal testing tool, I didn't see the need to standardize their names. But moving them out into rcpputils we have to.

test/mocking_utils.hpp

brawner · 2020-07-30T19:12:50Z

test/mocking_utils.hpp

+
+#else  // defined(__MACH__)
+
+class fake_clock : unique_in_scope<fake_clock>


Correct me if I'm wrong, but the only difference between these same class definitions is that do_clock_get_time returns a kern_return_t vs an int? Can you consolidate these differencese to a much smaller #if defined block to reduce duplicated code? I think some of the other classes can be consolidated in a similar way.

Can you consolidate these differences to a much smaller #if defined block to reduce duplicated code?

In this particular case, yes we can. A few using directives can go a long way here.

In general, all "equivalent" classes can be consolidated into one, but in some cases I think interleaving all platform-specific details works against readability and thus I tend to split.

test/mocking_utils.hpp

test/test_char_array.cpp

test/test_format_string.cpp

CMakeLists.txt

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-08-04T23:11:35Z

@Blast545 @brawner PTAL at 9e004d7, as it completely revamps how mocks are made using generic C++ code.

Blast545

Amazing work what you did here to improve Mimick's usage! Left a couple of minor comments to consider before merging

test/test_time.cpp

test/test_filesystem.cpp

Blast545 · 2020-08-05T15:58:00Z

test/test_filesystem.cpp

+    fs.file_info(path).st_mode |= mocking_utils::filesystem::FileTypes::DIRECTORY;
+    fs.exhaust_file_descriptors();
+    size = rcutils_calculate_directory_size(path, g_allocator);
+    EXPECT_EQ(0u, size);


Should be checked here as well the output from the error or errno? saw the rcutils_calculate_directory_size code and it's printed with fprintf to stderr, should that be changed to rcutils_set_error?

It could, but I'd keep changes to rcutils implementation out of this PR. Same as before.

Sounds good, I can help you opening an enhancement issue targeting that if you'd like.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-08-05T18:55:55Z

CI up to rcutils:

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-08-05T18:51:49Z

test/mocking_utils/filesystem.hpp

+};
+
+template<size_t N>
+class FileSystem


I know it's a bit subjective and I suggested the opposite on a different file, but this class doesn't seem to reuse much code across #ifdef blocks. Since it's the filesystem and each OS handles it differently, I think this file makes sense define the full class in single ifdef blocks.

See 83140ec.

brawner · 2020-08-05T18:54:55Z

test/mocking_utils/patch.hpp

+  mmk_mock_define(mock_type, ReturnType);
+};
+
+template<size_t N, typename ReturnType, typename ArgType0>


Is it not possible to use a parameter pack like you do the trampoline struct for the mmk_mock_define calls? I'm not familiar with parameter packs, but I'm guessing you would have used them if it were actually possible

It's not because mmk_mock_define is a macro that needs all the parameter types given as separate arguments at the preprocessing stage -- it won't play along with parameter packs.

brawner · 2020-08-05T19:03:07Z

test/mocking_utils/patch.hpp

+///
+/// Useful to enable patching functions that take arguments whose types
+/// do not define basic comparison operators required by Mimick.
+#define MOCKING_UTILS_DEFINE_DUMMY_OPERATOR(type, op) \


suggest: MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE

Just so when people see it in code, it's a bit clearer what this macro is doing.

See 83140ec.

brawner · 2020-08-05T19:28:06Z

test/test_split.cpp

 #include "rcutils/error_handling.h"
 #include "rcutils/split.h"
 #include "rcutils/types/string_array.h"
+#include "rcutils/types/char_array.h"


Fix ordering

That include actually shouldn't be there! It's a leftover, removed in 46a4c15.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

brawner

Looks good to me. Just a few minor comments.

brawner · 2020-08-05T20:47:21Z

test/mocking_utils/filesystem.hpp

+  explicit FileSystem(const std::string & scope)
+  : opendir_mock_(MOCKING_UTILS_PATCH_TARGET(scope, opendir),
+      MOCKING_UTILS_PATCH_PROXY(opendir)),
+#ifndef _GNU_SOURCE


Also seems like a good candidate for combining with the ifndef block at line 88 below. Line 87 is the only line shared between the two cases.

brawner · 2020-08-05T20:52:15Z

test/test_filesystem.cpp

    EXPECT_FALSE(rcutils_is_readable(path));
  }
+  {
+    char * path = rcutils_join_path(this->test_path, "dummy_nonexisting_file.txt", g_allocator);


Suggested change

char * path = rcutils_join_path(this->test_path, "dummy_nonexisting_file.txt", g_allocator);

char * path = rcutils_join_path(this->test_path, "dummy_nonexistent_file.txt", g_allocator);

brawner · 2020-08-05T20:53:24Z

test/test_filesystem.cpp

+  }
+  {
+    auto fs = mocking_utils::patch_filesystem("lib:rcutils");
+    const char * path = "dummy_non_readable_file.txt";


Suggested change

const char * path = "dummy_non_readable_file.txt";

const char * path = "dummy_unreadable_file.txt";

brawner · 2020-08-05T20:53:41Z

test/test_filesystem.cpp

  }
+  {
+    auto fs = mocking_utils::patch_filesystem("lib:rcutils");
+    const char * path = "dummy_non_writable_file.txt";


Suggested change

const char * path = "dummy_non_writable_file.txt";

const char * path = "dummy_unwritable_file.txt";

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-08-05T21:54:52Z

Well, it appears that there's something very peculiar about how va_list is implemented in aarch64. I don't know what the compiler is doing behind the hood, but the type does not define comparison operators nor it allows overloading such operators. As a result, functions taking va_list arguments are off the table in this platform 🙁

I'll exclude the tests that mock vsnprintf in this platform.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-08-06T19:33:38Z

Oh well, vsnprintf mocking only works on Linux x86-64: binary APIs on MacOS and Windows are inaccessible and undocumented respectively, and the va_list type on Linux aarch64 won't play along with Mimick.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-08-07T13:43:24Z

Alright, finally all green. @brawner @Blast545 PTAL!

hidmic · 2020-08-07T19:41:43Z

Alright, thanks for the reviews guys!

ivanpauno · 2020-08-10T13:48:20Z

I'm not completely sure if this PR is the root cause, but there are a bunch of new test failures on the nightlies https://ci.ros2.org/view/nightly/job/nightly_linux_release/1633/ (all related to rcutils tests).

hidmic · 2020-08-10T14:07:13Z

Indeed. Looks like another corner case when mocking vsnprintf... Looking.

dirk-thomas · 2020-08-10T15:44:09Z

I'm not completely sure if this PR is the root cause, but there are a bunch of new test failures on the nightlies

Since most of them started 3 days ago (see https://ci.ros2.org/view/nightly/job/nightly_linux_extra_rmw_release/797/testReport/ and https://ci.ros2.org/view/nightly/job/nightly_linux_thread_sanitizer/504/testReport/) I would think so.

hidmic · 2020-08-10T16:15:01Z

Fix PR'd in #274.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the hidmic/mock-system-calls branch from 0fb4e3b to bcb394d Compare July 28, 2020 22:10

hidmic force-pushed the hidmic/mock-system-calls branch 3 times, most recently from 4233dee to c9d722a Compare July 29, 2020 21:47

hidmic marked this pull request as ready for review July 29, 2020 22:46

hidmic requested review from Blast545 and brawner July 29, 2020 22:46

brawner reviewed Jul 30, 2020

View reviewed changes

hidmic mentioned this pull request Aug 3, 2020

[WIP] Add mimick based patch() functionality. ros2/rcpputils#92

Closed

Mock system calls in rcutils API implementation.

9e004d7

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the hidmic/mock-system-calls branch from c9d722a to 9e004d7 Compare August 4, 2020 23:10

hidmic requested a review from brawner August 4, 2020 23:10

Blast545 reviewed Aug 5, 2020

View reviewed changes

hidmic added 3 commits August 5, 2020 14:14

Address peer review comments.

164a36d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fully document mocking_utils

253d053

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix filesystem test.

e4cbb8a

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

brawner reviewed Aug 5, 2020

View reviewed changes

hidmic added 3 commits August 5, 2020 17:20

Fix ARM64 and MacOS failures.

b5098df

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Address peer review comments.

83140ec

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Remove unused include.

46a4c15

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

brawner approved these changes Aug 5, 2020

View reviewed changes

Address a few more review comments.

418a433

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic added 3 commits August 5, 2020 19:03

Disable va_list function patching on ARM machines.

004f3ca

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Disable va_list function patching if __aarch64__ is defined.

018a0cf

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix shared library test.

55d5c3d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic added 7 commits August 6, 2020 13:51

Deal with vsnprintf quirks in Windows and MacOS.

893c09c

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix failures on MacOS.

c8db8bd

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Avoid -Wdollar-in-identifier-extension.

44109e7

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix mocking_utils::filesystem constants.

2aaa433

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fully qualify _vsnprintf_s_type.

3f6416c

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix failures on Windows.

55dfebe

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix mocking_utils::filesystem on Windows.

44194a1

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic added 2 commits August 6, 2020 18:36

Fix typo.

1758045

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix another typo.

b22a57d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested review from Blast545 and brawner August 7, 2020 18:43

brawner approved these changes Aug 7, 2020

View reviewed changes

hidmic merged commit ee883ab into master Aug 7, 2020

hidmic deleted the hidmic/mock-system-calls branch August 7, 2020 19:42

hidmic mentioned this pull request Aug 10, 2020

Fix vsnprintf mocks for Release builds. #274

Merged

hidmic mentioned this pull request Aug 10, 2020

Complete subscription API test coverage ros2/rcl#734

Merged

ahcorde pushed a commit that referenced this pull request Oct 2, 2020

Improve test coverage mocking system calls (#272)

ecad08b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

ahcorde pushed a commit that referenced this pull request Oct 6, 2020

Improve test coverage mocking system calls (#272)

0a4e9a7

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

christophebedard mentioned this pull request Feb 2, 2024

Clean up unused references to mimick/mocking in tests #450

Merged


		#else // defined(__MACH__)

		class fake_clock : unique_in_scope<fake_clock>

	char * path = rcutils_join_path(this->test_path, "dummy_nonexisting_file.txt", g_allocator);
	char * path = rcutils_join_path(this->test_path, "dummy_nonexistent_file.txt", g_allocator);

	const char * path = "dummy_non_readable_file.txt";
	const char * path = "dummy_unreadable_file.txt";

	const char * path = "dummy_non_writable_file.txt";
	const char * path = "dummy_unwritable_file.txt";

Conversation

hidmic commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Jul 28, 2020

Uh oh!

hidmic commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidmic commented Aug 4, 2020

Uh oh!

Blast545 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brawner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Aug 5, 2020

Uh oh!

hidmic commented Aug 6, 2020

Uh oh!

hidmic commented Aug 7, 2020

Uh oh!

hidmic commented Aug 7, 2020

Uh oh!

ivanpauno commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

hidmic commented Jul 28, 2020 •

edited

Loading

hidmic commented Jul 29, 2020 •

edited

Loading

hidmic commented Aug 5, 2020 •

edited

Loading

ivanpauno commented Aug 10, 2020 •

edited

Loading

dirk-thomas commented Aug 10, 2020 •

edited

Loading