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 unit tests (coverage part 2/3) #766

Merged
merged 4 commits into from
Sep 2, 2020
Merged

Add fault-injection unit tests (coverage part 2/3) #766

merged 4 commits into from
Sep 2, 2020

Conversation

brawner
Copy link
Contributor

@brawner brawner commented Aug 26, 2020

This adds unit tests to rcl_yaml_param_parser to increase coverage to 95.2%. Since the PR is so long, I'm leaving it in draft for now and will plan to refactor it into a couple of parts for easier review.

Depends on #765

Signed-off-by: Stephen Brawner [email protected]

@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch 2 times, most recently from 55b4a78 to e1ec1db Compare August 27, 2020 21:32
@brawner brawner changed the base branch from brawner/rcl_yaml-set-values-null to brawner/rcl_yaml-basic-unit-tests August 27, 2020 21:49
@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch 2 times, most recently from ecd08bc to ffefb0f Compare August 27, 2020 21:54
@brawner brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from cf14a57 to 56ba274 Compare August 27, 2020 21:58
@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch 4 times, most recently from ce10329 to 599c3c8 Compare August 27, 2020 22:12
@brawner brawner mentioned this pull request Aug 27, 2020
Merged
@brawner brawner marked this pull request as ready for review August 27, 2020 22:22
@brawner
Copy link
Contributor Author

brawner commented Aug 27, 2020

This PR has now been split into several different PRs.

This depends on #771, which adds the basic unit tests.
It is also followed by #772, which adds the mocking unit test.

@brawner brawner changed the title Add fault-injection and mocking unit tests for 95% coverage Add fault-injection unit tests (part 2/3) Aug 27, 2020
@brawner brawner changed the title Add fault-injection unit tests (part 2/3) Add fault-injection unit tests (coverage part 2/3) Aug 27, 2020
@brawner brawner mentioned this pull request Aug 27, 2020
Merged
@brawner brawner requested review from hidmic and Blast545 August 27, 2020 22:34
@brawner brawner self-assigned this Aug 27, 2020
Blast545
Blast545 approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@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 brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 5940d72 to 8b9dd40 Compare August 28, 2020 23:33
@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch from b98697c to a7af61a Compare August 28, 2020 23:37
@brawner
Copy link
Contributor Author

brawner commented Aug 28, 2020 •
edited
Loading

I checked these tests for memory leaks. I discovered two additional ones when running test_parser. One was easily fixable with #775. The second is allocated in the same spot, but is more perplexing to me.

==2296119== HEAP SUMMARY:
==2296119==     in use at exit: 211 bytes in 18 blocks
==2296119==   total heap usage: 5,540,035 allocs, 5,540,017 frees, 480,244,485 bytes allocated
==2296119== 
==2296119== 171 (96 direct, 75 indirect) bytes in 5 blocks are definitely lost in loss record 3 of 3
==2296119==    at 0x483FFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==2296119==    by 0x507B2AA: __default_reallocate (allocator.c:53)
==2296119==    by 0x505F425: add_val_to_string_arr (add_to_arrays.c:107)
==2296119==    by 0x5062155: parse_value (parse.c:363)
==2296119==    by 0x5063C8B: parse_file_events (parse.c:614)
==2296119==    by 0x5065B62: rcl_parse_yaml_file (parser.c:206)
==2296119==    by 0x159ECE: RclYamlParamParser_test_parse_file_with_bad_allocator_Test::TestBody() (test_parser.cpp:344)
==2296119==    by 0x1D0717: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2447)
==2296119==    by 0x1C24B4: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2483)
==2296119==    by 0x175CB4: testing::Test::Run() (gtest.cc:2522)
==2296119==    by 0x1772AB: testing::TestInfo::Run() (gtest.cc:2703)
==2296119==    by 0x177FE2: testing::TestCase::Run() (gtest.cc:2825)
==2296119== 
==2296119== LEAK SUMMARY:
==2296119==    definitely lost: 96 bytes in 5 blocks
==2296119==    indirectly lost: 75 bytes in 12 blocks
==2296119==      possibly lost: 0 bytes in 0 blocks
==2296119==    still reachable: 40 bytes in 1 blocks
==2296119==         suppressed: 0 bytes in 0 blocks
==2296119== Reachable blocks (those to which a pointer was found) are not shown.
==2296119== To see them, rerun with: --leak-check=full --show-leak-kinds=all

@brawner brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 8b9dd40 to 422dd65 Compare August 29, 2020 00:50
@hidmic
Copy link
Contributor

hidmic commented Aug 31, 2020

@brawner could it be a fault injection-induced leak in rcutils_string_array_fini()?

@brawner brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 422dd65 to 337f9ee Compare August 31, 2020 22:00
@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch from a7af61a to 81d411c Compare August 31, 2020 22:04
@brawner brawner mentioned this pull request Sep 1, 2020
Closed
@brawner brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 68fb9cc to 37cac47 Compare September 1, 2020 23:16
@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch from 81d411c to 0446196 Compare September 1, 2020 23:22
@brawner
Copy link
Contributor Author

brawner commented Sep 1, 2020

@hidmic Yes I believe that's correct. It might be harder to address since we need those injected faults for coverage.

hidmic
hidmic approved these changes Sep 2, 2020
View reviewed changes
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.

LGTM !

rcl_yaml_param_parser/test/test_parser.cpp
"string_array_with_quoted_number.yaml"
};

for (auto & filename : filenames) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@brawner nit: can be made const?

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 const is already deduced by auto since filenames is const.

@brawner
Fault injection tests for rcl_yaml
dbf272f 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
PR Feedback
137820b 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
Pause fault injection
c4352a1 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
variant_copy
8ca6f42 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner brawner force-pushed the brawner/rcl_yaml-fault-injection-macros branch from 0446196 to 8ca6f42 Compare September 2, 2020 21:27
@brawner brawner changed the base branch from brawner/rcl_yaml-basic-unit-tests to master September 2, 2020 21:28
@brawner
Copy link
Contributor Author

brawner commented Sep 2, 2020

Rebased onto master after merging #771

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

@brawner brawner merged commit 88ed960 into master Sep 2, 2020
@delete-merged-branch delete-merged-branch bot deleted the brawner/rcl_yaml-fault-injection-macros branch September 2, 2020 21:35
ahcorde pushed a commit that referenced this pull request Oct 8, 2020
@brawner @ahcorde
Add fault-injection unit tests (coverage part 2/3) (#766)
c0f18f1 
* Fault injection tests for rcl_yaml

Signed-off-by: Stephen Brawner <[email protected]>

* PR Feedback

Signed-off-by: Stephen Brawner <[email protected]>

* Pause fault injection

Signed-off-by: Stephen Brawner <[email protected]>

* variant_copy

Signed-off-by: Stephen Brawner <[email protected]>
ahcorde pushed a commit that referenced this pull request Oct 9, 2020
@brawner @ahcorde
Add fault-injection unit tests (coverage part 2/3) (#766)
08bbdde 
* Fault injection tests for rcl_yaml

Signed-off-by: Stephen Brawner <[email protected]>

* PR Feedback

Signed-off-by: Stephen Brawner <[email protected]>

* Pause fault injection

Signed-off-by: Stephen Brawner <[email protected]>

* variant_copy

Signed-off-by: Stephen Brawner <[email protected]>
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.
3 participants
@brawner @hidmic @Blast545