Increase test coverage with 71 new tests and in-memory helpers#171
Increase test coverage with 71 new tests and in-memory helpers#171fernandotonon merged 3 commits intomasterfrom
Conversation
Add reusable in-memory mesh creation helpers (triangle, skeleton, animated entity) to TestHelpers.h and expand test coverage across 7 test files: - AnimationControlWidget: 23 new tests (was 3) for animation tree, keyframes, playback - MCPServer: 15 new tests for tool success paths with in-memory entities - MeshImporterExporter: 8 new tests for in-memory export (OBJ, STL, glTF, Ogre) - TransformOperator: 7 new tests for translate/rotate/scale with entities - PrimitivesWidget: 10 new tests verifying entity creation for all primitives - Manager: 6 new tests for in-memory entity lifecycle and scene hierarchy - Fix about_test/main_test creating local QApplication that destroyed the global one Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a broad set of headless/in-memory tests and helpers: in-memory mesh/skeleton/entity builders, extensive tests for MCPServer, Manager, mesh import/export, primitives, transforms, and animation widget flows; replaces local QApplication mocks with runtime QCoreApplication existence checks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
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: 65ed9641aa
ℹ️ 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".
src/animationcontrolwidget_test.cpp
Outdated
| EXPECT_EQ(entityItem->childCount(), 2); | ||
|
|
||
| // Switch to second animation | ||
| tree->setCurrentItem(entityItem->child(1)); |
There was a problem hiding this comment.
MultipleAnimationsInTree assumes entityItem->child(1) is the 0.5s animation, but AnimationControlWidget::updateAnimationTree() builds children by iterating getAnimationStates(), whose iteration order is not guaranteed to match creation order. In environments where the order differs (e.g., different STL/OGRE implementations), this test selects the wrong animation and fails despite correct behavior. Selecting the child by animation text (e.g., anim: Run) avoids this ordering-dependent failure.
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: 3
🧹 Nitpick comments (5)
src/main_test.cpp (2)
17-22: Remove unusedMockMainWindowclass.The
MockMainWindowclass is no longer used after removing theQApplicationAndMainWindowMocktest. Consider removing this dead code to keep the test file clean.🧹 Proposed removal
-// Mock class for MainWindow -class MockMainWindow : public MainWindow -{ -public: - MOCK_METHOD(void, show, ()); -}; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main_test.cpp` around lines 17 - 22, Remove the dead MockMainWindow class definition (class MockMainWindow : public MainWindow { ... }) from the test file: locate the MockMainWindow declaration and delete it, and then run/build tests to ensure there are no remaining references to MockMainWindow or relies-on symbols in other tests (if any, replace or update them accordingly).
50-50: Disabled test still creates a localQApplication.The
DISABLED_ImportMeshstest creates its ownQApplicationat line 50, which would cause the same destruction bug if this test is ever re-enabled. Consider refactoring this test to use the globalQApplicationfromtest_main.cppwhen fixing the underlying Ogre issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main_test.cpp` at line 50, The DISABLED_ImportMeshs test currently constructs a local QApplication (QApplication app(argc, mutable_argv)) which will reproduce the destruction bug if re-enabled; change the test to use the global QApplication provided by test_main.cpp instead of creating its own instance—e.g., obtain the existing application via QApplication::instance() or reference the global app object exported by test_main.cpp and remove the local QApplication construction in the DISABLED_ImportMeshs test so the test uses the shared application lifetime while you address the underlying Ogre destruction issue.src/animationcontrolwidget_test.cpp (1)
526-607: Consider extracting the multi-animation entity creation to a helper.This test duplicates significant mesh/skeleton creation code that's similar to
createAnimatedTestEntity()in TestHelpers.h. A parameterized helper or a variant that accepts multiple animations would reduce duplication and improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/animationcontrolwidget_test.cpp` around lines 526 - 607, The test duplicates mesh/skeleton/entity setup that matches createAnimatedTestEntity() in TestHelpers.h; extract the shared setup into a new helper (e.g., createAnimatedTestEntityWithAnimations or extend createAnimatedTestEntity) that accepts a list of animation descriptors (name and length/keyframe info), move skeleton/mesh/bone/vertex/index creation there, and return the created scene node/entity; then simplify AnimationControlWidgetTest::MultipleAnimationsInTree to call the new helper, attach/select the returned node, call widget.updateAnimationTree(), and keep the existing assertions (tree lookup, selecting child, checking horizontalSlider maximum). Ensure you reference and reuse createAnimatedTestEntity()/createAnimatedTestEntityWithAnimations and AnimationControlWidget::updateAnimationTree when updating the test.src/TestHelpers.h (1)
312-405: Consider adding error handling for entity creation failure.The function creates multiple Ogre resources (skeleton, animation, mesh) but if
sceneMgr->createEntity()fails, these resources are leaked. Since callers check fornullptrreturn, a failure path should clean up.💡 Proposed error handling
// Create entity via Manager auto* sceneMgr = Manager::getSingleton()->getSceneMgr(); auto* node = Manager::getSingleton()->addSceneNode(name.c_str()); - auto* entity = sceneMgr->createEntity(name, mesh); - node->attachObject(entity); + Ogre::Entity* entity = nullptr; + try { + entity = sceneMgr->createEntity(name, mesh); + node->attachObject(entity); + } catch (...) { + Manager::getSingleton()->destroySceneNode(node); + Ogre::MeshManager::getSingleton().remove(mesh); + Ogre::SkeletonManager::getSingleton().remove(skel); + return nullptr; + } return entity;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TestHelpers.h` around lines 312 - 405, If sceneMgr->createEntity(...) fails in createAnimatedTestEntity, clean up all created resources before returning nullptr: destroy/remove the Mesh (mesh), the Skeleton (skel), free any HardwareBuffers (vbuf, ibuf) if you allocated them, and remove the SceneNode (node) you created via Manager::getSingleton()->addSceneNode; then return nullptr. Implement a failure check immediately after calling sceneMgr->createEntity(name, mesh) that performs the above cleanup using the corresponding Ogre managers (e.g., Ogre::MeshManager::getSingleton().remove(mesh->getName()), Ogre::SkeletonManager::getSingleton().remove(skel->getName()), HardwareBufferManager cleanup, and Manager::getSingleton()->removeSceneNode or node destruction), ensuring no leaks when createEntity returns nullptr.src/MCPServer_test.cpp (1)
2060-2084: Custom-parameter tests don’t verify parameter effects yet.These tests currently pass if creation succeeds, even if
radius,height,segments, orsizeX/Y/Zare ignored. Consider asserting resulting geometry/state (e.g., bounds, vertex counts, or queried primitive properties) so coverage matches the test intent.Also applies to: 2170-2182
🤖 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/Manager_test.cpp`:
- Around line 582-597: The test unsafely assumes objects from
Manager::getEntities() are Ogre::Entity*; before casting/use, check each
object's getMovableType() equals "Entity" (or use dynamic_cast) to avoid
ManualObject crashes—e.g., after retrieving the last movable from
Manager::getEntities(), verify movable->getMovableType() == "Entity" and only
then static_cast/Ogre::Entity* entity = static_cast<Ogre::Entity*>(movable) and
call Manager::hasAnimationName(entity,...); alternatively update
Manager::getEntities() to filter by getMovableType() == "Entity" so callers
always receive only entities.
In `@src/MCPServer_test.cpp`:
- Around line 2323-2330: The test hardcodes "/tmp/…" which fails on Windows;
replace the hardcoded path usage around args and cleanup with a cross-platform
temp path (e.g., use QDir::tempPath() or better QTemporaryDir) when preparing
args["path"] passed to server->callTool("export_mesh", args) and when calling
QFile::remove to delete the exported files; construct the full file names using
the chosen temp dir (or use QTemporaryDir to auto-cleanup) so the test runs on
Windows/macOS/Linux without `#ifdef` guards.
In `@src/PrimitivesWidget_test.cpp`:
- Around line 282-292: The test dereferences mesh->sharedVertexData which is
null for procedurally generated meshes; update CreateCubeVerifyEntity (and the
other primitive tests: Sphere, Torus, Cone, Tube, Capsule, IcoSphere, RBox,
Spring) to first get auto* mesh = entity->getMesh() and check mesh is non-null,
then inspect per-submesh vertex data instead of sharedVertexData (e.g., if
mesh->getNumSubMeshes() > 0 iterate submesh = mesh->getSubMesh(i) and ensure
submesh->vertexData && submesh->vertexData->vertexCount > 0u or sum vertex
counts), mirroring the safe pattern used in MeshTransform_test.cpp; keep the
same setup/teardown (PrimitiveObject::createCube, Manager::getSingleton,
sceneMgr->getEntity, mgr->destroySceneNode) but replace the direct
sharedVertexData access with these null-checked per-submesh checks.
---
Nitpick comments:
In `@src/animationcontrolwidget_test.cpp`:
- Around line 526-607: The test duplicates mesh/skeleton/entity setup that
matches createAnimatedTestEntity() in TestHelpers.h; extract the shared setup
into a new helper (e.g., createAnimatedTestEntityWithAnimations or extend
createAnimatedTestEntity) that accepts a list of animation descriptors (name and
length/keyframe info), move skeleton/mesh/bone/vertex/index creation there, and
return the created scene node/entity; then simplify
AnimationControlWidgetTest::MultipleAnimationsInTree to call the new helper,
attach/select the returned node, call widget.updateAnimationTree(), and keep the
existing assertions (tree lookup, selecting child, checking horizontalSlider
maximum). Ensure you reference and reuse
createAnimatedTestEntity()/createAnimatedTestEntityWithAnimations and
AnimationControlWidget::updateAnimationTree when updating the test.
In `@src/main_test.cpp`:
- Around line 17-22: Remove the dead MockMainWindow class definition (class
MockMainWindow : public MainWindow { ... }) from the test file: locate the
MockMainWindow declaration and delete it, and then run/build tests to ensure
there are no remaining references to MockMainWindow or relies-on symbols in
other tests (if any, replace or update them accordingly).
- Line 50: The DISABLED_ImportMeshs test currently constructs a local
QApplication (QApplication app(argc, mutable_argv)) which will reproduce the
destruction bug if re-enabled; change the test to use the global QApplication
provided by test_main.cpp instead of creating its own instance—e.g., obtain the
existing application via QApplication::instance() or reference the global app
object exported by test_main.cpp and remove the local QApplication construction
in the DISABLED_ImportMeshs test so the test uses the shared application
lifetime while you address the underlying Ogre destruction issue.
In `@src/TestHelpers.h`:
- Around line 312-405: If sceneMgr->createEntity(...) fails in
createAnimatedTestEntity, clean up all created resources before returning
nullptr: destroy/remove the Mesh (mesh), the Skeleton (skel), free any
HardwareBuffers (vbuf, ibuf) if you allocated them, and remove the SceneNode
(node) you created via Manager::getSingleton()->addSceneNode; then return
nullptr. Implement a failure check immediately after calling
sceneMgr->createEntity(name, mesh) that performs the above cleanup using the
corresponding Ogre managers (e.g.,
Ogre::MeshManager::getSingleton().remove(mesh->getName()),
Ogre::SkeletonManager::getSingleton().remove(skel->getName()),
HardwareBufferManager cleanup, and Manager::getSingleton()->removeSceneNode or
node destruction), ensuring no leaks when createEntity returns nullptr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5c69ca2-2144-4c58-8df0-7bff460262a1
📒 Files selected for processing (9)
src/MCPServer_test.cppsrc/Manager_test.cppsrc/MeshImporterExporter_test.cppsrc/PrimitivesWidget_test.cppsrc/TestHelpers.hsrc/TransformOperator_test.cppsrc/about_test.cppsrc/animationcontrolwidget_test.cppsrc/main_test.cpp
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix null dereference on sharedVertexData in PrimitivesWidget tests: use per-submesh vertex data (procedural meshes don't use sharedVertexData) - Add movable type check in Manager_test before casting to Ogre::Entity* - Use QDir::tempPath() instead of hardcoded /tmp/ in MCPServer export test - Select animation by name instead of child index in MultipleAnimationsInTree (iteration order of animation states is not guaranteed) - Remove dead MockMainWindow class from main_test.cpp - Fix DISABLED_ImportMeshs to use global QApplication instead of creating local Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Manager_test.cpp (1)
592-596: Break after finding the first validOgre::Entity.Once an entity is found, continuing iteration can accidentally pick a different one and make assertions less deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Manager_test.cpp` around lines 592 - 596, In the loop over mgr->getEntities() stop iterating once you find the first matching Ogre::Entity: after detecting obj->getMovableType() == "Entity" and assigning entity = static_cast<Ogre::Entity*>(obj) add an early break so subsequent items cannot overwrite entity; update the loop that calls mgr->getEntities(), checks getMovableType(), and assigns to entity to include this break to make the test deterministic.src/PrimitivesWidget_test.cpp (1)
282-424: Consider extracting a helper for primitive entity verification.The 9
Create*VerifyEntitytests duplicate the same setup/verification/cleanup flow. A helper (or parameterized test) would reduce maintenance and keep assertions consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PrimitivesWidget_test.cpp` around lines 282 - 424, Multiple tests duplicate the same create/verify/destroy pattern; extract a reusable helper (e.g., verifyPrimitiveEntity) that accepts the primitive name and a creator callable (or use a parameterized test) to perform: call the creator (e.g., PrimitiveObject::createCube / createSphere / createTorus / createCone / createTube / createCapsule / createIcoSphere / createRoundedBox / createSpring) with the name, obtain Manager::getSingleton(), assert hasSceneNode(name), getSceneMgr(), assert hasEntity(name), fetch entity via sceneMgr->getEntity(name), check mesh/submesh and vertex data (use submesh->useSharedVertices ? mesh->sharedVertexData : submesh->vertexData) and EXPECT_GT(vdata->vertexCount, 0u), then call mgr->destroySceneNode(name); replace the nine TEST_F blocks with calls to this helper or a parameterized test feeding name + creator.
🤖 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/animationcontrolwidget_test.cpp`:
- Around line 227-231: Tests dereference QTableWidgetItem returned by
table->item(...) without null checks (e.g., the expression table->item(0,
1)->text()), which can segfault if the cell is empty; before calling methods on
the item, capture it into a QTableWidgetItem* (e.g., auto* item =
table->item(row, col)) and ASSERT_NE(item, nullptr) (or ASSERT_TRUE(item)) to
fail the test early, then use item->text() in the EXPECT_* assertions; apply
this guard to every occurrence in animationcontrolwidget_test.cpp where
table->item(...) is used (including the instances flagged in the review) to
avoid nullptr dereference.
- Around line 532-556: The test uses fixed resource names "MultiAnimSkel" and
"MultiAnimMesh" when calling Ogre::SkeletonManager::getSingleton().create and
Ogre::MeshManager::getSingleton().createManual which causes resource collisions
on repeated runs; update the test to either generate unique names (e.g., append
a UUID/timestamp) for the skeleton and mesh or explicitly remove the created
resources at the end of the test by calling the managers' remove() for the
created skeleton and mesh (referencing skel and mesh) so resources are cleaned
up and subsequent runs won't hit "resource already exists" errors.
In `@src/Manager_test.cpp`:
- Around line 636-638: The test uses EXPECT_TRUE for preconditions that are
immediately followed by dereferences (entity->hasSkeleton() before
entity->getSkeleton()->getNumBones() and entity->hasAnimationState("TestAnim")
before entity->getAnimationState("TestAnim")), which can lead to null-pointer
dereference on failure; change those EXPECT_* checks to ASSERT_* (e.g.,
ASSERT_TRUE(entity->hasSkeleton()) or ASSERT_NE(entity->getSkeleton(), nullptr)
and ASSERT_TRUE(entity->hasAnimationState("TestAnim")) or
ASSERT_NE(entity->getAnimationState("TestAnim"), nullptr)) so the test aborts on
failure and avoids dereferencing invalid pointers for getSkeleton(),
getNumBones(), hasAnimationState(), and getAnimationState("TestAnim").
In `@src/MCPServer_test.cpp`:
- Around line 2325-2332: The test currently constructs a fixed exportBase in the
shared temp dir (exportBase =
QDir(QDir::tempPath()).filePath("mcp_inmemory_export")) and writes/cleans files
which can collide; replace this with a QTemporaryDir instance and use its path()
to build a unique per-test directory for args["path"] (used by
server->callTool("export_mesh", args)) and adjust cleanup to rely on the
QTemporaryDir lifetime (or explicitly remove within that temp dir) so the export
.obj/.material/.mtl filenames are created inside the temporary directory rather
than a shared basename.
In `@src/PrimitivesWidget_test.cpp`:
- Around line 290-293: Add null-check assertions for mesh and submesh before
dereferencing: after obtaining auto* mesh = entity->getMesh().get() add
ASSERT_NE(mesh, nullptr), and after auto* submesh = mesh->getSubMesh(0) add
ASSERT_NE(submesh, nullptr); then keep the existing vdata selection and
ASSERT_NE(vdata, nullptr). Apply the same pattern for the other test blocks that
use getMesh(), getSubMesh(), and vdata.
---
Nitpick comments:
In `@src/Manager_test.cpp`:
- Around line 592-596: In the loop over mgr->getEntities() stop iterating once
you find the first matching Ogre::Entity: after detecting obj->getMovableType()
== "Entity" and assigning entity = static_cast<Ogre::Entity*>(obj) add an early
break so subsequent items cannot overwrite entity; update the loop that calls
mgr->getEntities(), checks getMovableType(), and assigns to entity to include
this break to make the test deterministic.
In `@src/PrimitivesWidget_test.cpp`:
- Around line 282-424: Multiple tests duplicate the same create/verify/destroy
pattern; extract a reusable helper (e.g., verifyPrimitiveEntity) that accepts
the primitive name and a creator callable (or use a parameterized test) to
perform: call the creator (e.g., PrimitiveObject::createCube / createSphere /
createTorus / createCone / createTube / createCapsule / createIcoSphere /
createRoundedBox / createSpring) with the name, obtain Manager::getSingleton(),
assert hasSceneNode(name), getSceneMgr(), assert hasEntity(name), fetch entity
via sceneMgr->getEntity(name), check mesh/submesh and vertex data (use
submesh->useSharedVertices ? mesh->sharedVertexData : submesh->vertexData) and
EXPECT_GT(vdata->vertexCount, 0u), then call mgr->destroySceneNode(name);
replace the nine TEST_F blocks with calls to this helper or a parameterized test
feeding name + creator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efa1ca29-f7f4-4541-bb82-5c91aa7c5503
📒 Files selected for processing (5)
src/MCPServer_test.cppsrc/Manager_test.cppsrc/PrimitivesWidget_test.cppsrc/animationcontrolwidget_test.cppsrc/main_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main_test.cpp
| auto* table = widget.findChild<QTableWidget*>("tableWidget"); | ||
| ASSERT_NE(table, nullptr); | ||
| // Translation X should be near 0.5 (kf1 translate) | ||
| EXPECT_FALSE(table->item(0, 1)->text().isEmpty()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the relevant sections of the test file
wc -l src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 109
🏁 Script executed:
# Check lines around 227-231, 252-256, and 439-442
sed -n '225,235p' src/animationcontrolwidget_test.cpp
echo "---"
sed -n '250,260p' src/animationcontrolwidget_test.cpp
echo "---"
sed -n '437,445p' src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1213
Guard QTableWidgetItem* before dereferencing.
At lines 230, 255, and 441, table->item(row, col) is dereferenced without a null check. Since QTableWidget::item() returns nullptr for out-of-bounds or unpopulated cells, these tests can segfault if the table is not populated as expected—particularly under Xvfb where UI state timing is unpredictable.
Apply null checks before dereferencing:
Fix pattern
- EXPECT_FALSE(table->item(0, 1)->text().isEmpty());
+ auto* txItem = table->item(0, 1);
+ ASSERT_NE(txItem, nullptr);
+ EXPECT_FALSE(txItem->text().isEmpty());- table->item(0, 1)->setText("99.5");
+ auto* txItem = table->item(0, 1);
+ ASSERT_NE(txItem, nullptr);
+ txItem->setText("99.5");📝 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.
| auto* table = widget.findChild<QTableWidget*>("tableWidget"); | |
| ASSERT_NE(table, nullptr); | |
| // Translation X should be near 0.5 (kf1 translate) | |
| EXPECT_FALSE(table->item(0, 1)->text().isEmpty()); | |
| } | |
| auto* table = widget.findChild<QTableWidget*>("tableWidget"); | |
| ASSERT_NE(table, nullptr); | |
| // Translation X should be near 0.5 (kf1 translate) | |
| auto* txItem = table->item(0, 1); | |
| ASSERT_NE(txItem, nullptr); | |
| EXPECT_FALSE(txItem->text().isEmpty()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/animationcontrolwidget_test.cpp` around lines 227 - 231, Tests
dereference QTableWidgetItem returned by table->item(...) without null checks
(e.g., the expression table->item(0, 1)->text()), which can segfault if the cell
is empty; before calling methods on the item, capture it into a
QTableWidgetItem* (e.g., auto* item = table->item(row, col)) and ASSERT_NE(item,
nullptr) (or ASSERT_TRUE(item)) to fail the test early, then use item->text() in
the EXPECT_* assertions; apply this guard to every occurrence in
animationcontrolwidget_test.cpp where table->item(...) is used (including the
instances flagged in the review) to avoid nullptr dereference.
| auto skel = Ogre::SkeletonManager::getSingleton().create( | ||
| "MultiAnimSkel", Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | ||
| auto* rootBone = skel->createBone("Root", 0); | ||
| auto* childBone = skel->createBone("Child", 1); | ||
| childBone->setPosition(Ogre::Vector3(0, 1, 0)); | ||
| rootBone->addChild(childBone); | ||
| skel->setBindingPose(); | ||
|
|
||
| // Animation 1 | ||
| auto* anim1 = skel->createAnimation("Walk", 1.0f); | ||
| auto* track1 = anim1->createNodeTrack(1); | ||
| track1->setAssociatedNode(childBone); | ||
| track1->createNodeKeyFrame(0.0f)->setTranslate(Ogre::Vector3::ZERO); | ||
| track1->createNodeKeyFrame(1.0f)->setTranslate(Ogre::Vector3(1, 0, 0)); | ||
|
|
||
| // Animation 2 | ||
| auto* anim2 = skel->createAnimation("Run", 0.5f); | ||
| auto* track2 = anim2->createNodeTrack(1); | ||
| track2->setAssociatedNode(childBone); | ||
| track2->createNodeKeyFrame(0.0f)->setTranslate(Ogre::Vector3::ZERO); | ||
| track2->createNodeKeyFrame(0.5f)->setTranslate(Ogre::Vector3(2, 0, 0)); | ||
|
|
||
| auto mesh = Ogre::MeshManager::getSingleton().createManual( | ||
| "MultiAnimMesh", Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | ||
| auto* sub = mesh->createSubMesh(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "animationcontrolwidget_test.cpp"Repository: fernandotonon/QtMeshEditor
Length of output: 105
🏁 Script executed:
wc -l src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 109
🏁 Script executed:
sed -n '532,556p' src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1241
🏁 Script executed:
sed -n '510,570p' src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2687
🏁 Script executed:
rg "MultiAnimSkel|MultiAnimMesh" src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 235
🏁 Script executed:
grep -n "class AnimationControlWidgetTest\|setUp\|TearDown\|TEST_F" src/animationcontrolwidget_test.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 1779
🏁 Script executed:
sed -n '42,58p' src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 654
🏁 Script executed:
grep -n "ResourceGroupManager\|destroy\|remove" src/animationcontrolwidget_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 243
🏁 Script executed:
sed -n '526,616p' src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 4020
🏁 Script executed:
rg "createManual|SkeletonManager.*create|MeshManager.*create" src/animationcontrolwidget_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 195
🏁 Script executed:
rg "QRandomGenerator" src/Repository: fernandotonon/QtMeshEditor
Length of output: 539
🏁 Script executed:
rg "SkeletonManager\|MeshManager" src/ -A 3 -B 1 | head -60Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
grep -r "SkeletonManager\|MeshManager" src/ --include="*.cpp" --include="*.h"Repository: fernandotonon/QtMeshEditor
Length of output: 8090
🏁 Script executed:
sed -n '1,60p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2267
🏁 Script executed:
grep -n "remove(" src/AnimationMerger_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 846
🏁 Script executed:
sed -n '85,140p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2046
Use unique Ogre resource names in tests to avoid collisions.
"MultiAnimSkel" and "MultiAnimMesh" are fixed names that persist in Ogre's resource managers. Repeated test runs in the same process will fail with "resource already exists" errors on the second execution. Other tests in the codebase (e.g., AnimationMerger_test.cpp) explicitly clean up resources via .remove(), but this test does not.
Proposed fix
+#include <QRandomGenerator>
...
- auto skel = Ogre::SkeletonManager::getSingleton().create(
- "MultiAnimSkel", Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME);
+ const QString suffix = QString::number(QRandomGenerator::global()->generate64());
+ const std::string skelName = ("MultiAnimSkel_" + suffix).toStdString();
+ const std::string meshName = ("MultiAnimMesh_" + suffix).toStdString();
+ auto skel = Ogre::SkeletonManager::getSingleton().create(
+ skelName, Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME);
...
- auto mesh = Ogre::MeshManager::getSingleton().createManual(
- "MultiAnimMesh", Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME);
+ auto mesh = Ogre::MeshManager::getSingleton().createManual(
+ meshName, Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/animationcontrolwidget_test.cpp` around lines 532 - 556, The test uses
fixed resource names "MultiAnimSkel" and "MultiAnimMesh" when calling
Ogre::SkeletonManager::getSingleton().create and
Ogre::MeshManager::getSingleton().createManual which causes resource collisions
on repeated runs; update the test to either generate unique names (e.g., append
a UUID/timestamp) for the skeleton and mesh or explicitly remove the created
resources at the end of the test by calling the managers' remove() for the
created skeleton and mesh (referencing skel and mesh) so resources are cleaned
up and subsequent runs won't hit "resource already exists" errors.
| EXPECT_TRUE(entity->hasSkeleton()); | ||
| EXPECT_EQ(entity->getSkeleton()->getNumBones(), 2u); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 670 src/Manager_test.cpp | tail -n 45Repository: fernandotonon/QtMeshEditor
Length of output: 1680
🏁 Script executed:
rg -n "EXPECT_TRUE\(entity->has(Skeleton|AnimationState)" src/Manager_test.cpp -A 2 -B 1Repository: fernandotonon/QtMeshEditor
Length of output: 363
Use ASSERT_* for skeleton/animation preconditions before dereference.
At lines 636–637, EXPECT_TRUE(entity->hasSkeleton()) is immediately followed by EXPECT_EQ(entity->getSkeleton()->getNumBones(), 2u). Similarly, at lines 649–652, EXPECT_TRUE(entity->hasSkeleton()) and EXPECT_TRUE(entity->hasAnimationState("TestAnim")) precede a direct call to entity->getAnimationState("TestAnim"). If these expectations fail, the test continues and dereferences potentially invalid pointers, causing a crash rather than a clean assertion failure. Use ASSERT_TRUE or ASSERT_NE for preconditions that guard against null or invalid pointer dereference.
Proposed fix
- EXPECT_TRUE(entity->hasSkeleton());
- EXPECT_EQ(entity->getSkeleton()->getNumBones(), 2u);
+ ASSERT_TRUE(entity->hasSkeleton());
+ ASSERT_NE(entity->getSkeleton(), nullptr);
+ EXPECT_EQ(entity->getSkeleton()->getNumBones(), 2u);
...
- EXPECT_TRUE(entity->hasSkeleton());
- EXPECT_TRUE(entity->hasAnimationState("TestAnim"));
-
- auto* state = entity->getAnimationState("TestAnim");
+ ASSERT_TRUE(entity->hasSkeleton());
+ ASSERT_TRUE(entity->hasAnimationState("TestAnim"));
+ auto* state = entity->getAnimationState("TestAnim");
+ ASSERT_NE(state, nullptr);
EXPECT_NEAR(state->getLength(), 1.0f, 0.01f);Also applies to: 649–653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Manager_test.cpp` around lines 636 - 638, The test uses EXPECT_TRUE for
preconditions that are immediately followed by dereferences
(entity->hasSkeleton() before entity->getSkeleton()->getNumBones() and
entity->hasAnimationState("TestAnim") before
entity->getAnimationState("TestAnim")), which can lead to null-pointer
dereference on failure; change those EXPECT_* checks to ASSERT_* (e.g.,
ASSERT_TRUE(entity->hasSkeleton()) or ASSERT_NE(entity->getSkeleton(), nullptr)
and ASSERT_TRUE(entity->hasAnimationState("TestAnim")) or
ASSERT_NE(entity->getAnimationState("TestAnim"), nullptr)) so the test aborts on
failure and avoids dereferencing invalid pointers for getSkeleton(),
getNumBones(), hasAnimationState(), and getAnimationState("TestAnim").
| const QString exportBase = QDir(QDir::tempPath()).filePath("mcp_inmemory_export"); | ||
| args["path"] = exportBase + ".obj"; | ||
| QJsonObject result = server->callTool("export_mesh", args); | ||
| EXPECT_FALSE(isError(result)); | ||
|
|
||
| QFile::remove(exportBase + ".obj"); | ||
| QFile::remove(exportBase + ".material"); | ||
| QFile::remove(exportBase + ".mtl"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the exact code at lines 2325-2332 and surrounding context
head -2332 src/MCPServer_test.cpp | tail -20Repository: fernandotonon/QtMeshEditor
Length of output: 912
🏁 Script executed:
# Check the include section to see what's already included
head -20 src/MCPServer_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 572
🏁 Script executed:
# Search for other temp file patterns and QTemporaryDir usage in the file
grep -n "QTemporaryDir\|QDir::tempPath\|tempPath\|mcp_inmemory" src/MCPServer_test.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 161
🏁 Script executed:
# Check if this test function is isolated or run in parallel
grep -n "^TEST\|^class.*Test" src/MCPServer_test.cpp | grep -B2 -A2 "2325\|export_mesh"Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Verify what files are cleaned up after the test
sed -n '2320,2340p' src/MCPServer_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 765
Use unique temporary directories to avoid test file collisions.
Line 2325 uses a fixed basename in the shared system temp directory. Concurrent or repeated test runs can cause file conflicts. Use QTemporaryDir to create a unique per-test directory instead.
Proposed fix
+#include <QTemporaryDir>
...
- const QString exportBase = QDir(QDir::tempPath()).filePath("mcp_inmemory_export");
+ QTemporaryDir tempDir;
+ ASSERT_TRUE(tempDir.isValid());
+ const QString exportBase = QDir(tempDir.path()).filePath("mcp_inmemory_export");
args["path"] = exportBase + ".obj";📝 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.
| const QString exportBase = QDir(QDir::tempPath()).filePath("mcp_inmemory_export"); | |
| args["path"] = exportBase + ".obj"; | |
| QJsonObject result = server->callTool("export_mesh", args); | |
| EXPECT_FALSE(isError(result)); | |
| QFile::remove(exportBase + ".obj"); | |
| QFile::remove(exportBase + ".material"); | |
| QFile::remove(exportBase + ".mtl"); | |
| QTemporaryDir tempDir; | |
| ASSERT_TRUE(tempDir.isValid()); | |
| const QString exportBase = QDir(tempDir.path()).filePath("mcp_inmemory_export"); | |
| args["path"] = exportBase + ".obj"; | |
| QJsonObject result = server->callTool("export_mesh", args); | |
| EXPECT_FALSE(isError(result)); | |
| QFile::remove(exportBase + ".obj"); | |
| QFile::remove(exportBase + ".material"); | |
| QFile::remove(exportBase + ".mtl"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 2325 - 2332, The test currently
constructs a fixed exportBase in the shared temp dir (exportBase =
QDir(QDir::tempPath()).filePath("mcp_inmemory_export")) and writes/cleans files
which can collide; replace this with a QTemporaryDir instance and use its path()
to build a unique per-test directory for args["path"] (used by
server->callTool("export_mesh", args)) and adjust cleanup to rely on the
QTemporaryDir lifetime (or explicitly remove within that temp dir) so the export
.obj/.material/.mtl filenames are created inside the temporary directory rather
than a shared basename.
| auto* mesh = entity->getMesh().get(); | ||
| auto* submesh = mesh->getSubMesh(0); | ||
| Ogre::VertexData* vdata = submesh->useSharedVertices ? mesh->sharedVertexData : submesh->vertexData; | ||
| ASSERT_NE(vdata, nullptr); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the test file exists and read the relevant section
if [ -f "src/PrimitivesWidget_test.cpp" ]; then
echo "=== Lines 280-295 (around first flagged issue) ==="
sed -n '280,295p' src/PrimitivesWidget_test.cpp | cat -n
echo ""
echo "=== Lines 305-315 (around second flagged issue) ==="
sed -n '305,315p' src/PrimitivesWidget_test.cpp | cat -n
echo ""
echo "=== Check for TEST_F or fixture setup ==="
head -n 50 src/PrimitivesWidget_test.cpp | grep -A 20 "TEST_F\|SetUp\|class.*Test"
else
echo "File not found"
fiRepository: fernandotonon/QtMeshEditor
Length of output: 2395
🏁 Script executed:
# Also check for patterns with mesh/submesh access across multiple test locations
rg -n "getEntity|getMesh|getSubMesh" src/PrimitivesWidget_test.cpp -A 3 -B 1Repository: fernandotonon/QtMeshEditor
Length of output: 3647
Add mesh/submesh existence assertions before dereferencing.
The pattern across nine tests dereferences mesh and submesh without null guards. If a primitive fails to create or mesh retrieval breaks, the test will segfault instead of reporting a clean assertion failure.
Proposed fix pattern
auto* entity = sceneMgr->getEntity("VerifyCube");
auto* mesh = entity->getMesh().get();
+ ASSERT_NE(mesh, nullptr);
+ ASSERT_GT(mesh->getNumSubMeshes(), 0u);
auto* submesh = mesh->getSubMesh(0);
+ ASSERT_NE(submesh, nullptr);
Ogre::VertexData* vdata = submesh->useSharedVertices ? mesh->sharedVertexData : submesh->vertexData;
ASSERT_NE(vdata, nullptr);Also applies to: 306–309, 322–325, 338–341, 354–357, 370–373, 386–389, 402–405, 418–421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PrimitivesWidget_test.cpp` around lines 290 - 293, Add null-check
assertions for mesh and submesh before dereferencing: after obtaining auto* mesh
= entity->getMesh().get() add ASSERT_NE(mesh, nullptr), and after auto* submesh
= mesh->getSubMesh(0) add ASSERT_NE(submesh, nullptr); then keep the existing
vdata selection and ASSERT_NE(vdata, nullptr). Apply the same pattern for the
other test blocks that use getMesh(), getSubMesh(), and vdata.
|
1 participant
Summary
TestHelpers.h(createInMemoryTriangleMesh,createInMemorySkeletonMesh,createAnimatedTestEntity) enabling tests to create entities without loading files from diskabout_test.cppandmain_test.cppwhere local QApplication creation destroyed the global one fromtest_main.cpp, breaking all subsequent test suitesTest plan
tryInitOgre()/canLoadMeshFiles()guards🤖 Generated with Claude Code
Summary by CodeRabbit