-
Notifications
You must be signed in to change notification settings - Fork 102
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
Increase testing coverage of rcutils to 95% #258
Conversation
52d56a5
to
62ed0ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments here and there. Overall looks good
Separate PR to fix Windows bug: |
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
938b03f
to
1aef73c
Compare
Signed-off-by: Stephen Brawner <[email protected]>
After merging #259, I've rebased this PR. Also for API considerations I moved the time_bomb allocator into the test directory. We can leave that discussion for post-foxy discussions. |
Also without #257 this shows up as only 94%. At 1666/1763, it just needs one more line of coverage or one less line of uncovered code 🤷♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, otherwise LGTM.
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
|
||
// There are no outputs of the handler function, and the only result are fprintf() calls. | ||
// This is just a smoke test to check that the code can handle simple inputs cleanly. | ||
TEST(TestLoggingConsoleOutputHandler, typical_inputs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brawner meta: thinking again about this test, you could mock stdout
and/or stderr
for the duration of the test. Won't block on it, but it is doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this out and couldn't get it to work. Assigning a new file pointer to stdout/stderr left me with blank files. Alternatively, using freopen()
worked but it wouldn't let me reset stdout/stderr back in an easily portable way.
I like the idea of checking expected input on stdout/stderr, but I'll need a better solution, which may have to come later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting. Oh well, it can be done in a follow-up PR.
|
||
// There are no outputs of the handler function, and the only result are fprintf() calls. | ||
// This is just a smoke test to check that the code can handle bad inputs cleanly. | ||
TEST(TestLoggingConsoleOutputHandler, bad_inputs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brawner same as above.
I've added a bit more test cases to the unit tests in rcutils to boost the coverage to 95% (just barely).
The big question about this PR is the desired location of
time_bomb_allocator
. @hidmic had suggested putting it into a centralized location so it can be used in other packages. I'm thinking I might want to do that for the allocator_testing_utils.h as well. But I don't think rcutils/include is the desired end location.The remaining 5% of untested logic pretty much consists of failure checks that won't fail, generally because bad parameters have already been sanitized by the time the function under test calls a function that is checked for failure. In a few cases it might be because the system call cannot be made to fail reliably or even at all in a simple unit test.
Signed-off-by: Stephen Brawner [email protected]