Improve unit test coverage from master - #221
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThree new test cases were added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
fe6d94d to
a6627e0
Compare
![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
🧹 Nitpick comments (1)
src/PropertiesPanelController_test.cpp (1)
174-176: Consider documenting the large rotation tolerance.The 20-degree tolerance for Euler angle verification is significant. While the comment explains quaternion round-trip instability, it would help future maintainers to understand if this is a known limitation or if tighter bounds could be achieved.
📝 Optional: Add more context to the comment
- // TransformOperator exposes Euler angles derived from Ogre quaternions. - // The round-trip is not stable enough here for tight per-axis equality. + // TransformOperator exposes Euler angles derived from Ogre quaternions. + // The round-trip is not stable due to gimbal lock and quaternion-to-Euler + // ambiguity near certain orientations. A 20° tolerance accommodates this. EXPECT_NEAR(controller->rotX(), 15.0f, 20.0f);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController_test.cpp` around lines 174 - 176, Document the large 20-degree tolerance used in the Euler angle assertions (EXPECT_NEAR(controller->rotX(), 15.0f, 20.0f), EXPECT_NEAR(controller->rotY(), 25.0f, 20.0f), EXPECT_NEAR(controller->rotZ(), 35.0f, 20.0f)) by adding a concise comment above these lines that explains this is an accepted/known limitation due to quaternion↔Euler round-trip instability (and whether it is expected to be fixed later or is an inherent limitation), and optionally note that tighter bounds may be possible if tests change to compare quaternions directly or use a canonicalization step.
🤖 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/MaterialComboDelegate_test.cpp`:
- Around line 149-152: The test is flaky when comboBox->count() == 1 because
setCurrentIndex(0) won't change the index and won't emit signals; modify the
test to force an actual index change: if comboBox->count() > 1 call
comboBox->setCurrentIndex(1) as before, otherwise insert a temporary item with
comboBox->insertItem(0, "tmp"), call comboBox->setCurrentIndex(1) to trigger
currentIndexChanged, then remove the temporary item with
comboBox->removeItem(0); keep assertions on commitSpy.count() and
closeSpy.count() unchanged.
---
Nitpick comments:
In `@src/PropertiesPanelController_test.cpp`:
- Around line 174-176: Document the large 20-degree tolerance used in the Euler
angle assertions (EXPECT_NEAR(controller->rotX(), 15.0f, 20.0f),
EXPECT_NEAR(controller->rotY(), 25.0f, 20.0f), EXPECT_NEAR(controller->rotZ(),
35.0f, 20.0f)) by adding a concise comment above these lines that explains this
is an accepted/known limitation due to quaternion↔Euler round-trip instability
(and whether it is expected to be fixed later or is an inherent limitation), and
optionally note that tighter bounds may be possible if tests change to compare
quaternions directly or use a canonicalization step.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa994681-4e01-4849-8fb1-92d7217a0b54
📒 Files selected for processing (6)
CMakeLists.txtsrc/EditorViewport.cppsrc/MaterialComboDelegate_test.cppsrc/PropertiesPanelController_test.cppsrc/ThemeManager_test.cpptests/CMakeLists.txt
| comboBox->setCurrentIndex(comboBox->count() > 1 ? 1 : 0); | ||
|
|
||
| EXPECT_EQ(commitSpy.count(), 1); | ||
| EXPECT_EQ(closeSpy.count(), 1); |
There was a problem hiding this comment.
Potential flaky test when combo has only one material.
If comboBox->count() is 1, line 149 evaluates to setCurrentIndex(0). Since QComboBox initializes with currentIndex of 0 when items exist, this won't trigger currentIndexChanged, causing the assertions on lines 151-152 to fail.
🔧 Proposed fix to ensure index change
- comboBox->setCurrentIndex(comboBox->count() > 1 ? 1 : 0);
+ ASSERT_GE(comboBox->count(), 2) << "Need at least 2 materials for signal test";
+ comboBox->setCurrentIndex(1);Alternatively, if you want to handle single-material scenarios:
+ if (comboBox->count() < 2) {
+ GTEST_SKIP() << "Skipping signal test: need at least 2 materials";
+ }
comboBox->setCurrentIndex(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| comboBox->setCurrentIndex(comboBox->count() > 1 ? 1 : 0); | |
| EXPECT_EQ(commitSpy.count(), 1); | |
| EXPECT_EQ(closeSpy.count(), 1); | |
| ASSERT_GE(comboBox->count(), 2) << "Need at least 2 materials for signal test"; | |
| comboBox->setCurrentIndex(1); | |
| EXPECT_EQ(commitSpy.count(), 1); | |
| EXPECT_EQ(closeSpy.count(), 1); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialComboDelegate_test.cpp` around lines 149 - 152, The test is flaky
when comboBox->count() == 1 because setCurrentIndex(0) won't change the index
and won't emit signals; modify the test to force an actual index change: if
comboBox->count() > 1 call comboBox->setCurrentIndex(1) as before, otherwise
insert a temporary item with comboBox->insertItem(0, "tmp"), call
comboBox->setCurrentIndex(1) to trigger currentIndexChanged, then remove the
temporary item with comboBox->removeItem(0); keep assertions on
commitSpy.count() and closeSpy.count() unchanged.
|
1 participant
Summary:
Verification:
Summary by CodeRabbit