Expand MainWindow and TransformOperator coverage - #228
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds extensive unit tests to TransformOperator and MainWindow covering gizmo visibility and transform-state transitions, orientation/scale propagation for node/entity selections, drag-and-drop and recent-files UI behavior, MCP server lifecycle, and small test helpers with environment-based skips. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
🤖 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/mainwindow_test.cpp`:
- Around line 87-95: The helper createAnimatedEntity currently contains
EXPECT_NE(entity, nullptr) which can record a test failure before callers have a
chance to call GTEST_SKIP; remove that assertion from createAnimatedEntity so it
simply returns nullptr when mesh loading or createAnimatedTestEntity fails (the
helper already checks canLoadMeshFiles and returns nullptr). Update callers to
check the returned pointer and call GTEST_SKIP() (or perform the EXPECT_NE
assertion) after skipping logic, or alternatively centralize skip behavior
inside createAnimatedEntity by detecting nullptr and calling GTEST_SKIP() there
instead of asserting; use the function names createAnimatedEntity,
canLoadMeshFiles, and createAnimatedTestEntity to locate and change the code.
In `@src/TransformOperator_test.cpp`:
- Around line 459-470: The test TEST_F(TransformOperatorTests,
EntityQuaternionRotationAccumulatesTrackedEulerDelta) only applies one
rotateSelected(Ogre::Quaternion...) so it can’t prove accumulation; update the
test to either seed the tracked rotation to a non-zero value via
SelectionSet::getSingleton()->setEntityRotation(...) (or equivalent) before
calling op->rotateSelected, or call op->rotateSelected(...) twice with known
quaternion deltas, then read the tracked Euler via
SelectionSet::getSingleton()->getEntityRotation(entity) and assert the tracked.y
equals the sum of the applied deltas (with the same EXPECT_NEAR tolerance) to
verify accumulation rather than overwrite.
🪄 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: b4b53a1f-fd0a-42db-b028-20d627a1c46e
📒 Files selected for processing (2)
src/TransformOperator_test.cppsrc/mainwindow_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/TransformOperator_test.cpp (1)
480-492: Consider adding assertions for x and z components.The test now correctly proves accumulation by seeding an initial rotation value. For completeness, you could also verify that the x and z components remain unchanged (at their initial values) to ensure the Y-axis rotation doesn't leak into other axes.
♻️ Suggested enhancement
const Ogre::Vector3 tracked = SelectionSet::getSingleton()->getEntityRotation(entity); + EXPECT_NEAR(tracked.x, 0.0f, 0.5f); EXPECT_NEAR(tracked.y, 30.0f, 0.5f); + EXPECT_NEAR(tracked.z, 0.0f, 0.5f); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator_test.cpp` around lines 480 - 492, Add assertions to verify x and z components remain unchanged after Y-axis rotation in the TEST_F TransformOperatorTests::EntityQuaternionRotationAccumulatesTrackedEulerDelta test: after calling SelectionSet::getSingleton()->setEntityRotation(...) and op->rotateSelected(...), check the tracked vector from SelectionSet::getSingleton()->getEntityRotation(entity) and add EXPECT_NEAR checks for tracked.x and tracked.z to be 0.0f (use the same tolerance as the existing y check, e.g., 0.5f) so the test ensures no unintended rotation leakage into X or Z.
🤖 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/TransformOperator_test.cpp`:
- Around line 480-492: Add assertions to verify x and z components remain
unchanged after Y-axis rotation in the TEST_F
TransformOperatorTests::EntityQuaternionRotationAccumulatesTrackedEulerDelta
test: after calling SelectionSet::getSingleton()->setEntityRotation(...) and
op->rotateSelected(...), check the tracked vector from
SelectionSet::getSingleton()->getEntityRotation(entity) and add EXPECT_NEAR
checks for tracked.x and tracked.z to be 0.0f (use the same tolerance as the
existing y check, e.g., 0.5f) so the test ensures no unintended rotation leakage
into X or Z.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e0a8b26-9415-4a61-a736-241bdb63d873
📒 Files selected for processing (1)
src/TransformOperator_test.cpp
|
1 participant
Summary
Testing
Summary by CodeRabbit
Bug Fixes / UI Behavior
Tests