Increase test coverage and stabilize GL-heavy suite execution - #302
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request expands test coverage across multiple test suites while refactoring fixture initialization patterns. Several animation and skeleton tests migrate from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
fernandotonon
marked this pull request as ready for review
April 23, 2026 14:36
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
Temporarily closing/reopening to retrigger CI on latest head SHA. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/WelcomeDialog_test.cpp (1)
35-39:⚠️ Potential issue | 🟡 MinorClean up the recent-files setting written by this test.
Line 145 writes
RecentFiles/files, but the fixture only removesWelcomeScreen/dontShowAgain. This leaves a temporary, soon-deleted path in persistentQSettings, which can leak into later tests or local developer settings.🧹 Proposed cleanup
void SetUp() override { ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); // Clear the setting before each test QSettings settings; settings.remove("WelcomeScreen/dontShowAgain"); + settings.remove("RecentFiles/files"); } void TearDown() override { // Clean up the setting after each test QSettings settings; settings.remove("WelcomeScreen/dontShowAgain"); + settings.remove("RecentFiles/files"); }Also applies to: 144-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WelcomeDialog_test.cpp` around lines 35 - 39, TearDown currently only removes "WelcomeScreen/dontShowAgain" but the test also writes "RecentFiles/files", leaving state behind; update the TearDown in the test fixture (the TearDown() override that uses QSettings) to also remove "RecentFiles/files" (call settings.remove("RecentFiles/files")) so both WelcomeScreen/dontShowAgain and RecentFiles/files are cleaned up after each test.src/OgreWidget_test.cpp (1)
81-91:⚠️ Potential issue | 🟡 MinorHandle
EditorViewportcreation failures the same way as MainWindow init.The suite now tolerates transient GL/MainWindow init failures, but
new EditorViewport(...)can still fail the test instead of skipping. Wrap it like the matchingSpaceCameraWidgetIntegrationTestfixture to keep the stabilization behavior consistent.Proposed defensive setup
void SetUp() override { if (!mainWindow) { GTEST_SKIP() << "Failed to initialize MainWindow for OgreWidgetTest"; } - viewport = new EditorViewport(mainWindow, 7); + try { + viewport = new EditorViewport(mainWindow, 7); + } catch (const std::exception& e) { + GTEST_SKIP() << "EditorViewport creation failed: " << e.what(); + } catch (...) { + GTEST_SKIP() << "EditorViewport creation failed with unknown exception"; + } ASSERT_NE(viewport, nullptr); widget = viewport->getOgreWidget();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OgreWidget_test.cpp` around lines 81 - 91, SetUp currently allocates EditorViewport directly and will fail the test on allocation or init errors; change it to detect creation failure and skip the test like the MainWindow block: wrap the EditorViewport construction (EditorViewport and variable viewport) in a defensive check/try-catch, and if construction returns nullptr or throws, call GTEST_SKIP() with a descriptive message; after a successful create, continue to fetch widget via viewport->getOgreWidget() and ASSERT_NE(widget, nullptr) as before so the stabilization behavior matches SpaceCameraWidgetIntegrationTest.src/SpaceCamera_test.cpp (1)
757-765:⚠️ Potential issue | 🟠 MajorDo not kill
SelectionSetwhile the suite-levelMainWindowis still alive.With
mainWindowreused untilTearDownTestSuite(), per-testSelectionSet::kill()can leave long-lived widgets connected to a destroyed singleton. Clear selection instead, and reserve singleton destruction for suite/global teardown.Proposed teardown adjustment
void TearDown() override { - SelectionSet::kill(); + if (SelectionSet::getSingletonPtr()) { + SelectionSet::getSingleton()->clear(); + } delete viewport; viewport = nullptr; widget = nullptr; camera = nullptr;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpaceCamera_test.cpp` around lines 757 - 765, Remove the call to SelectionSet::kill() from the per-test TearDown() and instead clear the current selection using the SelectionSet API (e.g., SelectionSet::clearSelection() or equivalent) so the singleton remains intact while mainWindow is still alive; reserve any destruction of the SelectionSet singleton for suite-level teardown (TearDownTestSuite()) where mainWindow is torn down. Ensure TearDown() still deletes per-test objects (viewport, widget, camera) but does not destroy the SelectionSet singleton.
🧹 Nitpick comments (2)
src/AnimationWidget_test.cpp (1)
53-59: Update the guard/comment now that this fixture uses an in-memory entity.Line 58 no longer loads
.meshmedia, but Lines 53-56 still gate the fixture on mesh-file loading support. If this guard is only about importer/file loading, it will skip the new in-memory coverage unnecessarily; otherwise, consider renaming the helper/comment to reflect that it guards GL/manual Ogre resource creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationWidget_test.cpp` around lines 53 - 59, The test is being skipped by canLoadMeshFiles() even though createAnimatedTestEntity("AnimationWidgetWithMeshEntity") now uses an in-memory entity; remove or narrow that guard so the test doesn't skip unrelated in-memory coverage. Specifically, either remove the canLoadMeshFiles() check around the Animated test or change the predicate to a GL/resource-creation specific check (rename canLoadMeshFiles() to something like canCreateGLResources() or add a new canCreateManualOgreResources() helper) and update the comment to reflect that it only blocks tests that require importer/file loading vs. manual/GL resource creation; keep references to canLoadMeshFiles() and createAnimatedTestEntity to locate the code to change.src/SkeletonDebug_test.cpp (1)
27-33: Avoid skipping the in-memory SkeletonDebug path behind a mesh-file guard.Line 31 no longer imports a mesh file; it creates an in-memory animated entity. If
canLoadMeshFiles()only reflects file-import support, this will skip valid coverage unnecessarily. Prefer relying oncreateAnimatedTestEntity(...) == nullptr, or rename/split the guard if it is actually checking broader GL/manual-resource support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SkeletonDebug_test.cpp` around lines 27 - 33, The test currently skips based on canLoadMeshFiles() which only reflects mesh file import capability but the test uses createAnimatedTestEntity() (an in-memory entity); remove or narrow the canLoadMeshFiles() guard so the test is not skipped erroneously: simply rely on the result of createAnimatedTestEntity("SkeletonDebugTestEntity") and call GTEST_SKIP() only if that returns nullptr; alternatively, if canLoadMeshFiles() is intended to express broader GL/manual-resource support, introduce/rename a capability check (e.g., supportsAnimatedEntities() or supportsManualResources()) and use that instead of canLoadMeshFiles() before skipping.
🤖 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 1984-2030: The tests for count-mode LOD generation (calling
CLIPipeline::cmdLod in tests CmdLod_CountMode and
CmdLod_CountModeWithoutOutputUsesInputStem) currently assert that at least one
of the two expected LOD files (variables lod1/lod2 and
expectedLod1/expectedLod2) exists using a logical OR, which allows a partial
export to pass; change these assertions to require both files exist (e.g.,
replace EXPECT_TRUE(QFile::exists(lod1) || QFile::exists(lod2)) with
EXPECT_TRUE(QFile::exists(lod1) && QFile::exists(lod2)) or ASSERT_TRUE if you
want immediate test failure) and do the same for expectedLod1/expectedLod2 so
the tests fail when only one LOD is produced.
- Around line 780-804: firstAnimationNameForFile currently returns early in
several cases after calling MeshImporterExporter::importer, which skips removing
scene nodes and leaves state behind; ensure the cleanup of Manager-created scene
nodes always runs when Manager::getSingletonPtr() is non-null and after importer
is invoked. Fix by moving the cleanup loop (calling
Manager::getSingleton()->getSceneNodes(),
destroyAllAttachedMovableObjects(node), destroySceneNode(node)) into a
guaranteed cleanup path (e.g., a scope guard/lambda or running the loop just
before every return after the importer call) so that regardless of the
early-return branches (no entities, no skeleton, no animations) the scene nodes
are destroyed before returning from firstAnimationNameForFile.
---
Outside diff comments:
In `@src/OgreWidget_test.cpp`:
- Around line 81-91: SetUp currently allocates EditorViewport directly and will
fail the test on allocation or init errors; change it to detect creation failure
and skip the test like the MainWindow block: wrap the EditorViewport
construction (EditorViewport and variable viewport) in a defensive
check/try-catch, and if construction returns nullptr or throws, call
GTEST_SKIP() with a descriptive message; after a successful create, continue to
fetch widget via viewport->getOgreWidget() and ASSERT_NE(widget, nullptr) as
before so the stabilization behavior matches SpaceCameraWidgetIntegrationTest.
In `@src/SpaceCamera_test.cpp`:
- Around line 757-765: Remove the call to SelectionSet::kill() from the per-test
TearDown() and instead clear the current selection using the SelectionSet API
(e.g., SelectionSet::clearSelection() or equivalent) so the singleton remains
intact while mainWindow is still alive; reserve any destruction of the
SelectionSet singleton for suite-level teardown (TearDownTestSuite()) where
mainWindow is torn down. Ensure TearDown() still deletes per-test objects
(viewport, widget, camera) but does not destroy the SelectionSet singleton.
In `@src/WelcomeDialog_test.cpp`:
- Around line 35-39: TearDown currently only removes
"WelcomeScreen/dontShowAgain" but the test also writes "RecentFiles/files",
leaving state behind; update the TearDown in the test fixture (the TearDown()
override that uses QSettings) to also remove "RecentFiles/files" (call
settings.remove("RecentFiles/files")) so both WelcomeScreen/dontShowAgain and
RecentFiles/files are cleaned up after each test.
---
Nitpick comments:
In `@src/AnimationWidget_test.cpp`:
- Around line 53-59: The test is being skipped by canLoadMeshFiles() even though
createAnimatedTestEntity("AnimationWidgetWithMeshEntity") now uses an in-memory
entity; remove or narrow that guard so the test doesn't skip unrelated in-memory
coverage. Specifically, either remove the canLoadMeshFiles() check around the
Animated test or change the predicate to a GL/resource-creation specific check
(rename canLoadMeshFiles() to something like canCreateGLResources() or add a new
canCreateManualOgreResources() helper) and update the comment to reflect that it
only blocks tests that require importer/file loading vs. manual/GL resource
creation; keep references to canLoadMeshFiles() and createAnimatedTestEntity to
locate the code to change.
In `@src/SkeletonDebug_test.cpp`:
- Around line 27-33: The test currently skips based on canLoadMeshFiles() which
only reflects mesh file import capability but the test uses
createAnimatedTestEntity() (an in-memory entity); remove or narrow the
canLoadMeshFiles() guard so the test is not skipped erroneously: simply rely on
the result of createAnimatedTestEntity("SkeletonDebugTestEntity") and call
GTEST_SKIP() only if that returns nullptr; alternatively, if canLoadMeshFiles()
is intended to express broader GL/manual-resource support, introduce/rename a
capability check (e.g., supportsAnimatedEntities() or supportsManualResources())
and use that instead of canLoadMeshFiles() before skipping.
🪄 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: 6bb5a87d-e2ae-4449-9426-05cd375ab84e
📒 Files selected for processing (7)
src/AnimationWidget_test.cppsrc/CLIPipeline_test.cppsrc/OgreWidget_test.cppsrc/SkeletonDebug_test.cppsrc/SpaceCamera_test.cppsrc/SubEntityHighlight_test.cppsrc/WelcomeDialog_test.cpp
Comment on lines
+780
to
+804
| static QByteArray firstAnimationNameForFile(const QString& filePath) | ||
| { | ||
| if (!Manager::getSingletonPtr()) | ||
| return QByteArray(); | ||
|
|
||
| MeshImporterExporter::importer({filePath}); | ||
| auto& entities = Manager::getSingleton()->getEntities(); | ||
| if (entities.isEmpty() || !entities.first()->hasSkeleton()) | ||
| return QByteArray(); | ||
|
|
||
| Ogre::SkeletonPtr skel = entities.first()->getMesh()->getSkeleton(); | ||
| if (!skel || skel->getNumAnimations() == 0) | ||
| return QByteArray(); | ||
|
|
||
| QByteArray name = QString::fromStdString( | ||
| skel->getAnimation(static_cast<unsigned short>(0))->getName()).toUtf8(); | ||
|
|
||
| auto nodes = Manager::getSingleton()->getSceneNodes(); | ||
| for (auto* node : nodes) { | ||
| Manager::getSingleton()->destroyAllAttachedMovableObjects(node); | ||
| Manager::getSingleton()->destroySceneNode(node); | ||
| } | ||
|
|
||
| return name; | ||
| } |
There was a problem hiding this comment.
Clean up imported scene state on every helper exit.
firstAnimationNameForFile() imports into the shared Manager, but the early returns at Lines 787-792 skip the cleanup loop. If import partially succeeds or returns an entity without animations, later in-process CLI tests can inherit leftover scene nodes.
Proposed cleanup refactor
static QByteArray firstAnimationNameForFile(const QString& filePath)
{
if (!Manager::getSingletonPtr())
return QByteArray();
+ auto cleanupScene = []() {
+ if (!Manager::getSingletonPtr())
+ return;
+ auto nodes = Manager::getSingleton()->getSceneNodes();
+ for (auto* node : nodes) {
+ Manager::getSingleton()->destroyAllAttachedMovableObjects(node);
+ Manager::getSingleton()->destroySceneNode(node);
+ }
+ };
+
MeshImporterExporter::importer({filePath});
auto& entities = Manager::getSingleton()->getEntities();
- if (entities.isEmpty() || !entities.first()->hasSkeleton())
+ if (entities.isEmpty() || !entities.first()->hasSkeleton()) {
+ cleanupScene();
return QByteArray();
+ }
Ogre::SkeletonPtr skel = entities.first()->getMesh()->getSkeleton();
- if (!skel || skel->getNumAnimations() == 0)
+ if (!skel || skel->getNumAnimations() == 0) {
+ cleanupScene();
return QByteArray();
+ }
QByteArray name = QString::fromStdString(
skel->getAnimation(static_cast<unsigned short>(0))->getName()).toUtf8();
- auto nodes = Manager::getSingleton()->getSceneNodes();
- for (auto* node : nodes) {
- Manager::getSingleton()->destroyAllAttachedMovableObjects(node);
- Manager::getSingleton()->destroySceneNode(node);
- }
+ cleanupScene();
return name;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLIPipeline_test.cpp` around lines 780 - 804, firstAnimationNameForFile
currently returns early in several cases after calling
MeshImporterExporter::importer, which skips removing scene nodes and leaves
state behind; ensure the cleanup of Manager-created scene nodes always runs when
Manager::getSingletonPtr() is non-null and after importer is invoked. Fix by
moving the cleanup loop (calling Manager::getSingleton()->getSceneNodes(),
destroyAllAttachedMovableObjects(node), destroySceneNode(node)) into a
guaranteed cleanup path (e.g., a scope guard/lambda or running the loop just
before every return after the importer call) so that regardless of the
early-return branches (no entities, no skeleton, no animations) the scene nodes
are destroyed before returning from firstAnimationNameForFile.
Comment on lines
+1984
to
+2030
| TestArgv args({"qtmesh", "lod", sourceBa.constData(), | ||
| "--count", "2", | ||
| "--reductions", "0.7,0.45", | ||
| "--output", outputBa.constData()}); | ||
| EXPECT_EQ(CLIPipeline::cmdLod(args.argc(), args.argv()), 0); | ||
|
|
||
| const QString lod1 = outDir.filePath("count_out_lod1.mesh"); | ||
| const QString lod2 = outDir.filePath("count_out_lod2.mesh"); | ||
| EXPECT_TRUE(QFile::exists(lod1) || QFile::exists(lod2)); | ||
|
|
||
| QFile::remove(lod1); | ||
| QFile::remove(lod2); | ||
| QFile::remove(outDir.filePath("count_out_lod1.material")); | ||
| QFile::remove(outDir.filePath("count_out_lod2.material")); | ||
| } | ||
|
|
||
| TEST_F(CLIPipelineCmdLodTest, CmdLod_CountModeWithoutOutputUsesInputStem) | ||
| { | ||
| const QString fixtureMesh = testDataDir() + "/robot.mesh"; | ||
| if (!QFile::exists(fixtureMesh)) GTEST_SKIP() << "Test data not found"; | ||
|
|
||
| QTemporaryDir sourceDir; | ||
| ASSERT_TRUE(sourceDir.isValid()); | ||
| const QString sourceFile = sourceDir.filePath("robot.mesh"); | ||
| QFile::remove(sourceFile); | ||
| ASSERT_TRUE(QFile::copy(fixtureMesh, sourceFile)); | ||
|
|
||
| const QString fixtureSkeleton = testDataDir() + "/robot.skeleton"; | ||
| if (QFile::exists(fixtureSkeleton)) { | ||
| const QString sourceSkeleton = sourceDir.filePath("robot.skeleton"); | ||
| QFile::remove(sourceSkeleton); | ||
| ASSERT_TRUE(QFile::copy(fixtureSkeleton, sourceSkeleton)); | ||
| } | ||
|
|
||
| QByteArray sourceBa = sourceFile.toUtf8(); | ||
|
|
||
| const QString expectedLod1 = sourceDir.filePath("robot_lod1.mesh"); | ||
| const QString expectedLod2 = sourceDir.filePath("robot_lod2.mesh"); | ||
| QFile::remove(expectedLod1); | ||
| QFile::remove(expectedLod2); | ||
| QFile::remove(sourceDir.filePath("robot_lod1.material")); | ||
| QFile::remove(sourceDir.filePath("robot_lod2.material")); | ||
|
|
||
| TestArgv args({"qtmesh", "lod", sourceBa.constData(), "--count", "2"}); | ||
| EXPECT_EQ(CLIPipeline::cmdLod(args.argc(), args.argv()), 0); | ||
| EXPECT_TRUE(QFile::exists(expectedLod1) || QFile::exists(expectedLod2)); | ||
|
|
There was a problem hiding this comment.
Assert both requested LOD outputs, not just either one.
Both tests request --count 2, but Lines 1992 and 2029 pass if only one LOD file is produced. That can mask partial count-mode export regressions.
Proposed assertion tightening
- EXPECT_TRUE(QFile::exists(lod1) || QFile::exists(lod2));
+ EXPECT_TRUE(QFile::exists(lod1));
+ EXPECT_TRUE(QFile::exists(lod2));
@@
- EXPECT_TRUE(QFile::exists(expectedLod1) || QFile::exists(expectedLod2));
+ EXPECT_TRUE(QFile::exists(expectedLod1));
+ EXPECT_TRUE(QFile::exists(expectedLod2));📝 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.
Suggested change
| TestArgv args({"qtmesh", "lod", sourceBa.constData(), | |
| "--count", "2", | |
| "--reductions", "0.7,0.45", | |
| "--output", outputBa.constData()}); | |
| EXPECT_EQ(CLIPipeline::cmdLod(args.argc(), args.argv()), 0); | |
| const QString lod1 = outDir.filePath("count_out_lod1.mesh"); | |
| const QString lod2 = outDir.filePath("count_out_lod2.mesh"); | |
| EXPECT_TRUE(QFile::exists(lod1) || QFile::exists(lod2)); | |
| QFile::remove(lod1); | |
| QFile::remove(lod2); | |
| QFile::remove(outDir.filePath("count_out_lod1.material")); | |
| QFile::remove(outDir.filePath("count_out_lod2.material")); | |
| } | |
| TEST_F(CLIPipelineCmdLodTest, CmdLod_CountModeWithoutOutputUsesInputStem) | |
| { | |
| const QString fixtureMesh = testDataDir() + "/robot.mesh"; | |
| if (!QFile::exists(fixtureMesh)) GTEST_SKIP() << "Test data not found"; | |
| QTemporaryDir sourceDir; | |
| ASSERT_TRUE(sourceDir.isValid()); | |
| const QString sourceFile = sourceDir.filePath("robot.mesh"); | |
| QFile::remove(sourceFile); | |
| ASSERT_TRUE(QFile::copy(fixtureMesh, sourceFile)); | |
| const QString fixtureSkeleton = testDataDir() + "/robot.skeleton"; | |
| if (QFile::exists(fixtureSkeleton)) { | |
| const QString sourceSkeleton = sourceDir.filePath("robot.skeleton"); | |
| QFile::remove(sourceSkeleton); | |
| ASSERT_TRUE(QFile::copy(fixtureSkeleton, sourceSkeleton)); | |
| } | |
| QByteArray sourceBa = sourceFile.toUtf8(); | |
| const QString expectedLod1 = sourceDir.filePath("robot_lod1.mesh"); | |
| const QString expectedLod2 = sourceDir.filePath("robot_lod2.mesh"); | |
| QFile::remove(expectedLod1); | |
| QFile::remove(expectedLod2); | |
| QFile::remove(sourceDir.filePath("robot_lod1.material")); | |
| QFile::remove(sourceDir.filePath("robot_lod2.material")); | |
| TestArgv args({"qtmesh", "lod", sourceBa.constData(), "--count", "2"}); | |
| EXPECT_EQ(CLIPipeline::cmdLod(args.argc(), args.argv()), 0); | |
| EXPECT_TRUE(QFile::exists(expectedLod1) || QFile::exists(expectedLod2)); | |
| TestArgv args({"qtmesh", "lod", sourceBa.constData(), | |
| "--count", "2", | |
| "--reductions", "0.7,0.45", | |
| "--output", outputBa.constData()}); | |
| EXPECT_EQ(CLIPipeline::cmdLod(args.argc(), args.argv()), 0); | |
| const QString lod1 = outDir.filePath("count_out_lod1.mesh"); | |
| const QString lod2 = outDir.filePath("count_out_lod2.mesh"); | |
| EXPECT_TRUE(QFile::exists(lod1)); | |
| EXPECT_TRUE(QFile::exists(lod2)); | |
| QFile::remove(lod1); | |
| QFile::remove(lod2); | |
| QFile::remove(outDir.filePath("count_out_lod1.material")); | |
| QFile::remove(outDir.filePath("count_out_lod2.material")); | |
| } | |
| TEST_F(CLIPipelineCmdLodTest, CmdLod_CountModeWithoutOutputUsesInputStem) | |
| { | |
| const QString fixtureMesh = testDataDir() + "/robot.mesh"; | |
| if (!QFile::exists(fixtureMesh)) GTEST_SKIP() << "Test data not found"; | |
| QTemporaryDir sourceDir; | |
| ASSERT_TRUE(sourceDir.isValid()); | |
| const QString sourceFile = sourceDir.filePath("robot.mesh"); | |
| QFile::remove(sourceFile); | |
| ASSERT_TRUE(QFile::copy(fixtureMesh, sourceFile)); | |
| const QString fixtureSkeleton = testDataDir() + "/robot.skeleton"; | |
| if (QFile::exists(fixtureSkeleton)) { | |
| const QString sourceSkeleton = sourceDir.filePath("robot.skeleton"); | |
| QFile::remove(sourceSkeleton); | |
| ASSERT_TRUE(QFile::copy(fixtureSkeleton, sourceSkeleton)); | |
| } | |
| QByteArray sourceBa = sourceFile.toUtf8(); | |
| const QString expectedLod1 = sourceDir.filePath("robot_lod1.mesh"); | |
| const QString expectedLod2 = sourceDir.filePath("robot_lod2.mesh"); | |
| QFile::remove(expectedLod1); | |
| QFile::remove(expectedLod2); | |
| QFile::remove(sourceDir.filePath("robot_lod1.material")); | |
| QFile::remove(sourceDir.filePath("robot_lod2.material")); | |
| TestArgv args({"qtmesh", "lod", sourceBa.constData(), "--count", "2"}); | |
| EXPECT_EQ(CLIPipeline::cmdLod(args.argc(), args.argv()), 0); | |
| EXPECT_TRUE(QFile::exists(expectedLod1)); | |
| EXPECT_TRUE(QFile::exists(expectedLod2)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLIPipeline_test.cpp` around lines 1984 - 2030, The tests for count-mode
LOD generation (calling CLIPipeline::cmdLod in tests CmdLod_CountMode and
CmdLod_CountModeWithoutOutputUsesInputStem) currently assert that at least one
of the two expected LOD files (variables lod1/lod2 and
expectedLod1/expectedLod2) exists using a logical OR, which allows a partial
export to pass; change these assertions to require both files exist (e.g.,
replace EXPECT_TRUE(QFile::exists(lod1) || QFile::exists(lod2)) with
EXPECT_TRUE(QFile::exists(lod1) && QFile::exists(lod2)) or ASSERT_TRUE if you
want immediate test failure) and do the same for expectedLod1/expectedLod2 so
the tests fail when only one LOD is produced.
|
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
AnimationWidgetManualObject tests into single-test suitesAnimationWidget_test.cpp(skeleton/bone-weight toggles, cleanup signal paths, null-row guards)CLIPipeline_test.cpp(full CLI scan override flag matrix)OgreWidget_test.cppandSpaceCamera_test.cpp(suite-level MainWindow reuse + additional input/frame-selection paths)SkeletonDebug_test.cpp(moved from media import dependency to in-memory animated test entity)WelcomeDialog_test.cpp(UI action/persistence/recent-file activation behavior)SubEntityHighlight_test.cppto cover singleton lifecycle and highlight application/removal/fallback behaviorCoverage
Using CI-like gcovr filters locally, total line coverage is now:
19462 / 22647(85%shown in gcovr text summary; ~85.95%raw)Notable file-level improvements:
src/AnimationWidget.cpp:79%(up from earlier baseline around low 60s)src/SpaceCamera.cpp:93%src/SubEntityHighlight.cpp:96%src/WelcomeDialog.cpp:91%Validation
Executed locally with CI-like headless settings (
QT_QPA_PLATFORM=xcb,LIBGL_ALWAYS_SOFTWARE=1,MESA_GL_VERSION_OVERRIDE=3.3):TOTAL=132 PASS=131 FAIL=1 CRASH=0SelectionSetTests; runningSelectionSetTests.*standalone passes (36/36) in ~14sAnimationWidget*,SpaceCamera*,SpaceCameraWidgetIntegrationTest.*,WelcomeDialogTests.*,SubEntityHighlightTests.*, and related updated suitesNotes
.codexandsrc/dependencies/ogre-proceduralwere intentionally left untouched.Summary by CodeRabbit