Expand selection and mesh IO coverage#226
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds multiple unit tests (MeshImporterExporter, SelectionSet, OgreWidget) and a small test-only public wrapper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MeshImporterExporter_test.cpp (1)
16-18: Consider using afrienddeclaration or test accessor instead of#define private public.This preprocessor hack is undefined behavior in C++ and can break with certain compilers or when standard library headers are inadvertently affected. A cleaner approach is adding a
friend classdeclaration inMeshImporterExporter.hfor the test class, or providing explicit test accessors.That said, this pattern is commonly used in test code and works reliably in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 16 - 18, Replace the preprocessor hack in MeshImporterExporter_test.cpp that temporarily redefines access specifiers with a safer test access mechanism: add a friend declaration in MeshImporterExporter.h (e.g., friend class MeshImporterExporterTest) or expose explicit test accessor methods on the MeshImporterExporter class so the test can access internals without using `#define private public`; update MeshImporterExporter_test.cpp to remove the define/undef and use the new friend class name or accessors (look for MeshImporterExporter_test.cpp and MeshImporterExporter.h and the MeshImporterExporter class to make the change).
🤖 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/MeshImporterExporter_test.cpp`:
- Around line 16-18: Replace the preprocessor hack in
MeshImporterExporter_test.cpp that temporarily redefines access specifiers with
a safer test access mechanism: add a friend declaration in
MeshImporterExporter.h (e.g., friend class MeshImporterExporterTest) or expose
explicit test accessor methods on the MeshImporterExporter class so the test can
access internals without using `#define private public`; update
MeshImporterExporter_test.cpp to remove the define/undef and use the new friend
class name or accessors (look for MeshImporterExporter_test.cpp and
MeshImporterExporter.h and the MeshImporterExporter class to make the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9f9368d-4736-4415-ac88-009156f966ad
📒 Files selected for processing (2)
src/MeshImporterExporter_test.cppsrc/SelectionSet_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/MeshImporterExporter_test.cpp (2)
392-416: Tighten texture-progress assertions to reduce false positives.The
Not(Contains("Exporting textures (2/3)..."))check is very specific. Consider asserting the count of"Exporting textures ("messages is exactly 1, then keep the explicit"(1/1)"assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 392 - 416, Replace the fragile negative assertion with an exact-count check: after calling MeshImporterExporter::sceneExporter and populating progressMessages, assert that progressMessages contains the explicit "Exporting textures (1/1)..." message (already present) and also assert that the number of entries in progressMessages that contain the substring "Exporting textures (" is exactly 1 (operate on the progressMessages QStringList and count occurrences), instead of using ::testing::Not(::testing::Contains(...)); reference progressMessages and MeshImporterExporter::sceneExporter to locate the test logic to update.
753-779: Consider asserting camera distance magnitude, not only sign.Current checks verify direction (
z < 0) but not the actual sizing math. Adding anEXPECT_NEARfor-size / (2*tan(fov/2))would harden this regression test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 753 - 779, The test only asserts the camera is placed on the negative Z axis but not at the expected distance; after calling MeshImporterExporter::configureCameraForTesting(entity) compute the expected distance using the entity’s size (e.g., bounding box half-size or bounding radius derived from the mesh/entity) and the camera FOV (use camera->getFOVy()), then add an EXPECT_NEAR comparing camera->getParentSceneNode()->getPosition().z to -size/(2*tan(fov/2)) with a small tolerance (e.g., 1e-3) so the test verifies the magnitude as well as the sign.src/MeshImporterExporter.h (1)
51-51: Gate test-only camera wrapper to test builds using existing BUILD_TESTS flag.
configureCameraForTestingat line 51 is used only insrc/MeshImporterExporter_test.cpp:773but declared as a public method, unnecessarily broadening the production API surface. The repository already has aBUILD_TESTSCMake flag that can gate this method to test builds only (similar to other test-specific infrastructure in the codebase). Wrap this declaration with#ifdef BUILD_TESTSor move it to a test-specific accessor file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.h` at line 51, The public test-only wrapper method configureCameraForTesting currently exposes test-only API; restrict it to test builds by wrapping its declaration in an `#ifdef` BUILD_TESTS (or alternatively move it into a test-only header/source) so it is only compiled when BUILD_TESTS is enabled; locate the static method configureCameraForTesting (which calls configureCamera) in MeshImporterExporter.h and gate that declaration with the BUILD_TESTS macro mirroring the project's other test-only guards.
🤖 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/MeshImporterExporter_test.cpp`:
- Around line 392-416: Replace the fragile negative assertion with an
exact-count check: after calling MeshImporterExporter::sceneExporter and
populating progressMessages, assert that progressMessages contains the explicit
"Exporting textures (1/1)..." message (already present) and also assert that the
number of entries in progressMessages that contain the substring "Exporting
textures (" is exactly 1 (operate on the progressMessages QStringList and count
occurrences), instead of using ::testing::Not(::testing::Contains(...));
reference progressMessages and MeshImporterExporter::sceneExporter to locate the
test logic to update.
- Around line 753-779: The test only asserts the camera is placed on the
negative Z axis but not at the expected distance; after calling
MeshImporterExporter::configureCameraForTesting(entity) compute the expected
distance using the entity’s size (e.g., bounding box half-size or bounding
radius derived from the mesh/entity) and the camera FOV (use camera->getFOVy()),
then add an EXPECT_NEAR comparing camera->getParentSceneNode()->getPosition().z
to -size/(2*tan(fov/2)) with a small tolerance (e.g., 1e-3) so the test verifies
the magnitude as well as the sign.
In `@src/MeshImporterExporter.h`:
- Line 51: The public test-only wrapper method configureCameraForTesting
currently exposes test-only API; restrict it to test builds by wrapping its
declaration in an `#ifdef` BUILD_TESTS (or alternatively move it into a test-only
header/source) so it is only compiled when BUILD_TESTS is enabled; locate the
static method configureCameraForTesting (which calls configureCamera) in
MeshImporterExporter.h and gate that declaration with the BUILD_TESTS macro
mirroring the project's other test-only guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d834d173-924f-411f-8172-722a44caa657
📒 Files selected for processing (3)
src/MeshImporterExporter.hsrc/MeshImporterExporter_test.cppsrc/SelectionSet_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SelectionSet_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/OgreWidget_test.cpp (1)
33-33: Prefer event flushing over fixedmsleep(50)delays in tests.Fixed sleeps make tests slower and timing-sensitive. A bounded event-processing helper is usually more deterministic.
Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OgreWidget_test.cpp` at line 33, Replace the fixed QThread::msleep(50) calls with a bounded event-processing helper that flushes the Qt event loop until the expected condition or a short timeout is reached; specifically, remove QThread::msleep(50) and instead call a helper (e.g., processEventsUntil(condition, timeoutMs) or a small loop using QCoreApplication::processEvents(QEventLoop::AllEvents) + QElapsedTimer) in the test containing the QThread::msleep(50) occurrences (both the one at the shown location and the similar call at the other occurrence), ensuring the loop checks the test condition and exits on timeout to keep tests deterministic and fast.
🤖 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/OgreWidget_test.cpp`:
- Around line 35-46: The test currently hard-fails with ASSERT_NE when render
setup is unavailable; change the creation of EditorViewport and retrieval of the
Ogre widget to follow the same skip pattern as MainWindow: after attempting
viewport = new EditorViewport(mainWindow, 7) and widget =
viewport->getOgreWidget(), if either construction throws or returns nullptr then
call GTEST_SKIP() with a clear message (e.g., "Skipping: render context
unavailable") instead of ASSERT_NE; update references in this block (MainWindow,
EditorViewport, getOgreWidget, viewport, widget) so the test gracefully skips
under Xvfb/headless environments.
---
Nitpick comments:
In `@src/OgreWidget_test.cpp`:
- Line 33: Replace the fixed QThread::msleep(50) calls with a bounded
event-processing helper that flushes the Qt event loop until the expected
condition or a short timeout is reached; specifically, remove
QThread::msleep(50) and instead call a helper (e.g.,
processEventsUntil(condition, timeoutMs) or a small loop using
QCoreApplication::processEvents(QEventLoop::AllEvents) + QElapsedTimer) in the
test containing the QThread::msleep(50) occurrences (both the one at the shown
location and the similar call at the other occurrence), ensuring the loop checks
the test condition and exits on timeout to keep tests deterministic and fast.
🪄 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: 10702224-2240-4d10-b9f1-9cb86d23008b
📒 Files selected for processing (2)
src/OgreWidget.cppsrc/OgreWidget_test.cpp
💤 Files with no reviewable changes (1)
- src/OgreWidget.cpp
| try { | ||
| mainWindow = new MainWindow(); | ||
| } catch (...) { | ||
| GTEST_SKIP() << "Skipping: MainWindow creation failed"; | ||
| } | ||
| ASSERT_NE(mainWindow, nullptr); | ||
|
|
||
| viewport = new EditorViewport(mainWindow, 7); | ||
| ASSERT_NE(viewport, nullptr); | ||
|
|
||
| widget = viewport->getOgreWidget(); | ||
| ASSERT_NE(widget, nullptr); |
There was a problem hiding this comment.
Skip render-dependent tests when Ogre context is unavailable.
ASSERT_NE on viewport/widget hard-fails in environments where render setup is unavailable; this should follow the same skip strategy used for MainWindow creation so Xvfb runs remain stable.
Proposed change
- try {
- mainWindow = new MainWindow();
- } catch (...) {
- GTEST_SKIP() << "Skipping: MainWindow creation failed";
- }
- ASSERT_NE(mainWindow, nullptr);
-
- viewport = new EditorViewport(mainWindow, 7);
- ASSERT_NE(viewport, nullptr);
-
- widget = viewport->getOgreWidget();
- ASSERT_NE(widget, nullptr);
+ try {
+ mainWindow = new MainWindow();
+ viewport = new EditorViewport(mainWindow, 7);
+ widget = viewport ? viewport->getOgreWidget() : nullptr;
+ } catch (...) {
+ GTEST_SKIP() << "Skipping: render setup failed in this environment";
+ }
+
+ if (mainWindow == nullptr || viewport == nullptr || widget == nullptr || widget->getSpaceCamera() == nullptr)
+ GTEST_SKIP() << "Skipping: Ogre render context unavailable under current backend";As per coding guidelines, src/**/*_test.cpp: "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/OgreWidget_test.cpp` around lines 35 - 46, The test currently hard-fails
with ASSERT_NE when render setup is unavailable; change the creation of
EditorViewport and retrieval of the Ogre widget to follow the same skip pattern
as MainWindow: after attempting viewport = new EditorViewport(mainWindow, 7) and
widget = viewport->getOgreWidget(), if either construction throws or returns
nullptr then call GTEST_SKIP() with a clear message (e.g., "Skipping: render
context unavailable") instead of ASSERT_NE; update references in this block
(MainWindow, EditorViewport, getOgreWidget, viewport, widget) so the test
gracefully skips under Xvfb/headless environments.
|
1 participant
Summary\n- expand SelectionSet coverage for resolved-entity and aggregate-selection paths\n- expand MeshImporterExporter coverage for node-only scene export/import and direct exporter/configureCamera paths\n- keep entity-dependent tests CI-safe by following the existing render-window capability checks\n\n## Verification\n- cmake --build build_covpush5 --target UnitTests -j2\n- xvfb-run -a ./build_covpush5/bin/UnitTests --gtest_filter='SelectionSetTests.:MeshImporterExporterTest.:MeshImporterExporterStandaloneTest.:SceneSaveLoadTest.'
Summary by CodeRabbit
Tests
Chores