Skip to content
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.

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

Fault injection macros and functionality (plus example) #264

Merged
merged 15 commits into from
Aug 19, 2020

Conversation

brawner
Copy link
Contributor

@brawner brawner commented Jul 1, 2020

This introduces a set of fault injection macros that enable selective functions to return failing error codes to help test upstream code.

The macro RCUTILS_MAYBE_RETURN_ERROR is a compile flag enabled macro that selectively returns the error value that is passed to it. It allows for simple unit tests of upstream code that helps ensure the code under test is robust to all possible return failures regardless of how difficult they are to trigger. In order to enable RCUTILS_MAYBE_RETURN_ERROR, you must set the global counter.

The counter is set with RCUTILS_SET_FAULT_INJECTION_COUNT. The counter counts down for every invocation of RCUTILS_MAYBE_RETURN_ERROR and when it reaches 0 will cause RCUTILS_MAYBE_RETURN_ERROR to return the designated error. As this macro may be widely placed in the code base, it is necessary to run this in a for loop in the unit tests. For example

TEST(TestMyFunction, test_function) {
  constexpr int maximum_calls_to_maybe_return = 100;
  for (int i = 0; i < maximum_calls_to_maybe_return; ++i) {
    RCUTILS_SET_FAULT_INJECTION_COUNT(i);
    auto result = function_under_test();
    // unless cleanup needs to occur, result is likely to be ignored
  }
}

This sort of test doesn't help you assert that the behavior of the function is correct in face of downstream code failing, but it does let you ensure that your code returns safely and that resources are adequately cleaned up in all possible return paths (with the appropriate analysis tools).

The high level macro RCUTILS_CAN_RETURN_WITH_ERROR_OF is a descriptive macro that includes a call to RCUTILS_MAYBE_RETURN_ERROR, with the RCUTILS_ENABLE_FAULT_INJECTION compiler flag enabled. It has the extra benefit of providing a quick glance at the expected failure return codes in the function.

Currently, the intention is to use RCUTILS_CAN_RETURN_WITH_ERROR_OF once for each error return value, but more fully featured macros could also be included. For example, variadic versions can be introduced like RCUTILS_CAN_RETURN_WITH_ERRORS_OF, RCUTILS_CAN_RETURN_WITH_STRING_ERRORS_OF, etc.

Possible Extensions

RCUTILS_CHECK_RETURN_OK
If labeling the upstream code is troublesome, an alternative macro like RCUTILS_CHECK_RETURN_OK could be created that would replace simple if statements. This would be placed in the downstream code that checks the called functions for error values. That macro would include the call to RCUTILS_MAYBE_FAIL, which executes a statement on error (not just return an error value).

#define RCUTILS_CHECK_RETURN_OK(ret, error_message, error_statement) \
  RCUTILS_MAYBE_FAIL(error_statement); \
  if (ret != RCUTILS_RET_OK) { \
    RCUTILS_SET_ERROR_MESSAGE(error_message); \
    error_statement; \
  }

