Increase coverage for main window and transform paths#223
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds and expands unit tests across three test files: introduces a QTemporaryDir fixture, new MeshImporterExporter importer/sceneImporter edge-case tests, extended TransformOperator behavior and signal tests, and isolated MainWindow UI/persistence tests. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mainwindow_test.cpp (1)
83-149:⚠️ Potential issue | 🟡 MinorHeap-allocated
QKeyEventobjects in direct handler calls require explicit cleanup.When calling
keyPressEvent()directly (not through Qt's event dispatch), the event is not owned by the framework and caller responsibility for cleanup applies. These 15 instances allocate withnewbut never delete, causing memory leaks in tests.Use stack-allocated events with address-of instead:
Fix pattern
- window->keyPressEvent(new QKeyEvent(QEvent::KeyPress, Qt::Key_X, Qt::NoModifier)); + QKeyEvent event(QEvent::KeyPress, Qt::Key_X, Qt::NoModifier); + window->keyPressEvent(&event);This is safe because
keyPressEventis synchronous and returns before the stack frame unwinds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow_test.cpp` around lines 83 - 149, The tests leak heap-allocated QKeyEvent objects because they call window->keyPressEvent(new QKeyEvent(...)) without deleting them; replace each heap allocation in tests (all calls to window->keyPressEvent(...) in MainWindowTest: e.g., tests SetTransformState..., KeyXTogglesTransformSpace, KeyDeleteWithEmptySelection, KeyFFrameSelectionEmptyDoesNotCrash, CycleAllStatesViaKeyboard, UnmappedKeyDoesNotCrash, etc.) with a stack-allocated QKeyEvent local (QKeyEvent ev(...)) and pass its address (window->keyPressEvent(&ev)), since keyPressEvent is synchronous and the stack event will be valid for the call.
🧹 Nitpick comments (2)
src/TransformOperator_test.cpp (1)
132-139: Test name overstates what is actually asserted
TransformStateChangeWithNoSelectionHidesAllGizmoscurrently verifies transform state/tracking only. Either rename it to reflect that scope or add explicit gizmo-visibility assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator_test.cpp` around lines 132 - 139, The test name claims gizmo visibility changes but only asserts transform state and tracking; either rename TEST_F TransformOperatorTests,TransformStateChangeWithNoSelectionHidesAllGizmos to a name that matches the assertions (e.g. TransformStateChangeWithoutSelectionUpdatesStateAndTracking) or extend the test after calling op->onTransformStateChange(TransformOperator::TS_TRANSLATE) to assert gizmo visibility via the appropriate gizmo members/methods on op (add EXPECT_* checks for the translate/rotate/scale gizmo visibility APIs you have in the class) so the name accurately reflects the assertions.src/mainwindow_test.cpp (1)
299-307: Test intent mismatch: asserting transform space does not validate transform state propagation.
setTransformState(...)should be validated via checked actions/state transitions; Line 306 checks transform space, which is orthogonal and may hide regressions inonTransformStateChange.🔧 Suggested assertion adjustment
TEST_F(MainWindowTest, SetTransformStateUpdatesActionsAndOperator) { window->setTransformState(TransformOperator::TS_ROTATE); EXPECT_TRUE(window->ui->actionRotate_Object->isChecked()); EXPECT_FALSE(window->ui->actionSelect_Object->isChecked()); window->setTransformState(TransformOperator::TS_SCALE); EXPECT_TRUE(window->ui->actionScale_Object->isChecked()); - EXPECT_EQ(TransformOperator::getSingleton()->getTransformSpace(), TransformOperator::SPACE_WORLD); + EXPECT_FALSE(window->ui->actionRotate_Object->isChecked()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow_test.cpp` around lines 299 - 307, The test is asserting transform space instead of the transform state propagation; change the final assertion to verify the transform state was updated (and/or the handler ran) by replacing the EXPECT_EQ checking TransformOperator::getTransformSpace() with an assertion that TransformOperator::getSingleton()->getTransformState() == TransformOperator::TS_SCALE (or assert the window UI action state like actionScale_Object->isChecked() if not already), ensuring setTransformState(), onTransformStateChange() and actionScale_Object reflect the TS_SCALE state.
🤖 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 40-43: SetUp() currently calls
QCoreApplication::setOrganizationName("QtMeshEditorTests") and
QCoreApplication::setApplicationName("MainWindowTest") without saving previous
values; capture the originals by reading QCoreApplication::organizationName()
and QCoreApplication::applicationName() into members (e.g., savedOrgName,
savedAppName) in the SetUp() of MainWindowTest, and then in TearDown() restore
them via QCoreApplication::setOrganizationName(savedOrgName) and
QCoreApplication::setApplicationName(savedAppName) before clearing QSettings so
the global QCoreApplication singleton state is returned to its prior values and
other tests are not contaminated.
---
Outside diff comments:
In `@src/mainwindow_test.cpp`:
- Around line 83-149: The tests leak heap-allocated QKeyEvent objects because
they call window->keyPressEvent(new QKeyEvent(...)) without deleting them;
replace each heap allocation in tests (all calls to window->keyPressEvent(...)
in MainWindowTest: e.g., tests SetTransformState..., KeyXTogglesTransformSpace,
KeyDeleteWithEmptySelection, KeyFFrameSelectionEmptyDoesNotCrash,
CycleAllStatesViaKeyboard, UnmappedKeyDoesNotCrash, etc.) with a stack-allocated
QKeyEvent local (QKeyEvent ev(...)) and pass its address
(window->keyPressEvent(&ev)), since keyPressEvent is synchronous and the stack
event will be valid for the call.
---
Nitpick comments:
In `@src/mainwindow_test.cpp`:
- Around line 299-307: The test is asserting transform space instead of the
transform state propagation; change the final assertion to verify the transform
state was updated (and/or the handler ran) by replacing the EXPECT_EQ checking
TransformOperator::getTransformSpace() with an assertion that
TransformOperator::getSingleton()->getTransformState() ==
TransformOperator::TS_SCALE (or assert the window UI action state like
actionScale_Object->isChecked() if not already), ensuring setTransformState(),
onTransformStateChange() and actionScale_Object reflect the TS_SCALE state.
In `@src/TransformOperator_test.cpp`:
- Around line 132-139: The test name claims gizmo visibility changes but only
asserts transform state and tracking; either rename TEST_F
TransformOperatorTests,TransformStateChangeWithNoSelectionHidesAllGizmos to a
name that matches the assertions (e.g.
TransformStateChangeWithoutSelectionUpdatesStateAndTracking) or extend the test
after calling op->onTransformStateChange(TransformOperator::TS_TRANSLATE) to
assert gizmo visibility via the appropriate gizmo members/methods on op (add
EXPECT_* checks for the translate/rotate/scale gizmo visibility APIs you have in
the class) so the name accurately reflects the assertions.
🪄 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: 1ee03f41-4be6-4c05-b1f8-441903f611dd
📒 Files selected for processing (3)
src/MeshImporterExporter_test.cppsrc/TransformOperator_test.cppsrc/mainwindow_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mainwindow_test.cpp (1)
15-19: Consider documenting the#define private publictechnique.This macro-based access pattern is a common testing workaround but is technically undefined behavior per the C++ standard. It works reliably with most compilers but may cause issues with certain optimizations or future compiler versions.
A brief comment explaining the rationale would help maintainers:
// NOTE: Temporarily redefine access specifiers to test private/protected members. // This is a pragmatic workaround for coverage; prefer public test APIs where feasible. `#define` private public `#define` protected public🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow_test.cpp` around lines 15 - 19, Add a brief comment above the macro redefinitions explaining that the code temporarily uses "#define private public" and "#define protected public" to access non-public members of MainWindow for testing, note that this is a pragmatic but technically undefined-behavior workaround and recommend preferring public test APIs or friend-test helpers where feasible; keep the existing undef lines after including "mainwindow.h" to limit scope.
🤖 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/mainwindow_test.cpp`:
- Around line 15-19: Add a brief comment above the macro redefinitions
explaining that the code temporarily uses "#define private public" and "#define
protected public" to access non-public members of MainWindow for testing, note
that this is a pragmatic but technically undefined-behavior workaround and
recommend preferring public test APIs or friend-test helpers where feasible;
keep the existing undef lines after including "mainwindow.h" to limit scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c39f8f04-4173-4c5d-a5d4-0036f6bf3fbd
📒 Files selected for processing (2)
src/TransformOperator_test.cppsrc/mainwindow_test.cpp
✅ Files skipped from review due to trivial changes (1)
- src/TransformOperator_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mainwindow_test.cpp (1)
90-108: Consider parameterizing repetitive key-shortcut tests.
Q/W/E/Rkey-path tests repeat the same setup/execution pattern. A parameterized test would reduce duplication and simplify future shortcut changes.Also applies to: 151-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow_test.cpp` around lines 90 - 108, The four repetitive tests in MainWindowTest (SetTransformStateSelect/Translate/Rotate/Scale) all call window->keyPressEvent(&event) with only the key differing; refactor them into a single parameterized test by converting MainWindowTest to use ::testing::TestWithParam<int> (or add a new param test fixture), move the common setup into the fixture, replace the four TEST_F cases with one TEST_P that builds QKeyEvent from GetParam() and calls window->keyPressEvent(&event) with EXPECT_NO_THROW, and add an INSTANTIATE_TEST_SUITE_P that supplies the Qt::Key values (Qt::Key_Q, Qt::Key_W, Qt::Key_E, Qt::Key_R) so future key changes require editing only the instantiation list.
🤖 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 203-206: The test currently asserts exact English labels on UI
actions (EXPECT_EQ(actions.first()->text(), "(No Recent Files)") and
EXPECT_EQ(actions.last()->text(), "Clear Recent Files")), which is
locale-sensitive; change these to locale-independent checks: remove the
exact-text EXPECT_EQs and instead assert structural/behavioral properties such
as actions.first()->isEnabled() (already present), the action count, the
QAction::objectName() or QAction::data() contains a stable identifier for the
"no recent" and "clear recent" actions, or that actions.last()->isEnabled() is
false/true as appropriate; update the assertions around actions.first() and
actions.last() to use objectName()/data() or role checks instead of literal text
comparisons so the test no longer depends on translations.
---
Nitpick comments:
In `@src/mainwindow_test.cpp`:
- Around line 90-108: The four repetitive tests in MainWindowTest
(SetTransformStateSelect/Translate/Rotate/Scale) all call
window->keyPressEvent(&event) with only the key differing; refactor them into a
single parameterized test by converting MainWindowTest to use
::testing::TestWithParam<int> (or add a new param test fixture), move the common
setup into the fixture, replace the four TEST_F cases with one TEST_P that
builds QKeyEvent from GetParam() and calls window->keyPressEvent(&event) with
EXPECT_NO_THROW, and add an INSTANTIATE_TEST_SUITE_P that supplies the Qt::Key
values (Qt::Key_Q, Qt::Key_W, Qt::Key_E, Qt::Key_R) so future key changes
require editing only the instantiation list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+203
to
+206
| EXPECT_EQ(actions.first()->text(), "(No Recent Files)"); | ||
| EXPECT_FALSE(actions.first()->isEnabled()); | ||
| EXPECT_EQ(actions.last()->text(), "Clear Recent Files"); | ||
| } |
There was a problem hiding this comment.
Avoid locale-sensitive assertions on visible UI text in tests.
Asserting exact English labels (e.g., "(No Recent Files)", "Clear Recent Files") can fail when translations are active. Prefer structural/behavioral assertions (enabled state, action role/data, or stable identifiers).
Suggested test-hardening diff
- EXPECT_EQ(actions.first()->text(), "(No Recent Files)");
EXPECT_FALSE(actions.first()->isEnabled());
- EXPECT_EQ(actions.last()->text(), "Clear Recent Files");
+ EXPECT_TRUE(actions.last()->isEnabled());
...
- ASSERT_EQ(clearAction->text(), "Clear Recent Files");Also applies to: 235-236, 240-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow_test.cpp` around lines 203 - 206, The test currently asserts
exact English labels on UI actions (EXPECT_EQ(actions.first()->text(), "(No
Recent Files)") and EXPECT_EQ(actions.last()->text(), "Clear Recent Files")),
which is locale-sensitive; change these to locale-independent checks: remove the
exact-text EXPECT_EQs and instead assert structural/behavioral properties such
as actions.first()->isEnabled() (already present), the action count, the
QAction::objectName() or QAction::data() contains a stable identifier for the
"no recent" and "clear recent" actions, or that actions.last()->isEnabled() is
false/true as appropriate; update the assertions around actions.first() and
actions.last() to use objectName()/data() or role checks instead of literal text
comparisons so the test no longer depends on translations.
|
This was referenced Mar 27, 2026
This was referenced Apr 6, 2026
3 tasks
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
1 participant
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.
Summary
Validation
Notes
Summary by CodeRabbit