Increase coverage across CLI pipeline, camera/view cube, and exporter tests - #233
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 Ogre- and Qt-backed unit/integration tests and in-test helpers: in-memory mesh and 1×1 texture creation, a CLIPipeline material/texture deduplication test, Collada/OBJ exporter tests, MCP JSON‑RPC transport framing and EOF handling tests, and SpaceCamera/ViewCubeController widget integration tests with lifecycle management. Changes
Sequence Diagram(s)(No sequence diagrams generated.) 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: 293972d8da
ℹ️ 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".
| } | ||
| ASSERT_NE(mainWindow, nullptr); | ||
|
|
||
| viewport = new EditorViewport(mainWindow, 31); |
There was a problem hiding this comment.
SpaceCameraWidgetIntegrationTest creates an EditorViewport unconditionally, but this constructor immediately builds an OgreWidget/render window; in headless or no-OpenGL environments that can fail before any test assertions run. Other Ogre-dependent fixtures in this repo use tryInitOgre()/canLoadMeshFiles() to skip gracefully, so this new path can turn an environment limitation into a hard test failure (or crash) instead of a skip.
Useful? React with 👍 / 👎.
| } | ||
| ASSERT_NE(mainWindow, nullptr); | ||
|
|
||
| viewport = new EditorViewport(mainWindow, 41); |
There was a problem hiding this comment.
This integration fixture also constructs EditorViewport without checking Ogre/render-window availability first. Because EditorViewport eagerly creates OgreWidget, these tests can fail hard on CI agents lacking a usable GL/display setup rather than being skipped like the rest of the Ogre tests, which makes the suite brittle across environments.
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: 5
🧹 Nitpick comments (2)
src/MeshImporterExporter_test.cpp (1)
76-95: Consider adding a null check for the created texture.If
createManualfails (e.g., due to a duplicate texture name), the subsequentgetBuffer()call will crash without a clear error message. Adding an assertion would improve test debuggability.🔧 Proposed defensive check
static Ogre::TexturePtr createSolidTexture2D(const std::string& name, uint32_t argb = 0xFFFFFFFF) { Ogre::TexturePtr texture = Ogre::TextureManager::getSingleton().createManual( name, Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME, Ogre::TEX_TYPE_2D, 1, 1, 0, Ogre::PF_A8R8G8B8, Ogre::TU_STATIC_WRITE_ONLY); + if (!texture) + return nullptr; Ogre::HardwarePixelBufferSharedPtr pixelBuffer = texture->getBuffer(0, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 76 - 95, The createSolidTexture2D function should guard against a null Ogre::TexturePtr returned by Ogre::TextureManager::getSingleton().createManual; check the texture pointer after createManual (in createSolidTexture2D) and before calling texture->getBuffer() or texture->load(), and if it is null fail fast (e.g., assert, throw an informative exception, or call the test logger) with a message that includes the texture name and intent; this prevents a crash when createManual fails (e.g., duplicate name) and makes failures clear during tests.src/MCPServer_test.cpp (1)
14-15: Consider: Platform-specific headers limit Windows CI compatibility.The POSIX headers
<unistd.h>and<fcntl.h>and their APIs (pipe(),fcntl(), etc.) are used throughout this test file without platform guards. While the new tests follow the established pattern, this prevents the test suite from compiling on Windows.If Windows CI is needed in the future, consider wrapping POSIX-dependent tests with
#ifndef Q_OS_WINor using cross-platform abstractions likeQProcessfor pipe operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 14 - 15, The test includes POSIX-only headers (<unistd.h>, <fcntl.h>) and uses APIs like pipe() and fcntl() which break Windows builds; guard the POSIX-specific code by wrapping the includes and any tests/functions that call pipe(), fcntl(), etc. with a platform check (e.g., `#ifndef` Q_OS_WIN ... `#endif`) or replace the pipe/fd logic with a cross-platform alternative such as QProcess; update the include block and any test cases in this file that reference pipe()/fcntl() to live inside the same guard or be rewritten to use QProcess.
🤖 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 689-705: The test currently uses hard ASSERTs for Ogre-backed
objects which can fail in headless/Xvfb; wrap the EditorViewport creation and
subsequent steps in try/catch blocks (or check for nullptr and call GTEST_SKIP)
similar to the MainWindow creation skip: catch exceptions from new
EditorViewport(mainWindow, 31) and skip with GTEST_SKIP on failure, and if
viewport->getOgreWidget() or widget->getSpaceCamera() or camera->getCamera()
return nullptr then call GTEST_SKIP with a descriptive message instead of
ASSERT_NE to avoid failing the whole suite when Ogre cannot fully initialize.
- Around line 707-709: TearDown unconditionally calls
SelectionSet::getSingleton()->clear() which can dereference a null/non-created
singleton after partial/early-exit setup; change TearDown() to guard against a
missing instance by using SelectionSet::getSingletonPtr() and only call clear()
(or better, call SelectionSet::kill()) when the pointer is non-null so teardown
never dereferences a non-existent singleton.
- Around line 741-751: The test currently only verifies animation stopped; add
an assertion that the camera's final orientation matches the requested target
after the frame update: after calling camera->frameStarted(frameEvent) (and
before/after checking camera->isAnimating()), fetch the camera's current
orientation (via the method or member used in this test fixture) and assert it
equals (or is approximately equal to) the Ogre::Quaternion target passed to
camera->animateToOrientation; use a quaternion-equality or component-wise
tolerance check rather than exact float equality if needed.
In `@src/ViewCube/ViewCubeController_test.cpp`:
- Around line 362-379: The fixture should skip when Ogre-backed objects can't be
created instead of using fatal ASSERTs; replace the ASSERT_NE checks for
EditorViewport, widget (EditorViewport::getOgreWidget), SpaceCamera
(widget->getSpaceCamera) and the ViewCubeController setup (ViewCubeController
and setActiveWidget) with runtime checks that call GTEST_SKIP() with a clear
message if any pointer is null or construction throws. Wrap EditorViewport and
widget/camera creation in try/catch (or check for null returns) and skip the
test on failure, then only proceed to construct controller and call
controller->setActiveWidget(widget) when all Ogre objects are non-null.
- Around line 420-437: The test currently only checks that snapToDirection(10.0,
0.0, 0.0) changed the camera orientation; change it to verify normalization and
correct pointing: after calling controller->snapToDirection(...) and advancing a
frame, read camera->getOrientation() (the after quaternion) and use it to
compute the camera's forward vector (apply the quaternion to the camera's
default forward axis, e.g. Ogre::Vector3::UNIT_Z or UNIT_X used by your camera).
Normalize the input direction (10,0,0) to (1,0,0) and assert the transformed
forward vector is nearly equal to that normalized direction (within a small
epsilon), and also assert the quaternion is normalized (length ~ 1) to prove the
normalization path rather than just any rotation.
---
Nitpick comments:
In `@src/MCPServer_test.cpp`:
- Around line 14-15: The test includes POSIX-only headers (<unistd.h>,
<fcntl.h>) and uses APIs like pipe() and fcntl() which break Windows builds;
guard the POSIX-specific code by wrapping the includes and any tests/functions
that call pipe(), fcntl(), etc. with a platform check (e.g., `#ifndef` Q_OS_WIN
... `#endif`) or replace the pipe/fd logic with a cross-platform alternative such
as QProcess; update the include block and any test cases in this file that
reference pipe()/fcntl() to live inside the same guard or be rewritten to use
QProcess.
In `@src/MeshImporterExporter_test.cpp`:
- Around line 76-95: The createSolidTexture2D function should guard against a
null Ogre::TexturePtr returned by
Ogre::TextureManager::getSingleton().createManual; check the texture pointer
after createManual (in createSolidTexture2D) and before calling
texture->getBuffer() or texture->load(), and if it is null fail fast (e.g.,
assert, throw an informative exception, or call the test logger) with a message
that includes the texture name and intent; this prevents a crash when
createManual fails (e.g., duplicate name) and makes failures clear during tests.
🪄 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: cbb4c225-6b48-46c9-b381-47185c32d45a
📒 Files selected for processing (5)
src/CLIPipeline_test.cppsrc/MCPServer_test.cppsrc/MeshImporterExporter_test.cppsrc/SpaceCamera_test.cppsrc/ViewCube/ViewCubeController_test.cpp
|
Addressed CodeRabbit actionable items in follow-up commit
I left the POSIX include note in |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/ViewCube/ViewCubeController_test.cpp (1)
381-390:⚠️ Potential issue | 🟠 MajorComplete the skip path for partial Ogre/QML setup.
Unlike the
SpaceCamerafixture, this block treatscamera != nullptras fully ready and performs controller setup outside a guarded skip path. If headless init yields aSpaceCamerawithout its Ogre camera, orViewCubeControllersetup throws, these tests still fail instead of reporting skipped integration coverage.💡 Suggested hardening
camera = widget->getSpaceCamera(); - if (!camera) { - GTEST_SKIP() << "Skipping: SpaceCamera is null"; + if (!camera || !camera->getCamera()) { + GTEST_SKIP() << "Skipping: SpaceCamera not fully initialized"; } - controller = new ViewCubeController(mainWindow); - if (!controller) { - GTEST_SKIP() << "Skipping: ViewCubeController creation failed"; + try { + controller = new ViewCubeController(mainWindow); + if (!controller) { + GTEST_SKIP() << "Skipping: ViewCubeController creation failed"; + } + controller->setActiveWidget(widget); + } catch (...) { + GTEST_SKIP() << "Skipping: ViewCubeController setup failed"; } - controller->setActiveWidget(widget);Based on learnings: Tests must work under Xvfb (headless X11) — avoid assumptions about a real display.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController_test.cpp` around lines 381 - 390, The test assumes a fully-initialized camera and controller; harden it by treating partial headless init as a skip: after obtaining camera via widget->getSpaceCamera(), verify the underlying Ogre camera is present (e.g., camera->getOgreCamera() or equivalent) and call GTEST_SKIP if it’s null, then wrap ViewCubeController creation and controller->setActiveWidget(widget) in a guarded block (check controller != nullptr and/or wrap in try/catch) and call GTEST_SKIP with a clear message if creation or setup fails so tests are skipped instead of failing on headless/Xvfb setups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ViewCube/ViewCubeController_test.cpp`:
- Around line 381-390: The test assumes a fully-initialized camera and
controller; harden it by treating partial headless init as a skip: after
obtaining camera via widget->getSpaceCamera(), verify the underlying Ogre camera
is present (e.g., camera->getOgreCamera() or equivalent) and call GTEST_SKIP if
it’s null, then wrap ViewCubeController creation and
controller->setActiveWidget(widget) in a guarded block (check controller !=
nullptr and/or wrap in try/catch) and call GTEST_SKIP with a clear message if
creation or setup fails so tests are skipped instead of failing on headless/Xvfb
setups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdcffb8d-f4cc-49e7-bff2-e2868f915f33
📒 Files selected for processing (3)
src/MeshImporterExporter_test.cppsrc/SpaceCamera_test.cppsrc/ViewCube/ViewCubeController_test.cpp
✅ Files skipped from review due to trivial changes (1)
- src/MeshImporterExporter_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ViewCube/ViewCubeController_test.cpp`:
- Around line 429-433: The test currently asserts fixed signs of quaternion
components from camera->getOrientation() (q) using EXPECT_NEAR on
q.w/q.x/q.y/q.z; since q and -q represent the same rotation this can fail
nondeterministically—change the assertions to be sign-agnostic (e.g., compare
std::abs(q.w), std::abs(q.x), std::abs(q.y), std::abs(q.z) to the expected
magnitudes or assert that either q or -q matches the expected components) so the
test verifies orientation equivalence rather than raw component signs.
🪄 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: 9c6269c1-f602-4c2a-ace6-9013ed35a5f3
📒 Files selected for processing (4)
src/MCPServer_test.cppsrc/MeshImporterExporter_test.cppsrc/SpaceCamera_test.cppsrc/ViewCube/ViewCubeController_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/MeshImporterExporter_test.cpp
- src/SpaceCamera_test.cpp
- src/MCPServer_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ViewCube/ViewCubeController_test.cpp (1)
386-394: Redundant null check afternewoperator.The check
if (!controller)on line 388 is unnecessary —newin C++ throwsstd::bad_allocon allocation failure rather than returning null. The enclosing try/catch block already handles this case.💡 Suggested simplification
try { controller = new ViewCubeController(mainWindow); - if (!controller) { - GTEST_SKIP() << "Skipping: ViewCubeController creation failed"; - } controller->setActiveWidget(widget); } catch (...) { GTEST_SKIP() << "Skipping: ViewCubeController setup failed"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController_test.cpp` around lines 386 - 394, Remove the redundant null check after allocation: the new ViewCubeController(mainWindow) call will throw on failure, so eliminate the `if (!controller)` block and its GTEST_SKIP; instead keep the try/catch around the allocation and the subsequent `controller->setActiveWidget(widget)` call (symbols: ViewCubeController, controller, mainWindow, setActiveWidget) so allocation failures are handled by the existing catch that triggers GTEST_SKIP.
🤖 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/ViewCube/ViewCubeController_test.cpp`:
- Around line 386-394: Remove the redundant null check after allocation: the new
ViewCubeController(mainWindow) call will throw on failure, so eliminate the `if
(!controller)` block and its GTEST_SKIP; instead keep the try/catch around the
allocation and the subsequent `controller->setActiveWidget(widget)` call
(symbols: ViewCubeController, controller, mainWindow, setActiveWidget) so
allocation failures are handled by the existing catch that triggers GTEST_SKIP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f147279-008d-4770-b20a-23248820f292
📒 Files selected for processing (1)
src/ViewCube/ViewCubeController_test.cpp
|
1 participant
Summary
CLIPipeline::extractMeshInfomaterial/texture dedupe pathsSpaceCameracamera-control paths (animation, wheel branches, mouse pan/roll, frame selection)ViewCubeControlleractive-camera paths (snapToView,snapToDirection,rotateByDelta, orientation sync)MCPServerprotocol tests for additional JSON-RPC dispatch and EOF handling branchesMeshImporterExporterNotes
Qt6Config.cmakemissing); validation will run in CIFiles
src/CLIPipeline_test.cppsrc/SpaceCamera_test.cppsrc/ViewCube/ViewCubeController_test.cppsrc/MCPServer_test.cppsrc/MeshImporterExporter_test.cppSummary by CodeRabbit