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.

Sign up for GitHub

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

Jump to bottom

Add fault injection tests to construction/destruction APIs. #144

Merged
merged 4 commits into from Oct 5, 2020
Merged

Add fault injection tests to construction/destruction APIs. #144

merged 4 commits into from Oct 5, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Sep 28, 2020 •
edited

This pull requests aims to add fault injections tests for most if not all rmw APIs.

@hidmic hidmic force-pushed the hidmic/fault-injection-everywhere branch 2 times, most recently from 6ad410c to 8211835 Compare September 30, 2020 17:31
@hidmic hidmic changed the title [WIP] Add fault injection tests to all APIs. Add fault injection tests to all APIs. Sep 30, 2020
@hidmic
Copy link
Contributor Author

hidmic commented Sep 30, 2020

CI up to test_rmw_implementation and rcl:

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

@hidmic hidmic requested review from Lobotuerk and brawner and removed request for Lobotuerk September 30, 2020 17:35
brawner
brawner reviewed Sep 30, 2020
View reviewed changes
test_rmw_implementation/test/test_client.cpp Outdated
rmw_client_t * client =
rmw_create_client(node, ts, service_name, &rmw_qos_profile_default);
if (client) {
int64_t count = rcutils_fault_injection_get_count();
Copy link
Contributor

Choose a reason for hiding this comment

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

Pausing the fault_injection happens a lot in these tests. If there isn't time to create a more obvious function call or macro, I would add a comment here so it's more clear why this is being done. Here and in the cases below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, let's add it. I have to re-release rmw_fastrtps anyways, adding rcutils to the queue won't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See ros2/rcutils#295. I'll adapt this PR.

@hidmic
Add fault injection tests to all construction/destruction APIs.
1bdb65b 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Use RCUTILS_NO_FAULT_INJECTION() macro.
7c8618d 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/fault-injection-everywhere branch from 5ebb778 to 7c8618d Compare September 30, 2020 21:00
@hidmic
Copy link
Contributor Author

hidmic commented Sep 30, 2020

CI up to rcutils, test_rmw_implementation, and rcl:

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

brawner
brawner reviewed Sep 30, 2020
View reviewed changes
test_rmw_implementation/test/test_client.cpp Outdated
ROSIDL_GET_SRV_TYPE_SUPPORT(test_msgs, srv, BasicTypes);
RCUTILS_FAULT_INJECTION_TEST(
{
int64_t count = rcutils_fault_injection_get_count();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing RCUTILS_NO_FAULT_INJECTION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, thought I had covered them all. See 7309204.

test_rmw_implementation/test/test_create_destroy_node.cpp Outdated
{
constexpr char node_name[] = "my_node";
constexpr char node_namespace[] = "/my_ns";
int64_t count = rcutils_fault_injection_get_count();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same 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.

See 7309204.

test_rmw_implementation/test/test_publisher.cpp Outdated
rmw_publisher_t * pub =
rmw_create_publisher(node, ts, topic_name, &rmw_qos_profile_default, &options);
if (pub) {
int64_t count = rcutils_fault_injection_get_count();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 7309204.

@hidmic
Use RCUTILS_NO_FAULT_INJECTION() macro everywhere.
7309204 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/fault-injection-everywhere branch from 0ec4a9b to 7309204 Compare September 30, 2020 22:16
@hidmic
Adapt wait_set tests for extra fault injection.
ceac659 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 30, 2020

CI up to rcutils, test_rmw_implementation, and rcl:

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

@hidmic
Copy link
Contributor Author

hidmic commented Oct 1, 2020

Hmm, CI failures appear to be coming from a different thread. I'll dig in.

@hidmic hidmic mentioned this pull request Oct 1, 2020
Merged
@hidmic
Copy link
Contributor Author

hidmic commented Oct 1, 2020 •
edited

CI again after ros2/rmw_fastrtps#458:

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

@hidmic
Copy link
Contributor Author

hidmic commented Oct 2, 2020

@ros-pull-request-builder retest this please

1 similar comment
@hidmic
Copy link
Contributor Author

hidmic commented Oct 2, 2020

@ros-pull-request-builder retest this please

@hidmic
Copy link
Contributor Author

hidmic commented Oct 5, 2020

All green ! Going in.

@hidmic hidmic changed the title Add fault injection tests to all APIs. Add fault injection tests to construction/destruction APIs. Oct 5, 2020
@hidmic hidmic merged commit 4204ed5 into master Oct 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/fault-injection-everywhere branch October 5, 2020 15:29
ahcorde pushed a commit that referenced this pull request Oct 9, 2020
@hidmic @ahcorde
Add fault injection tests to construction/destroy APIs. (#144)
40d1502 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 21, 2020
@hidmic @ahcorde
Add fault injection tests to construction/destroy APIs. (#144)
bd75bb0 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brawner brawner brawner approved these changes

@Lobotuerk Lobotuerk Awaiting requested review from Lobotuerk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hidmic @brawner