Improve test coverage to 100% functions / 88% lines; automate Windows coverage via OpenCppCoverage#5
Improve test coverage to 100% functions / 88% lines; automate Windows coverage via OpenCppCoverage#5VipinKum4r wants to merge 9 commits intoamoldhamale1105:masterfrom
Conversation
…d generate coverage in build script
| // ============================================================================= | ||
| // Test 1: Direct class coverage (no event-loop lifecycle required) | ||
| // ============================================================================= | ||
| TEST(EventLoopTest, DirectCoverage) |
There was a problem hiding this comment.
We should have more meaningful test case names, a good practice is <func_name>_test_scenario
| { | ||
| EventLoop::Event e("singleArgEvent"); | ||
| EXPECT_EQ(e.getName(), "singleArgEvent"); | ||
| EXPECT_EQ(e.getData(), nullptr); | ||
| } |
There was a problem hiding this comment.
Please create separate test cases for each of these scenarios and use proper naming convention as commented above
| // the multiset to invoke EventCompare::operator() to maintain ordering. | ||
| { | ||
| EventSender sender; | ||
| auto now = std::chrono::system_clock::now(); | ||
| EventLoop::Event* e1 = new EventLoop::Event("scheduled1"); | ||
| EventLoop::Event* e2 = new EventLoop::Event("scheduled2"); | ||
| sender.addScheduledEvent(e1, now + std::chrono::milliseconds(200)); | ||
| // Earlier timestamp forces a comparison against e1 during insertion | ||
| sender.addScheduledEvent(e2, now + std::chrono::milliseconds(50)); | ||
| EXPECT_FALSE(sender.eventScheduleEmpty()); | ||
|
|
||
| // Drain the queue to avoid leaks | ||
| while (!sender.eventScheduleEmpty()) { | ||
| EventLoop::Event* evt = sender.nextEventSchedule().first; | ||
| delete evt; | ||
| sender.removeEventSchedule(); | ||
| } | ||
| EXPECT_TRUE(sender.eventScheduleEmpty()); | ||
| } |
There was a problem hiding this comment.
This can be a separate test case
| while (!sender.eventScheduleEmpty()) { | ||
| EventLoop::Event* evt = sender.nextEventSchedule().first; | ||
| delete evt; | ||
| sender.removeEventSchedule(); | ||
| } |
There was a problem hiding this comment.
Instead of this, create all events you need in test fixtureSetUp() and ensure memory deallocation in TearDown()
| }); | ||
| // ── Error paths: empty event name guards (before Run) ──────────────────── | ||
| // Each call hits the `if (evtName.empty())` true branch, prints to stderr, | ||
| // and returns early – covering lines 49-50, 66-67, 75-76, 85-86 of EventLoop.cpp. |
There was a problem hiding this comment.
We probably don't need to add all these coverage details in comments here, the coverage report should speak for itself.
| * - EventManager::start() and eventScheduler() catch blocks – only reachable if OS-level | ||
| * thread creation fails (resource exhaustion). |
There was a problem hiding this comment.
I think this can be orchestrated via mock functions for thread creation that throw std::thread class exceptions.
| * - EventManager::start() and eventScheduler() catch blocks – only reachable if OS-level | ||
| * thread creation fails (resource exhaustion). | ||
| * - EventManager destructor line 45 (stop()) – requires the loop to be alive at program | ||
| * exit, which conflicts with safe test teardown. |
There was a problem hiding this comment.
Didn't get what you mean here by 'safe test teardown'
I think this should be testable if you simulate a sequence where program exits on a non-blocking event loop without calling EventLoop::Halt() in which case the EventManager destructor should detect m_shutdown as true and call stop()
| * - EventManager line 149 (direct eventLoop() BLOCK path) – the NON_BLOCK path on | ||
| * line 151 is exercised instead. Testing both modes in one binary is unsafe because | ||
| * the static EventManager singleton cannot be restarted after Halt() without joining | ||
| * the previous scheduler thread. |
There was a problem hiding this comment.
why not? Have a separate test case for this at the end, call halt in another thread. This should be fairly easy. Nothing is unsafe in unit testing :)
| * @file EventLoopBasicTest.cpp | ||
| * @brief Comprehensive unit tests targeting 100% function coverage and maximum line coverage. | ||
| * | ||
| * Coverage strategy: |
There was a problem hiding this comment.
Change this description once you split into multiple test cases. Also I would suggest adding a comment above every test case instead at the top of the file here which would be easier to read.
| @@ -1,58 +1,239 @@ | |||
| /** | |||
| * @file EventLoopBasicTest.cpp | |||
| * @brief Comprehensive unit tests targeting 100% function coverage and maximum line coverage. | |||
There was a problem hiding this comment.
No coverage metric info here please, this will be available in the report
2 participants
Improve coverage to 100% functions / ~89% lines and automate Windows coverage report
What this PR does
Builds on the test foundation from #4 to push coverage as high as the library's design permits, and adds automated coverage report generation on Windows via OpenCppCoverage.
Coverage results
These are defensive/safety net code paths — their presence is valuable for production robustness even if they cannot be exercised by unit tests.
build.shchangesThe
-tflag (test + coverage) now auto-detects the platform:lcov+genhtmlflow (unchanged)OpenCppCoverage, producing both an HTML report and a Cobertura XML report suitable for CIIf
OpenCppCoverageis not onPATH, the script prints a clear install instruction (winget install OpenCppCoverage) and exits gracefully instead of failing the build..gitignoreupdateAdded
CoverageReport-*/to ignore the timestamped report folders OpenCppCoverage generates in the working directory.