Feature/test cov - #20
Merged
Merged
Conversation
- Exclude vendored stb_image.h from lcov and codecov - Run coverage tests under xvfb-run so display-dependent tests work on CI - Add ImGuiContextFixture/ImGuiFrameFixture test helpers enabling headless ImGui tests with no GPU backend, plus a smoke test proving they work - Link implot to the test target for upcoming TemplateWindow tests Co-Authored-By: Claude <noreply@anthropic.com>
…ameter edge cases and TemplateWindow (issue #12): calls all 36 theme appliers directly with exact colour spot-checks, exercises StyleParameters::apply() dispatch while pinning its documented non-theme-field limitation, drives LogWindow/TemplateWindow render paths and the theme selector via the headless ImGui frame fixture, and adds WindowParameters load()/save() edge-case coverage (malformed INI values, partial files, negative values bypassing validation). Coverage goes from 96 to 141 tests (839 assertions), all green under both a single binary run and ctest's per-case process isolation. Co-Authored-By: Claude <noreply@anthropic.com>
Jgocunha
added
enhancement
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a suite of new Catch2 tests for imgui-platform-kit covering LogWindow, StyleParameters, TemplateWindow, ThemeSelector, and theme appliers, plus shared ImGui test fixtures and WindowParameters edge-case tests. It also updates the tests CMake build to link implot, and adjusts coverage CI/config to exclude vendored stb_image.h and run tests under xvfb. ChangesTest infrastructure and new test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a substantial set of headless Catch2 tests that create an ImGui (and when needed ImPlot) context/frame so previously unreachable UI/style code paths can be exercised in CI, and updates the coverage workflow/configuration to support these tests.
Changes:
- Introduces shared ImGui context/frame fixtures and multiple new test suites covering theme appliers, StyleParameters::apply(), LogWindow rendering, theme selector rendering, WindowParameters IO edge cases, and TemplateWindow rendering.
- Updates the test CMake target to include the new tests and link against ImPlot.
- Adjusts Linux coverage CI to run under Xvfb and excludes vendored
stb_image.hfrom coverage reporting.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| imgui-platform-kit/tests/test_helpers.h | Adds reusable ImGui context/frame fixtures for headless UI tests. |
| imgui-platform-kit/tests/test_fixture_smoke.cpp | Adds smoke coverage validating the fixtures work end-to-end. |
| imgui-platform-kit/tests/test_themes_apply.cpp | Adds direct and map-driven theme applier execution + idempotence/leak checks. |
| imgui-platform-kit/tests/test_style_parameters_apply.cpp | Adds tests validating StyleParameters::apply() dispatch and current documented limitations. |
| imgui-platform-kit/tests/test_log_window_render.cpp | Adds headless rendering coverage for LogWindow across multiple scenarios. |
| imgui-platform-kit/tests/test_theme_selector.cpp | Adds headless rendering coverage for the theme selector’s reachable paths. |
| imgui-platform-kit/tests/test_parameters_edge_cases.cpp | Adds IO edge-case tests for WindowParameters/UserInterfaceParameters. |
| imgui-platform-kit/tests/test_template_window.cpp | Adds headless coverage for TemplateWindow rendering via base-class interface, including ImPlot context setup. |
| imgui-platform-kit/tests/CMakeLists.txt | Registers new test sources and links implot. |
| .github/workflows/coverage.yml | Runs tests under Xvfb for coverage and excludes stb_image from lcov output. |
| .codecov.yml | Excludes vendored stb_image header from Codecov and documents future target raising. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
imgui-platform-kit/tests/test_style_parameters_apply.cpp (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
colorsEqualintotest_helpers.h.This helper is duplicated verbatim in all four new test files (
test_style_parameters_apply.cpp,test_themes_apply.cpp,test_template_window.cpp,test_theme_selector.cpp). Moving it to the sharedtest_helpers.hheader eliminates the copies and prevents drift if the comparison logic ever needs to change (e.g., switching to approximate float comparison).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@imgui-platform-kit/tests/test_style_parameters_apply.cpp` around lines 23 - 26, The colorsEqual helper is duplicated across multiple test files, so move the shared comparison logic into test_helpers.h and update the test files to use that shared function instead of keeping local copies. Locate the helper by its colorsEqual name and place it alongside other shared test utilities so test_style_parameters_apply.cpp, test_themes_apply.cpp, test_template_window.cpp, and test_theme_selector.cpp all reference the same implementation.imgui-platform-kit/tests/test_parameters_edge_cases.cpp (1)
143-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
TempIniFilefor consistent RAII cleanup in remaining tests.Three tests manually manage temp files with
fs::removeat the end. If an assertion fails mid-test, cleanup is skipped and temp files leak.TempIniFilewas created specifically to guarantee cleanup even on assertion failure. Create it with empty content —save()truncates and overwrites anyway.♻️ Proposed refactor for title round-trip test (lines 143-157)
TEST_CASE("WindowParameters: title containing spaces and '=' round-trips exactly", "[parameters][window][io][edge]") { - const std::string path = (fs::temp_directory_path() / "ipk_edge_weird_title.ini").string(); - fs::remove(path); + TempIniFile ini("ipk_edge_weird_title.ini", ""); + const std::string& path = ini.str(); const WindowParameters saved("My = Weird == Title", 1280, 720, 1, 2); saved.save(path); @@ -154,7 +151,5 @@ loaded.load(path); CHECK(loaded.title == "My = Weird == Title"); - - fs::remove(path); }♻️ Proposed refactor for save() determinism test (lines 182-203)
TEST_CASE("WindowParameters: save() is deterministic and writes exactly 6 lines", "[parameters][window][io][edge]") { - const std::string pathA = (fs::temp_directory_path() / "ipk_edge_det_a.ini").string(); - const std::string pathB = (fs::temp_directory_path() / "ipk_edge_det_b.ini").string(); - fs::remove(pathA); - fs::remove(pathB); + TempIniFile iniA("ipk_edge_det_a.ini", ""); + TempIniFile iniB("ipk_edge_det_b.ini", ""); fs::remove(defaultWindowParametersFilename); const WindowParameters p("Deterministic", 1024, 768, 3, 4); - p.save(pathA); - p.save(pathB); + p.save(iniA.str()); + p.save(iniB.str()); - const std::string contentA = readFile(pathA); - const std::string contentB = readFile(pathB); + const std::string contentA = readFile(iniA.str()); + const std::string contentB = readFile(iniB.str()); CHECK(contentA == contentB); @@ -199,7 +196,5 @@ const auto lineCount = std::count(contentA.begin(), contentA.end(), '\n'); CHECK(lineCount == 6); - - fs::remove(pathA); - fs::remove(pathB); }♻️ Proposed refactor for UserInterfaceParameters delegation test (lines 218-232)
TEST_CASE("UserInterfaceParameters: save() with a custom filename delegates to WindowParameters::save()", "[parameters][io][edge]") { - const std::string path = (fs::temp_directory_path() / "ipk_edge_uiparams.ini").string(); - fs::remove(path); + TempIniFile ini("ipk_edge_uiparams.ini", ""); const UserInterfaceParameters params(WindowParameters("Delegated", 800, 600, 0, 0)); - params.save(path); + params.save(ini.str()); - REQUIRE(fs::exists(path)); - const std::string content = readFile(path); + REQUIRE(fs::exists(ini.str())); + const std::string content = readFile(ini.str()); CHECK(content.rfind("[Window Parameters]", 0) == 0); CHECK(content.find("Title=Delegated") != std::string::npos); - - fs::remove(path); }Also applies to: 182-203, 218-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@imgui-platform-kit/tests/test_parameters_edge_cases.cpp` around lines 143 - 157, The tests in WindowParameters save/load still manually create and remove temp INI files, so cleanup is skipped if an assertion fails. Replace the ad hoc fs::temp_directory_path/fs::remove pattern in this WindowParameters test with TempIniFile so RAII always removes the file, and use an empty initial payload since save() overwrites it anyway. Keep the test logic focused on the round-trip check and update the related save() determinism and UserInterfaceParameters delegation tests in the same way by locating the existing TempIniFile helper and the WindowParameters::save/load usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@imgui-platform-kit/tests/test_parameters_edge_cases.cpp`:
- Line 29: The TempIniFile destructor currently calls fs::remove(path) in a
noexcept destructor context, which can throw and trigger std::terminate. Update
the TempIniFile destructor to use the filesystem remove overload that takes an
error_code (or otherwise swallow/report failures without throwing), and keep the
cleanup logic inside the destructor non-throwing.
In `@imgui-platform-kit/tests/test_themes_apply.cpp`:
- Around line 209-222: The theme idempotence test uses std::pair class template
argument deduction in the applier list, which deduces an invalid function-type
second element and breaks compilation. Update the loop in test_themes_apply.cpp
to use the existing NamedApplier type or otherwise make the second member an
explicit function pointer type, and keep the applier.second() calls unchanged so
the test logic stays the same.
---
Nitpick comments:
In `@imgui-platform-kit/tests/test_parameters_edge_cases.cpp`:
- Around line 143-157: The tests in WindowParameters save/load still manually
create and remove temp INI files, so cleanup is skipped if an assertion fails.
Replace the ad hoc fs::temp_directory_path/fs::remove pattern in this
WindowParameters test with TempIniFile so RAII always removes the file, and use
an empty initial payload since save() overwrites it anyway. Keep the test logic
focused on the round-trip check and update the related save() determinism and
UserInterfaceParameters delegation tests in the same way by locating the
existing TempIniFile helper and the WindowParameters::save/load usage.
In `@imgui-platform-kit/tests/test_style_parameters_apply.cpp`:
- Around line 23-26: The colorsEqual helper is duplicated across multiple test
files, so move the shared comparison logic into test_helpers.h and update the
test files to use that shared function instead of keeping local copies. Locate
the helper by its colorsEqual name and place it alongside other shared test
utilities so test_style_parameters_apply.cpp, test_themes_apply.cpp,
test_template_window.cpp, and test_theme_selector.cpp all reference the same
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8c709ee-e119-4cf2-8b11-c42c638c4efe
📒 Files selected for processing (11)
.codecov.yml.github/workflows/coverage.ymlimgui-platform-kit/tests/CMakeLists.txtimgui-platform-kit/tests/test_fixture_smoke.cppimgui-platform-kit/tests/test_helpers.himgui-platform-kit/tests/test_log_window_render.cppimgui-platform-kit/tests/test_parameters_edge_cases.cppimgui-platform-kit/tests/test_style_parameters_apply.cppimgui-platform-kit/tests/test_template_window.cppimgui-platform-kit/tests/test_theme_selector.cppimgui-platform-kit/tests/test_themes_apply.cpp
- Fix std::pair CTAD failure that broke Linux/macOS builds: a raw function name deduces to a function type (not a pointer), which std::pair cannot hold. Reuse the existing NamedApplier struct instead. - Make TempIniFile's constructor/destructor non-throwing (error_code overload of fs::remove) to avoid std::terminate on cleanup failure - Drop unnecessary fs::remove(defaultWindowParametersFilename) calls; every test uses explicit positive dimensions and its own load(), so the shared default INI is never actually read - Extract the duplicated colorsEqual helper into test_helpers.h - Use TempIniFile's RAII cleanup instead of manual fs::remove in the three tests that still did it by hand Co-Authored-By: Claude <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #12
What this PR does
New headless tests using the
ImGuiContextFixture/ImGuiFrameFixturefixtures added earlier,targeting the code paths that were previously entirely untested because nothing created an
ImGui context.
test_themes_apply.cpp— calls all 36applyXxxTheme()functionsdirectly, spot-checks exact colour/rounding literals (verified against source) for 8
representative themes, exercises the full 39-entry
themesmap (validating Light/Dark/Classicagainst ImGui's own built-ins), and checks idempotence and no state leakage across sequential
theme applications. This alone targets ~3,240 previously 0%-covered lines in
themes.cpp.test_style_parameters_apply.cpp— provesStyleParameters::apply()dispatchesto the correct theme for all 39 values, and pins the documented limitation that
windowBgColor/windowPadding/windowRounding/transparency/textColorare stored but neverforwarded to
ImGui::GetStyle()— with a comment flagging that a future fix must deliberatelyinvert this test, not accidentally break it.
test_log_window_render.cpp—renderContent()/draw()with empty, single, 200and 500 log entries, the collapsed-window early-return branch, embedding inside a caller-owned
window, and multi-frame
addLog/clearLogsinterleaving.test_theme_selector.cpp—showImGuiKitThemeSelector()render paths; documentsthat the in-combo selection branch needs simulated input and is unreachable headless.
test_parameters_edge_cases.cpp—WindowParameters::load()/save()edge cases:malformed and overflowing numeric fields (pins the current uncaught
std::stoithrowbehavior), partial files, unknown keys, titles containing
=, negative values bypassingvalidation on load, and save() determinism.
test_template_window.cpp—TemplateWindow::render()via theUserInterfaceWindowbase pointer (it's private on the subclass); required creating anImPlotContextalongside the ImGui one since the window unconditionally callsImPlot::ShowDemoWindow().Verification
ctest(which runs each case as an isolatedprocess) — confirms no cross-test static-state leakage.
Summary by CodeRabbit
Bug Fixes
Tests