[codex] increase CLIPipeline test coverage for info/anim/validate/lod branches - #255
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a test-only helper to generate/export an in-memory Ogre triangle mesh and extends CLI in-process tests (cmdInfo, cmdAnim --list/--merge, cmdValidate, cmdLod) to exercise error and success paths using exported temp meshes; also increases MainWindow construction retry attempts and introduces Manager::kill()/event-processing and attempt-scaled backoff across several widget/view tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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/CLIPipeline_test.cpp`:
- Around line 1049-1084: The test TEST_F(CLIPipelineCmdTest,
CmdAnimList_NoAnimationsReturnsSuccess) is asserting success (0) for
CLIPipeline::cmdAnim when a mesh has zero animations, but the command contract
defines this as an error (return 1); update the two expectations calling
CLIPipeline::cmdAnim (the non-JSON and JSON invocations created via TestArgv
textArgs and jsonArgs) to expect 1 instead of 0 so the test matches the command
behavior (or alternatively rename the test and change CLIPipeline::cmdAnim if
you intend to change the command contract).
- Around line 666-685: The helper exportGeneratedTriangleMesh(...) creates a
mesh, scene node and entity but never tears them down, leaking scene state;
after the export (before each return) call the appropriate teardown:
remove/destroy the created Entity and SceneNode via the Manager singleton (e.g.,
use Manager::getSingleton()->destroyEntity(entity) or removeSceneNode(nodeName)
/ removeSceneNode(node) and then destroy the node), and also remove the
in-memory mesh from Ogre::MeshManager (e.g.,
Ogre::MeshManager::getSingleton().remove(meshName) or equivalent) so the mesh,
entity and node are fully cleaned up before returning; ensure these teardown
calls run on both success and failure paths in exportGeneratedTriangleMesh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/CLIPipeline_test.cpp (1)
1069-1104: Consider removing the in-memory mesh from MeshManager after export for test isolation.The mesh created by
createInMemorySkeletonMesh("cli_no_anim_mesh")remains inOgre::MeshManagerafter the test completes. While scene objects are properly cleaned up (lines 1089-1093), the mesh resource persists. If tests are run repeatedly in the same process, this could cause conflicts or stale state.Suggested cleanup addition
auto nodes = manager->getSceneNodes(); for (auto* n : nodes) { manager->destroyAllAttachedMovableObjects(n); manager->destroySceneNode(n); } + if (auto old = Ogre::MeshManager::getSingleton().getByName("cli_no_anim_mesh")) + Ogre::MeshManager::getSingleton().remove(old); QByteArray sourceBa = sourceFile.toUtf8();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1069 - 1104, The test leaves the in-memory mesh created by createInMemorySkeletonMesh("cli_no_anim_mesh") registered in Ogre::MeshManager, so after exporting and tearing down the scene you should explicitly remove/unregister that mesh from MeshManager to avoid cross-test pollution; locate the mesh variable (mesh) created in CmdAnimList_NoAnimationsGeneratedMeshReturnsError and after destroying scene nodes (or immediately after exporter calls) call the appropriate MeshManager singleton remove/unload method for mesh->getName() (or remove the resource by name "cli_no_anim_mesh") to ensure the resource is removed from Ogre::MeshManager before the test exits.
🤖 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/CLIPipeline_test.cpp`:
- Around line 1069-1104: The test leaves the in-memory mesh created by
createInMemorySkeletonMesh("cli_no_anim_mesh") registered in Ogre::MeshManager,
so after exporting and tearing down the scene you should explicitly
remove/unregister that mesh from MeshManager to avoid cross-test pollution;
locate the mesh variable (mesh) created in
CmdAnimList_NoAnimationsGeneratedMeshReturnsError and after destroying scene
nodes (or immediately after exporter calls) call the appropriate MeshManager
singleton remove/unload method for mesh->getName() (or remove the resource by
name "cli_no_anim_mesh") to ensure the resource is removed from
Ogre::MeshManager before the test exits.
|
1 participant
Summary
CLIPipelinetests that exercise additionalcmdInfo,cmdAnim,cmdValidate, andcmdLodbranch pathsWhy
Coverage regressed after merges, with notable gaps in CLI command handling branches. These tests target previously untested decision paths and error/success flows using generated meshes instead of external fixtures.
Validation
cmake --build . --target UnitTests -j6./bin/UnitTests --gtest_filter='CLIPipelineFormatForExtension.CaseInsensitive'xvfb-run -a ./bin/UnitTests --gtest_filter='CLIPipelineCmdTest.CmdInfo_InvalidExistingFile:CLIPipelineCmdTest.CmdAnimList_NoAnimationsReturnsSuccess:CLIPipelineCmdTest.CmdAnimMerge_WithoutSourcesReturnsError:CLIPipelineCmdValidateTest.CmdValidate_SucceedsForGeneratedMeshTextAndJson:CLIPipelineCmdLodTest.CmdLod_InfoAndRemoveFromGeneratedMesh'(local env reports Ogre unavailable so these skip locally; CI environment will run with project CI setup)Summary by CodeRabbit