Targeted fault injection macros
While RCUTILS_MAYBE_RETURN_ERROR allows for robust coverage of numerous possible return paths with very easy unit tests, it doesn't allow for targeted failures. Another extension of this feature would be to give string names to each RCUTILS_CAN_RETURN_WITH_ERROR_OF macro that are registered with a global lookup. In the unit test, these macros could then be set to specifically fail and the expected failure behavior can then be asserted.

Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner self-assigned this Jul 1, 2020
src/snprintf.c Outdated
@@ -39,6 +39,8 @@ rcutils_snprintf(char * buffer, size_t buffer_size, const char * format, ...)
int
rcutils_vsnprintf(char * buffer, size_t buffer_size, const char * format, va_list args)
{
RCUTILS_CAN_RETURN_WITH_ERROR_OF(-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main concern here is that we are going to litter our code with these macros. It's not the end of the world, but it is noisy and can make the code harder to read.

I have 2 separate suggestions:

  1. Before we start down this road, let's see what @hidmic's work on Mimick looks like. If we can make that work, that will obviate the need for this, I think (I'm currently ignoring the fact that Mimick doesn't work for C++, while this one would).
  2. If we can't get Mimick to work for some reason, another alternative is to use the LD_PRELOAD mechanism to override the symbols as needed. This is less desirable than Mimick since it is limited to Linux-only, but I think we can make the case (at least for now) that doing it on one platform is better than doing it on zero platforms.

The benefit to both solutions above is that there is no changes to the code, just changes to the tests. But I'd also like to hear what others think.

Copy link
Contributor Author

@brawner brawner Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed this PR last night, and intended to give a more full writeup. Please take a look at the above PR description for some more information as well as possible extensions.

The approach taken in this PR allows for immediate cross-platform, cross-language utilization with simple unit tests with no external dependencies. You can write a unit test that easily verifies your code is hardened against failures in upstream libraries. See ros2/rcl_logging#48 for an example.

Another approach was to add an RCUTILS_CHECK_ macro that replaced simple if statements. Based on feedback from you and @wjwwood that suggested we want to be able to test the same code that we ship, I took this approach. The use of a macro like RCUTILS_CAN_RETURN_WITH_ERROR_OF allows for testing shipped production libraries against modified upstream libraries, which is not something our CI or buildfarm are setup for today but if desired could be feasibly updated in the future.

I took care in the design of RCUTILS_CAN_RETURN_WITH_ERROR_OF to ensure that it wasn't just litter. It actually serves an additional purpose of describing intended failure cases of the function. While docblocks typically try to capture the possible failures cases, this would be another approach that can also be integrated with unit testing. It may be difficult for a maintainer to see all the possible return values of a high level function that it passes on from lower level functions. However, with unit tests utilizing RCUTILS_MAYBE_RETURN_ERROR, you can verify that all possible return values are sufficiently understood.

This PR is intended as a starting point, and I want to make it as useful as possible. There are several possible feature additions that I think could fill it out, but I would like to get some more buy in as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for using potentially harsh language above. That wasn't my intention.

As this stands, I think you've done a good job of minimizing the impact of these macros on the "live" code (that is, the real code we are trying to test and that we ship). That is, I don't see how this method of mocking could be any simpler than adding a single macro line to any function/method that you want to mock out. As we've discussed elsewhere, this also sets the stage for having the macro enabled for our nightly and CI builds, but disabled for our packaging and debian builds.

My biggest concern is deploying this kind of change across a large number of functions and methods throughout the ROS 2 codebase. Adding things like this to every function places additional mental burden on contributors, as all readers have to understand why they are here and what they do. It also makes it somewhat difficult to move away from this solution in the future if we find a better way.

I want to be clear that I am not totally against this change. However, I think some of the other options we are pursuing don't have the downsides of this solution, so I'd prefer to investigate those other options first. If those solutions turn out not to work for one reason or another, this is an acceptable path forward.

Does that clear up my thinking on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, it does help clarify your thinking. My hope is that this sort of feature is used in conjunction with mocking like what @hidmic is investigating. That sort of testing would allow for more targeted failures, while this approach especially with the for loop example might be more like throwing a grenade at your code and seeing if it survives.

You're absolutely right that there are a large number of places where the RCUTILS_CAN_RETURN_WITH_ERROR_OF macro can be placed, and it would be overwhelming to put them in every single location at the outset, or even for every contribution. But I also think it's possible to add this macro in piecemeal, and we can start by only adding it in when it's needed for unit tests. For example, to push rcl_logging_spdlog to 100% coverage, you only need rcutils_snprintf to fail, but given the correct inputs that function probably never will on our supported platforms.

My suggestion with this sort of change is to start small and expand it as its role in this codebase is better understood. That's partially the reason why I only introduce one flavor of RCUTILS_CAN_RETURN_WITH_ERROR_OF, even though I also want a variadic version.

That macro is empty if fault injection is not enabled, which I think helps make it easier to remove in the future. The example unit test in rcl_logging_spdlog would pass if RCUTILS_CAN_RETURN_WITH_ERROR_OF did not exist at all. We could also properly deprecate RCUTILS_SET_FAULT_INJECTION_COUNT to help point external developers to the appropriate replacement.

I also agree that seeing this macro at the top of a function would be strange and unusual to many developers, particularly newer ROS 2 users who are unfamiliar with the code. While I tried to be careful about the macro name, I'm also very open to rewording RCUTILS_CAN_RETURN_WITH_ERROR_OF. I chose the wording so that it's hopefully clear to developers unfamiliar with it that doesn't change the nominal behavior of the function, while conveying more information about the intent of the function's behavior if you are familiar with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we start down this road, let's see what @hidmic's work on Mimick looks like. If we can make that work, that will obviate the need for this, I think (I'm currently ignoring the fact that Mimick doesn't work for C++, while this one would).

@clalancette I think this serves a different use case. Mocking is good for white-box unit testing, while this is performing black-box integration testing to see how the system, with all its side effects, behaves in an exceptional event (akin to exception safety verification). Could we use mocks to implement it? Yes, but we'd end up coupling a package to its dependencies' implementation and recreating side effects -- possible, but much harder.

IMHO, RCUTILS_CAN_RETURN_WITH_ERROR_OF, though atypical, isn't strictly worse than the prologue of RCUTILS_CHECK_ARGUMENT_FOR_NULL our C functions usually have.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good!

* production operation. This is because the fault injection will cause the function to return
* where this macro is used, not at the location the error values are typically returned. To help
* protect against this scenario you may consider adding unit tests that checks your function does
* not modify output parameters when it actually returns a failing error code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brawner I think this the a strong (the strongest?) constrain. This will cover 90% of our use cases, but, in the most general case, not all errors occur during early checks and not all errors can (should?) have zero observable side-effects. Could the macro take some statements to mimic such side effects if any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that may be a reasonable extension to add. The difficulty though is that there may be multiple instances of returning the same error code in a function (for example RCUTILS_RET_ERROR or RCUTILS_BAD_ALLOC), then the side effects may be different depending on where it returns.

While many cases might have side effects after returning through an error case, my limited experience with this code base suggests those typically result in an undefined state more than an expected partially completed state.

I think for this case, we'll just have to build up a list of examples and what the requirements for those functions would be.

Copy link
Contributor

@hidmic hidmic Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I'd like to point out though that while:

While many cases might have side effects after returning through an error case, my limited experience with this code base suggests those typically result in an undefined state more than an expected partially completed state.

often holds true, IMHO it's bad practice. Really bad practice. If your program is left in an undefined state after returning an error, you may as well terminate it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(does) not modify output parameters when it actually returns a failing error code.

Meta: that's not always true or possible, finalization functions being a good counterexample. I don't think the limitation should be addressed here though (or ever if mocks help us cover those cases).

include/rcutils/testing/fault_injection.h Outdated Show resolved Hide resolved
/**
* \def RCUTILS_MAYBE_RETURN_ERROR
* \brief This macro checks and decrements a static global variable atomic counter and returns
* `return_value_on_error` if 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brawner one thing this scheme cannot do is force an error during cleanup after one or more errors have been forced. What about if, in addition to forcing a single function to fail, we make all following functions fail as well? Like a flag to prevent the count to ever be decremented below 0.

I envision the RCUTILS_CHECK_FAULT_SAFETY macro below taking an optional FLUKE or MASSIVE argument for each case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some examples where a failure in one location is likely to result in a failure in another location. For example, allocations may continue to fail, or file system functions may continue to fail. For the sake of coverage, there are locations where it would take two failures to reach some lines of code. Your example of cleanup is a good one.

I think there are a few of potential solutions:

  1. Setting potential alternate behaviors when the counter is negative, which is what you are suggesting. I think the idea works great, it would just require writing multiple unit tests for each behavior.

  2. Instead of leaving it as -1, the RCUTILS_SET_FAULT_COUNT macro could have a variadic version which takes as parameters counts that it's reset to after it reaches 0.

RCUTILS_SET_FAULT_COUNT(0, 0)

For example, would trip a second failure immediately after its first failure. This would let you test more types of failures, and then we could use this type of macro to check that code is not only one-fault tolerant, but two-fault tolerant, three-fault tolerant etc.

  1. For instances where testing two particular faults is important, then we could introduce a named fault injection. Instead of RCUTILS_CAN_RETURN_WITH_ERROR, there would be a RCUTILS_CAN_RETURN_WITH_ERROR_NAMED. Then in the unit test, we could target those locations to fail explicitly in a unit test.
RCUTILS_SET_FAULT_INJECTION_NAMED(...);
RCUTILS_SET_FAULT_INJECTION_NAMED(...);
... // Run test code

If we do introduce this one at a later date, I could see it replacing the unnamed version macro entirely. This idea requires multiple unit tests for each targeted failed injection, but is also the only option that allows for targeted injections in the first place.

Do you have any thoughts about the other options suitability for the situation you are describing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think solution (2.) would be best. We don't really know what we're hitting, so N-fault tolerant sounds as good as it gets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be added in a followup PR when the functionality becomes necessary for better coverage.

src/snprintf.c Outdated
@@ -39,6 +39,8 @@ rcutils_snprintf(char * buffer, size_t buffer_size, const char * format, ...)
int
rcutils_vsnprintf(char * buffer, size_t buffer_size, const char * format, va_list args)
{
RCUTILS_CAN_RETURN_WITH_ERROR_OF(-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we start down this road, let's see what @hidmic's work on Mimick looks like. If we can make that work, that will obviate the need for this, I think (I'm currently ignoring the fact that Mimick doesn't work for C++, while this one would).

@clalancette I think this serves a different use case. Mocking is good for white-box unit testing, while this is performing black-box integration testing to see how the system, with all its side effects, behaves in an exceptional event (akin to exception safety verification). Could we use mocks to implement it? Yes, but we'd end up coupling a package to its dependencies' implementation and recreating side effects -- possible, but much harder.

IMHO, RCUTILS_CAN_RETURN_WITH_ERROR_OF, though atypical, isn't strictly worse than the prologue of RCUTILS_CHECK_ARGUMENT_FOR_NULL our C functions usually have.

@hidmic
Copy link
Contributor

hidmic commented Jul 3, 2020

RCUTILS_CHECK_RETURN_OK

Hmm, if the call succeeded, with its side effects, cleanup code may not be enough when simulating an error. Also, if the callee does fault injection, there's no need for the caller to do the same. For those callees that don't, perhaps:

auto result = RCUTILS_MAY_FAIL_WITH(/* bad_result */, /* expression */);

can bypass the expression? It does clutter the code a bit though.

Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/fault-injection-macros branch from 6a115e6 to adad3bb Compare July 28, 2020 22:50
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Copy link
Contributor Author

@brawner brawner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been plugging away at adding these to more packages. I've adjusted the unit test macros so it's even simpler now. There is just a single RCUTILS_FAULT_INJECTION_TEST macro to wrap your code-under-test.

See the following PRs for more examples.
ros2/rosidl#509 rosidl_runtime_c
ros2/rcl_logging#49 rcl_logging_spdlog
ros2/rosidl_typesupport#80 rosidl_typesupport_c, rosidl_typesupport_cpp
ros2/rmw_dds_common#27 rmw_dds_common
ros2/rcl#727 rcl
ros2/rcl#730 rcl_action
ros2/rcl#731 rcl_lifecycle

@@ -51,6 +54,8 @@ int _rcutils_maybe_fail();
*/
#define RCUTILS_MAYBE_RETURN_ERROR(return_value_on_error) \
if (RCUTILS_FAULT_INJECTION_FAIL_NOW == _rcutils_maybe_fail()) { \
printf( \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been a very helpful error message to print out while debugging tests. Is there not a way to use a logging macro here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why not having an RCUTILS_LOG_DEBUG call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RCUTILS_LOG_DEBUG is not defined inside the rcutils library unfortunately.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good!

@@ -51,6 +54,8 @@ int _rcutils_maybe_fail();
*/
#define RCUTILS_MAYBE_RETURN_ERROR(return_value_on_error) \
if (RCUTILS_FAULT_INJECTION_FAIL_NOW == _rcutils_maybe_fail()) { \
printf( \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why not having an RCUTILS_LOG_DEBUG call here.

int fault_injection_count = 0; \
do { \
RCUTILS_FAULT_INJECTION_SET_COUNT(fault_injection_count++); \
code; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brawner nice! I still think we need a way to disable fault injection temporarily within the code statement, otherwise you may hit a fault while handling or checking the outcome of the piece of functionality that is actually under test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be done by way of:

int current_count = RCUTILS_FAULT_INJECTION_GET_COUNT();
RCUTILS_FAULT_INJECTION_SET_COUNT(RCUTILS_FAULT_INJECTION_NEVER_FAIL);
...
RCUTILS_FAULT_INJECTION_SET_COUNT(current_count);

Do you think we need something more like:

RCUTILS_FAULT_INJECTION_PAUSE_TEST();
...
RCUTILS_FAULT_INJECTION_RESUME_TEST();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, some sugar would be nice. I'd personally like to have something along the lines of:

RCUTILS_FAULT_INJECTION_NO_INJECT({
  // ...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your solution will introduce scoping issues, but I still agree that a potential use case along these lines may be useful. As I currently don't need it for the existing packages, I'll save this for a followup PR.

Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/fault-injection-macros branch 2 times, most recently from 6ba4dea to 0ad3845 Compare August 12, 2020 22:14
Copy link
Contributor Author

@brawner brawner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated how the compile definitions are set in the CMakeLists.txt. If they required a cmake variable to enable them, then you would get many cmake warnings about unused cmake variables for most other packages.

@@ -51,6 +54,8 @@ int _rcutils_maybe_fail();
*/
#define RCUTILS_MAYBE_RETURN_ERROR(return_value_on_error) \
if (RCUTILS_FAULT_INJECTION_FAIL_NOW == _rcutils_maybe_fail()) { \
printf( \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RCUTILS_LOG_DEBUG is not defined inside the rcutils library unfortunately.

int fault_injection_count = 0; \
do { \
RCUTILS_FAULT_INJECTION_SET_COUNT(fault_injection_count++); \
code; \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be done by way of:

int current_count = RCUTILS_FAULT_INJECTION_GET_COUNT();
RCUTILS_FAULT_INJECTION_SET_COUNT(RCUTILS_FAULT_INJECTION_NEVER_FAIL);
...
RCUTILS_FAULT_INJECTION_SET_COUNT(current_count);

Do you think we need something more like:

RCUTILS_FAULT_INJECTION_PAUSE_TEST();
...
RCUTILS_FAULT_INJECTION_RESUME_TEST();

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/fault-injection-macros branch from 0ad3845 to f5bb491 Compare August 12, 2020 23:04
@brawner
Copy link
Contributor Author

brawner commented Aug 12, 2020

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 Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Aug 12, 2020

Rebuilding

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner brawner force-pushed the brawner/fault-injection-macros branch from 58f6f5e to 63a620c Compare August 12, 2020 23:50
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/fault-injection-macros branch from 63a620c to 2e99f4c Compare August 12, 2020 23:51
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Contributor Author

brawner commented Aug 13, 2020

Today Stephen learned about ATOMIC_VAR_INIT. I also removed the get/set macros and just made the functions public.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner
Copy link
Contributor Author

brawner commented Aug 14, 2020

Rebasing rosidl_typesupport_c onto new bug fix!

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner brawner marked this pull request as ready for review August 14, 2020 00:40
@brawner
Copy link
Contributor Author

brawner commented Aug 14, 2020

rosidl_typesupport_c fix ros2/rosidl_typesupport#82

@brawner
Copy link
Contributor Author

brawner commented Aug 14, 2020

@hidmic @clalancette I think this is ready for a re-review. The above jobs are now working well, with the recent bug fix and a new push to rcl_action, which should resolve the windows build warning.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

include/rcutils/macros.h Outdated Show resolved Hide resolved
include/rcutils/macros.h Outdated Show resolved Hide resolved
* production operation. This is because the fault injection will cause the function to return
* where this macro is used, not at the location the error values are typically returned. To help
* protect against this scenario you may consider adding unit tests that checks your function does
* not modify output parameters when it actually returns a failing error code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(does) not modify output parameters when it actually returns a failing error code.

Meta: that's not always true or possible, finalization functions being a good counterexample. I don't think the limitation should be addressed here though (or ever if mocks help us cover those cases).

include/rcutils/testing/fault_injection.h Outdated Show resolved Hide resolved
include/rcutils/testing/fault_injection.h Show resolved Hide resolved
int fault_injection_count = 0; \
do { \
RCUTILS_FAULT_INJECTION_SET_COUNT(fault_injection_count++); \
code; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, some sugar would be nice. I'd personally like to have something along the lines of:

RCUTILS_FAULT_INJECTION_NO_INJECT({
  // ...
});

src/snprintf.c Show resolved Hide resolved
include/rcutils/macros.h Show resolved Hide resolved
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Copy link
Contributor Author

@brawner brawner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new macro to cover the snprintf case which also sets errno

include/rcutils/macros.h Show resolved Hide resolved
include/rcutils/testing/fault_injection.h Show resolved Hide resolved
int fault_injection_count = 0; \
do { \
RCUTILS_FAULT_INJECTION_SET_COUNT(fault_injection_count++); \
code; \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your solution will introduce scoping issues, but I still agree that a potential use case along these lines may be useful. As I currently don't need it for the existing packages, I'll save this for a followup PR.

src/snprintf.c Show resolved Hide resolved
include/rcutils/testing/fault_injection.h Outdated Show resolved Hide resolved
include/rcutils/macros.h Outdated Show resolved Hide resolved
include/rcutils/macros.h Outdated Show resolved Hide resolved
@brawner
Copy link
Contributor Author

brawner commented Aug 15, 2020

More testing.

Standard builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Debug builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Linux only, rmw specific builds:

  • Connext Build Status
  • CycloneDDS Build Status

@brawner
Copy link
Contributor Author

brawner commented Aug 15, 2020

Testing this patch of rcutils against current ros2 branches with --packages-above rcutils to ensure I'm not breaking anything.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner
Copy link
Contributor Author

brawner commented Aug 17, 2020

All the unstable builds for the wider array of packages were caused by a cpplint issue with rcl_action. Addressed that and pushed to that respective branch. Failures for builds against current branches of ros 2 with this PR were due to an incompatibility with qt_gui_cpp and gcov it looks like.

Building and testing all fault injection branches

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner
Copy link
Contributor Author

brawner commented Aug 17, 2020

Coverage results with current fault injection branches:

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/fault-injection-macros branch from b823bc1 to 6a5daad Compare August 17, 2020 20:16
@brawner
Copy link
Contributor Author

brawner commented Aug 18, 2020

Testing this patch against current sources one more time after yesterdays commits.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner
Copy link
Contributor Author

brawner commented Aug 19, 2020

All tests pass, except qt_gui_cpp which fails due to the same gcov issue. That package and packages that depend on it are excluded from the nightly coverage jobs (I'm assuming for this reason).

@brawner brawner merged commit af3ac24 into master Aug 19, 2020
ahcorde pushed a commit that referenced this pull request Oct 2, 2020
* Initial commit of fault injection macros and functionality

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

* Add can_return_with_failure to rcutils_vsnprintf

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

* Add getter for fault injection count

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

* Adjust headers

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

* Adding more fault injection locations

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

* Revert header strdup

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

* Adjust names

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

* Remove init/fini macros

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

* uncrustify fix

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

* Update definitions

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

* Change maybe_fail int int_least64

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

* ATOMIC_VAR_INIT

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

* Move get/set to public functions

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

* Add RCUTILS_CAN_FAIL_WITH

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

* Updating comments

Signed-off-by: Stephen Brawner <brawner@gmail.com>
ahcorde pushed a commit that referenced this pull request Oct 6, 2020
* Initial commit of fault injection macros and functionality

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

* Add can_return_with_failure to rcutils_vsnprintf

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

* Add getter for fault injection count

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

* Adjust headers

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

* Adding more fault injection locations

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

* Revert header strdup

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

* Adjust names

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

* Remove init/fini macros

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

* uncrustify fix

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

* Update definitions

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

* Change maybe_fail int int_least64

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

* ATOMIC_VAR_INIT

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

* Move get/set to public functions

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

* Add RCUTILS_CAN_FAIL_WITH

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

* Updating comments

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@clalancette clalancette deleted the brawner/fault-injection-macros branch April 22, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants