Increase unit test coverage - #219
Conversation
📝 WalkthroughWalkthroughRemoves Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/PropertiesPanelController_test.cpp`:
- Around line 109-119: The test waits for
PropertiesPanelController::themeChanged after calling app->setPalette() but does
not process the event queue, causing intermittent failures in headless
environments; after the call to app->setPalette() in the
TEST_F(PropertiesPanelControllerTests, PaletteChangeEmitsThemeChanged) test,
call app->processEvents() to flush pending events so the QSignalSpy watching the
themeChanged signal can observe the emitted signal before the EXPECT_GE
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3db8cd2-38de-4c71-a14a-de563163270f
📒 Files selected for processing (4)
CMakeLists.txtsrc/PropertiesPanelController_test.cppsrc/ThemeManager_test.cpptests/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/PropertiesPanelController_test.cpp (2)
57-59: Add a null assertion in helper for clearer failures.If node creation fails,
selectOne(node)obscures the root cause. Assertnode != nullptrinsidecreateSelectedNode.Proposed patch
Ogre::SceneNode* createSelectedNode(const QString& name) { Ogre::SceneNode* node = Manager::getSingleton()->addSceneNode(name); + EXPECT_NE(node, nullptr); SelectionSet::getSingleton()->selectOne(node); return node; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController_test.cpp` around lines 57 - 59, The helper createSelectedNode should assert the created node is non-null before calling SelectionSet::getSingleton()->selectOne(node); update createSelectedNode to check the result of Manager::getSingleton()->addSceneNode(name) (e.g., using an appropriate test assertion or assert(node != nullptr)) and return or fail immediately if null so selectOne is never called with a null pointer; ensure the assertion references the local variable node so failures point to node creation.
30-30: Replace fixed-duration sleeps with deterministic event-driven synchronization.
QThread::msleep(50)afterkill()operations is inefficient and can cause flaky tests across CI machines. Thekill()implementations (PropertiesPanelController::kill(),TransformOperator::kill(),SelectionSet::kill(),Manager::kill()) are all synchronous (direct deletion with no async patterns), so these sleeps do not wait for asynchronous cleanup. If the sleep is needed for Qt event loop or widget teardown side effects, use explicitapp->processEvents()loops with deterministic conditions instead of arbitrary timing.Applies to lines: 30, 52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController_test.cpp` at line 30, Replace the fixed QThread::msleep(50) waits with deterministic event-driven synchronization: remove the sleep at the test locations and, after calling PropertiesPanelController::kill() (and any downstream TransformOperator::kill(), SelectionSet::kill(), Manager::kill() calls), run the Qt event loop until a clear condition is met (e.g. widget destroyed, controller pointer null, or a specific ready/finished flag) by repeatedly calling app->processEvents() with a short timeout and checking that condition; ensure you use the synchronous kill() semantics (no async wait) to assert immediate cleanup or loop until the GUI teardown condition is observed instead of sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/PropertiesPanelController_test.cpp`:
- Around line 99-100: After calling app->setPalette(palette) the test must flush
Qt's event loop so the paletteChanged signal is processed and
PropertiesPanelController::themeChanged() runs; add a call to
app->processEvents() immediately after app->setPalette(palette) so the signal
handlers execute before asserting controller->panelColor() (references:
app->setPalette, app->processEvents, PropertiesPanelController::themeChanged,
paletteChanged signal, controller->panelColor()).
---
Nitpick comments:
In `@src/PropertiesPanelController_test.cpp`:
- Around line 57-59: The helper createSelectedNode should assert the created
node is non-null before calling SelectionSet::getSingleton()->selectOne(node);
update createSelectedNode to check the result of
Manager::getSingleton()->addSceneNode(name) (e.g., using an appropriate test
assertion or assert(node != nullptr)) and return or fail immediately if null so
selectOne is never called with a null pointer; ensure the assertion references
the local variable node so failures point to node creation.
- Line 30: Replace the fixed QThread::msleep(50) waits with deterministic
event-driven synchronization: remove the sleep at the test locations and, after
calling PropertiesPanelController::kill() (and any downstream
TransformOperator::kill(), SelectionSet::kill(), Manager::kill() calls), run the
Qt event loop until a clear condition is met (e.g. widget destroyed, controller
pointer null, or a specific ready/finished flag) by repeatedly calling
app->processEvents() with a short timeout and checking that condition; ensure
you use the synchronous kill() semantics (no async wait) to assert immediate
cleanup or loop until the GUI teardown condition is observed instead of
sleeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b41e415-b6d8-4668-92b5-518681941cd6
📒 Files selected for processing (1)
src/PropertiesPanelController_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/PropertiesPanelController_test.cpp (1)
171-173: Consider tightening the rotation tolerance.A tolerance of 20.0 degrees is quite permissive—rotations could be significantly off and still pass. If quaternion-to-Euler conversion instabilities are a concern, consider documenting this choice or narrowing the tolerance to ~1.0 degree for more precise regression detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController_test.cpp` around lines 171 - 173, Tighten the rotation assertions by replacing the very permissive 20.0f tolerance with a stricter ~1.0f tolerance on the EXPECT_NEAR checks for controller->rotX(), controller->rotY(), and controller->rotZ(); if quaternion-to-Euler instability is a known issue, either add a short comment above the three checks explaining why a larger tolerance would be required or keep the stricter 1.0f and add a TODO referencing the conversion instability for future investigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/PropertiesPanelController_test.cpp`:
- Around line 171-173: Tighten the rotation assertions by replacing the very
permissive 20.0f tolerance with a stricter ~1.0f tolerance on the EXPECT_NEAR
checks for controller->rotX(), controller->rotY(), and controller->rotZ(); if
quaternion-to-Euler instability is a known issue, either add a short comment
above the three checks explaining why a larger tolerance would be required or
keep the stricter 1.0f and add a TODO referencing the conversion instability for
future investigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7eefa14-4402-4079-bab9-7b7bf98603d1
📒 Files selected for processing (2)
src/EditorViewport.cppsrc/PropertiesPanelController_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/PropertiesPanelController_test.cpp`:
- Around line 50-55: The helper createSelectedNode currently uses
EXPECT_NE(node, nullptr) which is non-fatal; change it to ASSERT_NE(node,
nullptr) so the test aborts immediately if
Manager::getSingleton()->addSceneNode(name) returns null, preventing
SelectionSet::getSingleton()->selectOne(node) from being called with a null
pointer and avoiding downstream crashes; update the assertion in
createSelectedNode accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c68876d-6855-4c85-9884-84aea575d797
📒 Files selected for processing (1)
src/PropertiesPanelController_test.cpp
| Ogre::SceneNode* createSelectedNode(const QString& name) | ||
| { | ||
| Ogre::SceneNode* node = Manager::getSingleton()->addSceneNode(name); | ||
| EXPECT_NE(node, nullptr); | ||
| SelectionSet::getSingleton()->selectOne(node); | ||
| return node; |
There was a problem hiding this comment.
Use ASSERT_NE in createSelectedNode to prevent null-path crashes
EXPECT_NE is non-fatal here; if node creation fails, the helper still selects/returns a null pointer and downstream tests may crash on dereference. Make this check fatal in the helper.
Proposed fix
Ogre::SceneNode* createSelectedNode(const QString& name)
{
Ogre::SceneNode* node = Manager::getSingleton()->addSceneNode(name);
- EXPECT_NE(node, nullptr);
+ ASSERT_NE(node, nullptr);
SelectionSet::getSingleton()->selectOne(node);
return node;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PropertiesPanelController_test.cpp` around lines 50 - 55, The helper
createSelectedNode currently uses EXPECT_NE(node, nullptr) which is non-fatal;
change it to ASSERT_NE(node, nullptr) so the test aborts immediately if
Manager::getSingleton()->addSceneNode(name) returns null, preventing
SelectionSet::getSingleton()->selectOne(node) from being called with a null
pointer and avoiding downstream crashes; update the assertion in
createSelectedNode accordingly.
|
1 participant
Summary
Verification
Summary by CodeRabbit
Tests
Chores
Refactor