Skip to content

Conversation

@brawner
Copy link
Contributor

@brawner brawner commented Jun 8, 2020

It looks like OutputFileHandler was the only feature that is not currently tested in this package. This adds a test for that, and also basic stdout/stderr smoke tests.

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

Signed-off-by: Stephen Brawner <brawner@gmail.com>
Comment on lines 91 to 94
CONSOLE_BRIDGE_logDebug("Testing Log");
CONSOLE_BRIDGE_logInform("Testing Log");
CONSOLE_BRIDGE_logWarn("Testing Log");
CONSOLE_BRIDGE_logError("Testing Log");
Copy link
Contributor

Choose a reason for hiding this comment

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

Other than coverage, this isn't a super helpful test as there are no ASSERT/EXPECT here. Most of the point of the OutputHandlerString class is so we can capture these and test against them. So with that in mind, I'd suggest just reusing the OutputHandlerString class here, and calling it at the various levels.

Alternatively, if you want to test the default handler for stdout, you can do something like #74.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is just to add the default handler code path to the test file. I don't have a good solution to capturing stdout/stderr for testing purposes. I just ran into this issue with ros2/rcutils#258. It looks like it is currently not recommended to use testing::internal::CaptureStdout() since it's in an internal namespace.

I've added expect_no_throw statements for now. I'm not yet convinced it's actually adding coverage. I'll remove the test if it doesn't actually affect coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this test adds a couple of lines and bumps coverage over 95% with setLogLevel to debug.

@brawner brawner force-pushed the brawner/console_bridge-test-coverage branch from 8c9b652 to 58513f9 Compare June 8, 2020 22:03
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #84 into master will increase coverage by 33.33%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #84       +/-   ##
===========================================
+ Coverage   65.00%   98.33%   +33.33%     
===========================================
  Files           2        2               
  Lines          60       60               
===========================================
+ Hits           39       59       +20     
+ Misses         21        1       -20     
Impacted Files Coverage Δ
src/console.cpp 98.21% <0.00%> (+35.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a64d37...63c6fea. Read the comment docs.

@brawner brawner force-pushed the brawner/console_bridge-test-coverage branch from 58513f9 to 8f5f5ab Compare June 8, 2020 22:05
@brawner
Copy link
Contributor Author

brawner commented Jun 8, 2020

The StdoutStderrOutput now adds the two other lines of coverage by exercising one more path in the OutputStdHandler. 98% coverage! The only thing that remains would be mocking a failing fclose().

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

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few minor nits, but I'll approve anyway.

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

this looks good, but I was just tinkering with the test and re-arranged the order of the test cases in scpeters@3e5db76 and now one of them fails on my mac

[----------] 3 tests from FileHandlerTest
[ RUN      ] FileHandlerTest.TestErrorLogs
/Users/scpeters/clone/console_bridge/test/console_TEST.cc:300: Failure
Expected: (result.find(expected_text)) != (result.npos), actual: 18446744073709551615 vs 18446744073709551615
Log file did not contain expected text, instead it contained:

: 
[  FAILED  ] FileHandlerTest.TestErrorLogs (3 ms)
[ RUN      ] FileHandlerTest.TestInformDoesntLog
[       OK ] FileHandlerTest.TestInformDoesntLog (0 ms)
[ RUN      ] FileHandlerTest.TestInformLogsWithLogLevel
[       OK ] FileHandlerTest.TestInformLogsWithLogLevel (1 ms)
[----------] 3 tests from FileHandlerTest (4 ms total)

do you have any thoughts?

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

brawner commented Jun 12, 2020

Addressed initialization issue. It looks like DOH is only initialized once in the whole test file.

@scpeters scpeters merged commit 3249f1d into ros:master Jun 15, 2020
@chapulina chapulina mentioned this pull request Jul 16, 2020
22 tasks
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.

4 participants