Skip to content

Add fault injection macros and unit tests#80

Merged
brawner merged 6 commits intomasterfrom
brawner/fault-injection-macros
Aug 26, 2020
Merged

Add fault injection macros and unit tests#80
brawner merged 6 commits intomasterfrom
brawner/fault-injection-macros

Conversation

@brawner
Copy link
Copy Markdown
Contributor

@brawner brawner commented Jul 29, 2020

This adds unit test macros and fault injection to rosidl_typesupport_c and rosidl_typesupport_cpp. With these fault injections, coverage can get to 100%. I'll post CI results soon.

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

@brawner brawner self-assigned this Jul 29, 2020
@brawner
Copy link
Copy Markdown
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

@brawner brawner force-pushed the brawner/fault-injection-macros branch 2 times, most recently from 8749636 to 1110fbe Compare August 14, 2020 00:39
@brawner brawner mentioned this pull request Aug 17, 2020
Merged
@brawner brawner marked this pull request as ready for review August 24, 2020 19:07
brawner added 3 commits August 24, 2020 12:08
@brawner
Add fault injection macros and unit tests
707840a 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
target definitions
1d2bfd6 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Adjust tests
a6fa53e 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/fault-injection-macros branch from 1110fbe to a6fa53e Compare August 24, 2020 19:13
@brawner
Copy link
Copy Markdown
Contributor Author

brawner commented Aug 24, 2020

Rebase against master after merging #82

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

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

@brawner brawner requested review from Blast545 and hidmic August 24, 2020 19:15
Blast545
Blast545 reviewed Aug 24, 2020
View reviewed changes
Copy link
Copy Markdown

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

Minor comments

rosidl_typesupport_cpp/test/test_message_type_support_dispatch.cpp
&type_support_cpp_identifier,
"test_type_support1");
EXPECT_NE(result, nullptr);
} catch (...) {
Copy link
Copy Markdown

@Blast545 Blast545 Aug 24, 2020

Choose a reason for hiding this comment

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

Why the try catch block in this scenario? if it is an expected throw, maybe EXPECT_THROW can be used? (Not clear to me if possible though)

Copy link
Copy Markdown
Contributor Author

@brawner brawner Aug 25, 2020

Choose a reason for hiding this comment

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

Because it doesn't throw for all potential errors. In some instances it will just return a nullptr.

Copy link
Copy Markdown
Contributor

@hidmic hidmic Aug 26, 2020

Choose a reason for hiding this comment

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

Should we be catching specific exceptions? Or in other words, doesn't the API specify what exceptions it may throw?

Copy link
Copy Markdown
Contributor Author

@brawner brawner Aug 26, 2020

Choose a reason for hiding this comment

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

I don't think the API does say anything about it, but adding a specific catch for the runtime errors seems reasonable.

Copy link
Copy Markdown
Contributor Author

@brawner brawner Aug 26, 2020

Choose a reason for hiding this comment

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

Added (btw)

Copy link
Copy Markdown
Contributor

@hidmic hidmic Aug 26, 2020

Choose a reason for hiding this comment

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

At some point, we should document potential exceptions just like we document error codes.

@brawner
PR Fixup
5ca86c2 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Blast545
Blast545 approved these changes Aug 26, 2020
View reviewed changes
Copy link
Copy Markdown

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

LGTM

@brawner
Catch runtime exception
4fb9a2f 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Copy Markdown
Contributor Author

brawner commented Aug 26, 2020

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

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

brawner commented Aug 26, 2020

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

@brawner
Copy link
Copy Markdown
Contributor Author

brawner commented Aug 26, 2020

The Rpr job fails because it requires a new release to rcutils for the fault injection macros. CI jobs all pass.

@brawner brawner merged commit cc5dfe8 into master Aug 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the brawner/fault-injection-macros branch August 26, 2020 23:28
ahcorde pushed a commit that referenced this pull request Oct 2, 2020
@brawner @ahcorde
Add fault injection macros and unit tests (#80)
f138f56 
* Add fault injection macros and unit tests

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

* target definitions

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

* Adjust tests

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

* PR Fixup

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

* Catch runtime exception

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

* Unused variable

Signed-off-by: Stephen Brawner <brawner@gmail.com>
ahcorde pushed a commit that referenced this pull request Oct 8, 2020
@brawner @ahcorde
Add fault injection macros and unit tests (#80)
7dab58c 
* Add fault injection macros and unit tests

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

* target definitions

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

* Adjust tests

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

* PR Fixup

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

* Catch runtime exception

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

* Unused variable

Signed-off-by: Stephen Brawner <brawner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Blast545 Blast545 Blast545 approved these changes

@hidmic hidmic hidmic approved these changes

Assignees

@brawner brawner

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brawner @Blast545 @hidmic @chapulina