Improve test coverage for 2.16.0 UX redesign code - #211
Merged
Conversation
New test files: - TransformCommands_test.cpp (20 tests): translate/rotate/scale/delete command undo/redo, destroyed node safety, multi-node operations - MaterialPresetLibrary_test.cpp (8 tests): singleton, preset names, apply preset with/without selection, signal emission - BatchExporter_test.cpp (10 tests): setters, empty execute, signals Expanded test files: - UndoManager_test.cpp (+7): stack(), 15-command undo, redo cleared by new push, text signals, empty stack safety, kill/recreate - ScaleGizmo_test.cpp (+7): fading, default colors, unnamed gizmo, highlight null, multiple scale, custom colours, visibility toggle - SceneTreeModel_test.cpp (+10): availableMaterials, invalid index safety for select/set/get material, roles, rebuild with entity - TransformOperator_test.cpp (+4): TS_SCALE with entity, remove clears undo, scale signal, sequential operations - ThemeManager_test.cpp (+8): additional color validity, qmlInstance, kill/recreate, alpha, multiple refresh, theme name - SpaceCamera_test.cpp (+6): frameSelection empty, speed/control key, key F, shift press/release, multi-frame movement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds extensive GoogleTest suites across nine test files, exercising construction, lifecycle, signals, input handling, scene/model interactions, transform commands, undo/redo semantics, and UI/theme accessors; tests include conditional skips for missing rendering resources and ensure Qt event processing in teardown. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b38deb88f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+835
to
+839
| // Perform a translate to push to undo stack | ||
| instance->setSelectedPosition(Ogre::Vector3(10, 20, 30)); | ||
|
|
||
| // Remove selected should work without crash | ||
| instance->removeSelected(); |
There was a problem hiding this comment.
setSelectedPosition() does not record a QUndoCommand; it computes a delta and calls translateSelected() directly. That means this test reaches removeSelected() with an empty UndoManager, so it would still pass if the UndoManager::clear() call were removed from TransformOperator::removeSelected(). As written, the new coverage does not actually exercise the stale-undo case the test name claims to protect.
Useful? React with 👍 / 👎.
Comment on lines
+793
to
+797
| // Hold W and A | ||
| QKeyEvent pressW(QEvent::KeyPress, Qt::Key_W, Qt::NoModifier); | ||
| QKeyEvent pressA(QEvent::KeyPress, Qt::Key_A, Qt::NoModifier); | ||
| spaceCamera.keyPressEvent(&pressW); | ||
| spaceCamera.keyPressEvent(&pressA); |
There was a problem hiding this comment.
MockSpaceCamera is built with the protected default constructor, which never calls setKeyMapping(), and the real SpaceCamera::setKeyMapping() no longer binds WASD anyway. Pressing W and A here therefore leaves the motion vectors at zero, so frameStarted() just returns true without touching the held-key movement branch. Any regression in per-frame keyboard camera motion would still pass this test.
Useful? React with 👍 / 👎.
Comment on lines
+345
to
+349
| // Undo should restore visibility | ||
| cmd->undo(); | ||
| // The node was visible=true originally | ||
| // After undo, wasVisible=true so node should be visible again | ||
|
|
There was a problem hiding this comment.
This test never verifies the node becomes visible again after cmd->undo(). In its current form it passes even if DeleteCommand::undo() stops restoring hidden nodes, so the commit reports new delete/undo coverage while leaving that regression completely unguarded.
Useful? React with 👍 / 👎.
![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 (12)
src/BatchExporter_test.cpp (2)
38-41: Consider usingQTemporaryDirfor cross-platform test paths.The hardcoded
/tmp/paths work since these tests only exercise setters with empty file lists, but usingQTemporaryDirwould make the tests more portable and theQTemporaryDirinclude is already present.Suggested improvement using QTemporaryDir
TEST_F(BatchExporterTests, SetOutputDirectory) { + QTemporaryDir tempDir; + ASSERT_TRUE(tempDir.isValid()); BatchExporter exporter; - EXPECT_NO_THROW(exporter.setOutputDirectory("/tmp/batch_export")); + EXPECT_NO_THROW(exporter.setOutputDirectory(tempDir.path())); } TEST_F(BatchExporterTests, ExecuteWithEmptyFileList) { + QTemporaryDir tempDir; + ASSERT_TRUE(tempDir.isValid()); BatchExporter exporter; exporter.setInputFiles(QStringList()); exporter.setOutputFormat("gltf2"); - exporter.setOutputDirectory("/tmp/batch_export_test"); + exporter.setOutputDirectory(tempDir.path()); QSignalSpy finishedSpy(&exporter, &BatchExporter::finished);As per coding guidelines: "All code must compile and run on Windows, Linux (Ubuntu), and macOS."
Also applies to: 43-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BatchExporter_test.cpp` around lines 38 - 41, Replace hardcoded "/tmp/..." test paths with a QTemporaryDir to ensure cross-platform portability: in the TEST_F named BatchExporterTests (tests SetOutputDirectory and the following cases in the same file) create a QTemporaryDir instance, verify it is valid, and pass temporaryDir.path() (or tempDir.path().toStdString() if needed) into BatchExporter::setOutputDirectory instead of the literal "/tmp/..." string; reuse this pattern for the other tests covering lines 43-57 and remove the hardcoded path. Ensure the QTemporaryDir variable outlives the call to setOutputDirectory for the duration of the test.
6-6: Remove unused include.
QTemporaryDiris included but never used in this file.Suggested fix
-#include <QTemporaryDir>Alternatively, see the next comment for using
QTemporaryDirto replace hardcoded/tmp/paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BatchExporter_test.cpp` at line 6, Remove the unused include by deleting the `#include` <QTemporaryDir> directive in BatchExporter_test.cpp (the unused include line is currently present as "#include <QTemporaryDir>"); if you prefer to keep temporary-dir usage instead, replace hardcoded "/tmp/" usages in the tests with a QTemporaryDir instance and use its path() where needed (ensure teardown/auto-removal remains correct).src/commands/TransformCommands_test.cpp (1)
315-427: Consider adding a destroyed-node safety test forDeleteCommand.Unlike
TranslateCommand,RotateCommand, andScaleCommand, theDeleteCommand::undo()implementation (per context snippet atsrc/commands/TransformCommands.cpp:118-131) does not useisNodeValid()to check pointer validity before dereferencing. If a node is externally destroyed,undo()would cause a use-after-free.The other command tests include
WithDestroyedNodetests, butDeleteCommandis missing this coverage. Consider adding a test to document this behavior or filing an issue to add the validity check toDeleteCommand::undo().Example test to expose the missing safety check
TEST_F(TransformCommandsTests, DeleteCommand_WithDestroyedNode) { Manager* mgr = Manager::getSingleton(); Ogre::SceneNode* node = mgr->addSceneNode("DelCmdDestroyed"); ASSERT_NE(node, nullptr); QList<Ogre::SceneNode*> nodes = {node}; auto* cmd = new DeleteCommand(nodes); cmd->redo(); // First redo (no-op) // Destroy the node externally mgr->destroySceneNode("DelCmdDestroyed"); // WARNING: This will crash with current implementation // because DeleteCommand::undo() doesn't use isNodeValid() // EXPECT_NO_THROW(cmd->undo()); delete cmd; }Would you like me to open an issue to track adding
isNodeValid()checks toDeleteCommand::undo()andDeleteCommand::redo()for consistency with other transform commands?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/TransformCommands_test.cpp` around lines 315 - 427, The DeleteCommand tests and implementation lack safety checks for externally destroyed nodes: update DeleteCommand::undo() (and optionally DeleteCommand::redo()) to guard dereferences with the existing isNodeValid(node) check used by TranslateCommand/RotateCommand/ScaleCommand, and add a unit test DeleteCommand_WithDestroyedNode (similar to other *WithDestroyedNode tests) that calls mgr->destroySceneNode(...) after the first redo and then asserts that cmd->undo() does not crash (EXPECT_NO_THROW) to document and verify the behavior; locate the logic in the DeleteCommand class in TransformCommands.cpp and mirror the validity checks and test pattern used by the other transform command tests.src/SpaceCamera_test.cpp (1)
777-784: Add at least one behavioral assertion for Shift handling.This currently checks only “no throw”. If possible, assert an observable post-condition (e.g.,
frameStartedstill returns true after Shift press/release) so the test validates behavior, not just crash-safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpaceCamera_test.cpp` around lines 777 - 784, The test only checks for no exceptions; add a behavioral assertion that Shift key handling preserves camera state: after creating MockSpaceCamera, call spaceCamera.keyPressEvent(&pressShift) and assert an observable post-condition such as EXPECT_TRUE(spaceCamera.frameStarted()) (or the appropriate state/query method on MockSpaceCamera) and then after keyReleaseEvent(&releaseShift) assert the same or expected change (e.g., EXPECT_TRUE(spaceCamera.frameStarted()) or EXPECT_FALSE(...) depending on intended behavior). Update TEST(SpaceCamera, KeyPressShift) to include these assertions referencing MockSpaceCamera, keyPressEvent, keyReleaseEvent, and frameStarted.src/ScaleGizmo_test.cpp (4)
161-165:HighlightNullReturnsZerois redundant with existing null-highlight coverage.This behavior is already asserted in
HighlightAxis; consider merging to keep the suite lean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScaleGizmo_test.cpp` around lines 161 - 165, The test HighlightNullReturnsZero is redundant with the existing null-case coverage in the HighlightAxis test; remove HighlightNullReturnsZero (TEST_F ScaleGizmoTests, HighlightNullReturnsZero) and instead ensure the null-pointer assertion (calling mScaleGizmo->highlightAxis(nullptr) expecting Ogre::Vector3::ZERO and !isHighlighted()) is present within the existing HighlightAxis test or merged into it so the behavior remains covered without a duplicate test.
192-201:VisibilityToggleoverlapsSetVisibleand only checks X-axis.Either consolidate with
SetVisibleor extend this one to assert Y/Z too; otherwise it adds maintenance cost with limited extra signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScaleGizmo_test.cpp` around lines 192 - 201, The VisibilityToggle test duplicates SetVisible and only asserts the X axis; either remove/merge this test into the existing SetVisible test or extend it to assert all axes' visibility. Update TEST_F(ScaleGizmoTests, VisibilityToggle) to call mScaleGizmo->setVisible(true/false/true) and replace EXPECT_TRUE/EXPECT_FALSE(mScaleGizmo->getXAxis().isVisible()) with equivalent assertions for getYAxis().isVisible() and getZAxis().isVisible(), or delete this test and ensure SetVisible covers X/Y/Z visibility toggles.
203-220: Sequential highlight test mostly repeatsHighlightAxis; parameterization would simplify this.A table-driven test can cover axis sequence + clear-state transitions with less duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScaleGizmo_test.cpp` around lines 203 - 220, The test HighlightEachAxisSequentially duplicates calls to highlightAxis/getXAxis/getYAxis/getZAxis and assertions; refactor into a parameterized/table-driven test using GoogleTest's TEST_P or a loop over a vector of pairs (axis accessor pointer-to-member or lambdas and expected vectors) to iterate X, Y, Z and finally the nullptr clear case, asserting returned vector equals expected and isHighlighted() matches true for axes and false for nullptr; update ScaleGizmoTests to provide the parameter set and replace the existing TEST_F with the new parameterized test using the same symbols: highlightAxis, getXAxis/getYAxis/getZAxis, and isHighlighted.
154-159: Prefer isolated scene nodes for additional gizmo instances in fixture tests.Both tests create extra
ScaleGizmoobjects on the samemLinkNodeused by the fixture object. Using a dedicated node per extra gizmo would reduce coupling and future flakiness.Also applies to: 222-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScaleGizmo_test.cpp` around lines 154 - 159, The extra ScaleGizmo instances in the UnnamedGizmoCreation test (and the other test mentioned) are created on the shared fixture node mLinkNode; instead, construct a dedicated scene/link node for each extra gizmo by calling the same constructor/factory used to initialize mLinkNode in the test fixture, then pass that new node into ScaleGizmo (e.g., create a fresh node instance and use it instead of mLinkNode when constructing the extra ScaleGizmo), ensuring each test-owned gizmo uses its own node to avoid coupling and flakiness.src/MaterialPresetLibrary_test.cpp (1)
31-42: Redundant assertion afterASSERT_NE.Line 41
EXPECT_NE(inst2, nullptr)is redundant because line 38 already usesASSERT_NE(inst2, nullptr)which would abort the test if null.Suggested fix
auto* inst2 = MaterialPresetLibrary::instance(); ASSERT_NE(inst2, nullptr); - // After kill+recreate, it should be a new instance - // (pointer may or may not differ due to memory reuse, but it should work) - EXPECT_NE(inst2, nullptr); + // After kill+recreate, it should be a fresh working instance + // (pointer address may coincide due to memory reuse, so we just verify functionality) + EXPECT_FALSE(inst2->presetNames().isEmpty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialPresetLibrary_test.cpp` around lines 31 - 42, In the TEST_F(MaterialPresetLibraryTests, KillAndRecreate) test, remove the redundant EXPECT_NE(inst2, nullptr) that duplicates the earlier ASSERT_NE(inst2, nullptr); keep the ASSERT_NE(inst2, nullptr) which already aborts on failure and verifies MaterialPresetLibrary::instance() returned a non-null pointer for inst2 after MaterialPresetLibrary::kill(), and ensure no additional null-check assertion for inst2 remains.src/SceneTreeModel_test.cpp (2)
163-192: Test may silently pass without exercising assertions.When
rows == 0, all assertions inside the conditional are skipped and the test passes silently. Consider either addingGTEST_SKIP()when no rows exist or usingASSERT_GT(rows, 0)to explicitly fail/skip when the precondition isn't met.Suggested fix
model->rebuild(); QModelIndex root = model->rootIndex(); int rows = model->rowCount(root); - if (rows > 0) { + if (rows == 0) { + GTEST_SKIP() << "No rows available to test data roles"; + } + { QModelIndex idx = model->index(0, 0, root); - if (idx.isValid()) { + ASSERT_TRUE(idx.isValid()); + { // Test all roles🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneTreeModel_test.cpp` around lines 163 - 192, The test currently skips all assertions when rows == 0, allowing a silent pass; update TEST_F(SceneTreeModelTests, DataWithVariousRoles) to enforce the precondition by checking the row count immediately after model->rebuild(): call ASSERT_GT(rows, 0) (or GTEST_SKIP() with an explanatory message) right after computing int rows = model->rowCount(root) so the test fails or is explicitly skipped when there are no rows and the subsequent role checks (model->data(..., SceneTreeModel::NameRole), SceneTreeModel::TypeRole, SceneTreeModel::TypeLabelRole, SceneTreeModel::SelectedRole, and the unknown role check) are guaranteed to run.
65-71: No-op assertion:EXPECT_GE(mats.size(), 0)always passes.
QStringList::size()returns a non-negativeint, so this assertion can never fail. Consider replacing it with a more meaningful check or removing it since the comment already acknowledges the test just verifies no crash.Suggested fix
TEST_F(SceneTreeModelTests, AvailableMaterials) { // availableMaterials() should return a list (possibly empty in test env) QStringList mats = model->availableMaterials(); - // Just verify it returns without crash; the list may be empty - // if no materials besides internal ones are loaded - EXPECT_GE(mats.size(), 0); + // Verify it returns without crash (list may be empty in test env) + SUCCEED(); // Explicit no-op assertion for crash-safety test }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneTreeModel_test.cpp` around lines 65 - 71, The assertion EXPECT_GE(mats.size(), 0) is a no-op because QStringList::size() is always non-negative; replace it with a meaningful check that verifies the call does not throw/crash: remove the mats size assertion and instead use EXPECT_NO_THROW(model->availableMaterials()) (or ASSERT_NO_THROW if you prefer to abort on failure) in the SceneTreeModelTests::AvailableMaterials test to ensure availableMaterials() can be invoked safely on the model instance.src/ThemeManager_test.cpp (1)
131-136: Duplicate test:ThemeNameIsLightOrDarkduplicatesThemeNameNotEmpty.This test's assertion (
name == "light" || name == "dark") is identical to line 42 inThemeNameNotEmpty. Consider removing this test or consolidating the two.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ThemeManager_test.cpp` around lines 131 - 136, The test TEST_F(ThemeManagerTests, ThemeNameIsLightOrDark) duplicates the assertion in ThemeNameNotEmpty; remove or consolidate it by deleting this TEST_F block or merging its check into ThemeNameNotEmpty: locate the ThemeNameIsLightOrDark test (uses ThemeManager::instance() and themeName()) and either remove the entire test or move its EXPECT_TRUE(name == "light" || name == "dark") assertion into the existing ThemeNameNotEmpty test so there’s a single test validating themeName() against "light" or "dark".
🤖 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/SpaceCamera_test.cpp`:
- Around line 728-735: The test uses the global SelectionSet singleton via
SelectionSet::getSingleton() but never destroys it, risking leaked state across
tests; update TEST(SpaceCamera, FrameSelectionWithEmptySelection) (and the other
test around lines 763-771) to perform proper setup/teardown around SelectionSet
by ensuring SelectionSet::kill() is called after the test (or in a test fixture
TearDown) so the singleton is cleared between tests; locate usages of
SelectionSet::getSingleton() in these tests and add corresponding cleanup (or
convert the tests to use a fixture that calls SelectionSet::kill() in TearDown)
to guarantee deterministic singleton lifecycle.
In `@src/TransformOperator_test.cpp`:
- Around line 827-841: The test RemoveSelectedClearsUndoStack currently only
checks that SelectionSet is empty but should also verify the undo stack was
cleared; update TEST_F(TransformOperatorTestFixture,
RemoveSelectedClearsUndoStack) to `#include` "UndoManager.h" and add an assertion
after instance->removeSelected() that checks
UndoManager::getSingleton()->isEmpty() (or appropriate method on UndoManager
confirming the stack was cleared) to directly verify that
TransformOperator::removeSelected() invoked
UndoManager::getSingleton()->clear().
---
Nitpick comments:
In `@src/BatchExporter_test.cpp`:
- Around line 38-41: Replace hardcoded "/tmp/..." test paths with a
QTemporaryDir to ensure cross-platform portability: in the TEST_F named
BatchExporterTests (tests SetOutputDirectory and the following cases in the same
file) create a QTemporaryDir instance, verify it is valid, and pass
temporaryDir.path() (or tempDir.path().toStdString() if needed) into
BatchExporter::setOutputDirectory instead of the literal "/tmp/..." string;
reuse this pattern for the other tests covering lines 43-57 and remove the
hardcoded path. Ensure the QTemporaryDir variable outlives the call to
setOutputDirectory for the duration of the test.
- Line 6: Remove the unused include by deleting the `#include` <QTemporaryDir>
directive in BatchExporter_test.cpp (the unused include line is currently
present as "#include <QTemporaryDir>"); if you prefer to keep temporary-dir
usage instead, replace hardcoded "/tmp/" usages in the tests with a
QTemporaryDir instance and use its path() where needed (ensure
teardown/auto-removal remains correct).
In `@src/commands/TransformCommands_test.cpp`:
- Around line 315-427: The DeleteCommand tests and implementation lack safety
checks for externally destroyed nodes: update DeleteCommand::undo() (and
optionally DeleteCommand::redo()) to guard dereferences with the existing
isNodeValid(node) check used by TranslateCommand/RotateCommand/ScaleCommand, and
add a unit test DeleteCommand_WithDestroyedNode (similar to other
*WithDestroyedNode tests) that calls mgr->destroySceneNode(...) after the first
redo and then asserts that cmd->undo() does not crash (EXPECT_NO_THROW) to
document and verify the behavior; locate the logic in the DeleteCommand class in
TransformCommands.cpp and mirror the validity checks and test pattern used by
the other transform command tests.
In `@src/MaterialPresetLibrary_test.cpp`:
- Around line 31-42: In the TEST_F(MaterialPresetLibraryTests, KillAndRecreate)
test, remove the redundant EXPECT_NE(inst2, nullptr) that duplicates the earlier
ASSERT_NE(inst2, nullptr); keep the ASSERT_NE(inst2, nullptr) which already
aborts on failure and verifies MaterialPresetLibrary::instance() returned a
non-null pointer for inst2 after MaterialPresetLibrary::kill(), and ensure no
additional null-check assertion for inst2 remains.
In `@src/ScaleGizmo_test.cpp`:
- Around line 161-165: The test HighlightNullReturnsZero is redundant with the
existing null-case coverage in the HighlightAxis test; remove
HighlightNullReturnsZero (TEST_F ScaleGizmoTests, HighlightNullReturnsZero) and
instead ensure the null-pointer assertion (calling
mScaleGizmo->highlightAxis(nullptr) expecting Ogre::Vector3::ZERO and
!isHighlighted()) is present within the existing HighlightAxis test or merged
into it so the behavior remains covered without a duplicate test.
- Around line 192-201: The VisibilityToggle test duplicates SetVisible and only
asserts the X axis; either remove/merge this test into the existing SetVisible
test or extend it to assert all axes' visibility. Update TEST_F(ScaleGizmoTests,
VisibilityToggle) to call mScaleGizmo->setVisible(true/false/true) and replace
EXPECT_TRUE/EXPECT_FALSE(mScaleGizmo->getXAxis().isVisible()) with equivalent
assertions for getYAxis().isVisible() and getZAxis().isVisible(), or delete this
test and ensure SetVisible covers X/Y/Z visibility toggles.
- Around line 203-220: The test HighlightEachAxisSequentially duplicates calls
to highlightAxis/getXAxis/getYAxis/getZAxis and assertions; refactor into a
parameterized/table-driven test using GoogleTest's TEST_P or a loop over a
vector of pairs (axis accessor pointer-to-member or lambdas and expected
vectors) to iterate X, Y, Z and finally the nullptr clear case, asserting
returned vector equals expected and isHighlighted() matches true for axes and
false for nullptr; update ScaleGizmoTests to provide the parameter set and
replace the existing TEST_F with the new parameterized test using the same
symbols: highlightAxis, getXAxis/getYAxis/getZAxis, and isHighlighted.
- Around line 154-159: The extra ScaleGizmo instances in the
UnnamedGizmoCreation test (and the other test mentioned) are created on the
shared fixture node mLinkNode; instead, construct a dedicated scene/link node
for each extra gizmo by calling the same constructor/factory used to initialize
mLinkNode in the test fixture, then pass that new node into ScaleGizmo (e.g.,
create a fresh node instance and use it instead of mLinkNode when constructing
the extra ScaleGizmo), ensuring each test-owned gizmo uses its own node to avoid
coupling and flakiness.
In `@src/SceneTreeModel_test.cpp`:
- Around line 163-192: The test currently skips all assertions when rows == 0,
allowing a silent pass; update TEST_F(SceneTreeModelTests, DataWithVariousRoles)
to enforce the precondition by checking the row count immediately after
model->rebuild(): call ASSERT_GT(rows, 0) (or GTEST_SKIP() with an explanatory
message) right after computing int rows = model->rowCount(root) so the test
fails or is explicitly skipped when there are no rows and the subsequent role
checks (model->data(..., SceneTreeModel::NameRole), SceneTreeModel::TypeRole,
SceneTreeModel::TypeLabelRole, SceneTreeModel::SelectedRole, and the unknown
role check) are guaranteed to run.
- Around line 65-71: The assertion EXPECT_GE(mats.size(), 0) is a no-op because
QStringList::size() is always non-negative; replace it with a meaningful check
that verifies the call does not throw/crash: remove the mats size assertion and
instead use EXPECT_NO_THROW(model->availableMaterials()) (or ASSERT_NO_THROW if
you prefer to abort on failure) in the SceneTreeModelTests::AvailableMaterials
test to ensure availableMaterials() can be invoked safely on the model instance.
In `@src/SpaceCamera_test.cpp`:
- Around line 777-784: The test only checks for no exceptions; add a behavioral
assertion that Shift key handling preserves camera state: after creating
MockSpaceCamera, call spaceCamera.keyPressEvent(&pressShift) and assert an
observable post-condition such as EXPECT_TRUE(spaceCamera.frameStarted()) (or
the appropriate state/query method on MockSpaceCamera) and then after
keyReleaseEvent(&releaseShift) assert the same or expected change (e.g.,
EXPECT_TRUE(spaceCamera.frameStarted()) or EXPECT_FALSE(...) depending on
intended behavior). Update TEST(SpaceCamera, KeyPressShift) to include these
assertions referencing MockSpaceCamera, keyPressEvent, keyReleaseEvent, and
frameStarted.
In `@src/ThemeManager_test.cpp`:
- Around line 131-136: The test TEST_F(ThemeManagerTests,
ThemeNameIsLightOrDark) duplicates the assertion in ThemeNameNotEmpty; remove or
consolidate it by deleting this TEST_F block or merging its check into
ThemeNameNotEmpty: locate the ThemeNameIsLightOrDark test (uses
ThemeManager::instance() and themeName()) and either remove the entire test or
move its EXPECT_TRUE(name == "light" || name == "dark") assertion into the
existing ThemeNameNotEmpty test so there’s a single test validating themeName()
against "light" or "dark".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60fb72e5-6508-4345-b2b9-08e17cf9cb38
📒 Files selected for processing (9)
src/BatchExporter_test.cppsrc/MaterialPresetLibrary_test.cppsrc/ScaleGizmo_test.cppsrc/SceneTreeModel_test.cppsrc/SpaceCamera_test.cppsrc/ThemeManager_test.cppsrc/TransformOperator_test.cppsrc/UndoManager_test.cppsrc/commands/TransformCommands_test.cpp
Comment on lines
+728
to
+735
| TEST(SpaceCamera, FrameSelectionWithEmptySelection) | ||
| { | ||
| MockSpaceCamera spaceCamera; | ||
| // frameSelection() should return early when selection is empty | ||
| // (it checks sel->isEmpty() and returns before dereferencing mTarget) | ||
| SelectionSet::getSingleton()->clear(); | ||
| EXPECT_NO_THROW(spaceCamera.frameSelection()); | ||
| } |
There was a problem hiding this comment.
Clean up SelectionSet singleton lifecycle in tests.
These tests create/use the global SelectionSet singleton but never destroy it. That can leak state across test order/files and make behavior brittle. Prefer fixture-based setup/teardown with SelectionSet::kill().
Suggested refactor
+class SpaceCameraSelectionTest : public ::testing::Test
+{
+protected:
+ void SetUp() override
+ {
+ SelectionSet::getSingleton()->clear();
+ }
+
+ void TearDown() override
+ {
+ SelectionSet::kill();
+ }
+};
-
-TEST(SpaceCamera, FrameSelectionWithEmptySelection)
+TEST_F(SpaceCameraSelectionTest, FrameSelectionWithEmptySelection)
{
MockSpaceCamera spaceCamera;
- SelectionSet::getSingleton()->clear();
EXPECT_NO_THROW(spaceCamera.frameSelection());
}
-
-TEST(SpaceCamera, KeyPressF)
+TEST_F(SpaceCameraSelectionTest, KeyPressF)
{
MockSpaceCamera spaceCamera;
- SelectionSet::getSingleton()->clear();
QKeyEvent pressF(QEvent::KeyPress, Qt::Key_F, Qt::NoModifier);
EXPECT_NO_THROW(spaceCamera.keyPressEvent(&pressF));
}As per coding guidelines: "Use singleton pattern for core state via ClassName::getSingleton() or ClassName::getSingletonPtr(), and destroy with ClassName::kill()."
Also applies to: 763-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SpaceCamera_test.cpp` around lines 728 - 735, The test uses the global
SelectionSet singleton via SelectionSet::getSingleton() but never destroys it,
risking leaked state across tests; update TEST(SpaceCamera,
FrameSelectionWithEmptySelection) (and the other test around lines 763-771) to
perform proper setup/teardown around SelectionSet by ensuring
SelectionSet::kill() is called after the test (or in a test fixture TearDown) so
the singleton is cleared between tests; locate usages of
SelectionSet::getSingleton() in these tests and add corresponding cleanup (or
convert the tests to use a fixture that calls SelectionSet::kill() in TearDown)
to guarantee deterministic singleton lifecycle.
- RemoveSelectedClearsUndoStack: push actual TranslateCommand before removing, verify canUndo() becomes false (was testing with empty stack) - DeleteCommand_UndoRestoresVisibility: attach entity and assert getVisible() after undo (was missing assertion) - FrameStartedWithMovementKeys: use arrow keys instead of WASD (WASD was removed from SpaceCamera key mappings) - SpaceCamera key release: match arrow keys in release events Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/TransformOperator_test.cpp (1)
850-860: Minor inconsistency in signal assertion style.Line 859 uses
EXPECT_GE(spy.count(), 1)while other signal tests in this file (e.g., lines 307, 319, 330) useEXPECT_EQ(spy.count(), 1). Consider usingEXPECT_EQfor consistency, unless the implementation is expected to emit multiple times.Suggested change for consistency
- EXPECT_GE(spy.count(), 1); + EXPECT_EQ(spy.count(), 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator_test.cpp` around lines 850 - 860, In the test ScaleSelectedEmitsSignal, replace the loose assertion EXPECT_GE(spy.count(), 1) with the stricter EXPECT_EQ(spy.count(), 1) to match the style used by other tests (e.g., earlier signal tests) and ensure the TransformOperator::selectedScaleChanged signal from TransformOperator::getSingleton() is asserted to be emitted exactly once after calling setSelectedScale; update the assertion in TEST_F(TransformOperatorTestFixture, ScaleSelectedEmitsSignal) accordingly.
🤖 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 850-860: In the test ScaleSelectedEmitsSignal, replace the loose
assertion EXPECT_GE(spy.count(), 1) with the stricter EXPECT_EQ(spy.count(), 1)
to match the style used by other tests (e.g., earlier signal tests) and ensure
the TransformOperator::selectedScaleChanged signal from
TransformOperator::getSingleton() is asserted to be emitted exactly once after
calling setSelectedScale; update the assertion in
TEST_F(TransformOperatorTestFixture, ScaleSelectedEmitsSignal) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 817abd46-e175-4867-b249-f1d04ef7b5cf
📒 Files selected for processing (3)
src/SpaceCamera_test.cppsrc/TransformOperator_test.cppsrc/commands/TransformCommands_test.cpp
✅ Files skipped from review due to trivial changes (1)
- src/SpaceCamera_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/TransformCommands_test.cpp
|
This was referenced Mar 23, 2026
This was referenced Apr 8, 2026
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
Adds 80 new unit tests to improve coverage for the UX redesign code introduced in 2.16.0. Targets files that had 0% or low coverage.
Coverage targets
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit