Increase primitives and mesh exporter coverage - #229
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdded extensive unit and UI tests across mesh exporting, primitives widget, model downloader, SD manager, and material UI; inserted LCOV exclusion markers around modal/native file-dialog code; no public API signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/PrimitivesWidget_test.cpp (2)
1340-1357: Extract the modal-dialog driver into a helper.The
QTimer+QInputDialoglookup block is copied three times. A small helper that takes the text/result would keep this harness in one place and make future adjustments less error-prone.Also applies to: 1375-1392, 1410-1426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PrimitivesWidget_test.cpp` around lines 1340 - 1357, Extract the repeated QTimer::singleShot lambda into a single helper function (e.g., driveModalInputDialogResponse) that accepts the desired text and QDialog::DialogCode result; inside it perform the QInputDialog lookup (first QApplication::activeModalWidget(), then iterate QApplication::topLevelWidgets()), set handled to true, setTextValue(text) and call done(result). Replace the three in-place copies (the blocks currently using QTimer::singleShot with the captured handled variable and empty string/result) with calls to this new helper, ensuring the helper can be invoked where the local handled variable is visible or return a bool/modify handled via reference as needed. Ensure the helper is declared in the test translation unit before use and used for lines around the current lambda copies.
1586-1589: Useclick()instead ofsetChecked()for consistency with earlier tests and to ensure the signal is always emitted.Earlier tests (lines 311, 523) use
click()forpb_switchUV. WhilesetChecked(true)will emittoggled(bool)in this case (since the button starts unchecked after selection), the conditional click pattern is more defensive and aligns with the established test pattern.🔧 Safer interaction in the test
- toggle_uv->setChecked(true); + if (!toggle_uv->isChecked()) + toggle_uv->click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PrimitivesWidget_test.cpp` around lines 1586 - 1589, Replace the direct state change toggle_uv->setChecked(true) with a user-action simulation toggle_uv->click() so the clicked signal handlers run consistently with earlier tests (see pb_switchUV usage); keep edit_radius2->setValue(0.25) and edit_height->setValue(9.5) as-is, but use toggle_uv->click() to ensure the toggled/clicked signals are emitted and follow the established conditional click pattern in the test.
🤖 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/MeshImporterExporter_test.cpp`:
- Around line 849-853: The test currently calls statusMessages.back() without
checking for emptiness; insert an emptiness guard by asserting the container is
not empty (e.g., call ASSERT_FALSE(statusMessages.isEmpty()) or equivalent)
immediately before the EXPECT_EQ(statusMessages.back(), QStringLiteral("Done."))
so the test fails with a clear message when the callback never populated
statusMessages instead of invoking undefined behavior in statusMessages.back().
In `@src/PrimitivesWidget_test.cpp`:
- Around line 1510-1512: The test is calling a non-existent
SelectionSet::select(); replace every additive use of selection->select(...)
with selection->append(...) so the sequence uses selection->selectOne(...) to
set the initial selection and selection->append(...) to add "MixedCubeA" and
"MixedCubeB" (i.e., replace
selection->select(Manager::getSingleton()->getSceneMgr()->getSceneNode("MixedCubeA"))
and similar calls with selection->append(...)); scan the file for other
selection->select(...) occurrences in the multi-selection test cases (they
mirror the pattern around selectOne) and change them to selection->append(...)
to match the API.
---
Nitpick comments:
In `@src/PrimitivesWidget_test.cpp`:
- Around line 1340-1357: Extract the repeated QTimer::singleShot lambda into a
single helper function (e.g., driveModalInputDialogResponse) that accepts the
desired text and QDialog::DialogCode result; inside it perform the QInputDialog
lookup (first QApplication::activeModalWidget(), then iterate
QApplication::topLevelWidgets()), set handled to true, setTextValue(text) and
call done(result). Replace the three in-place copies (the blocks currently using
QTimer::singleShot with the captured handled variable and empty string/result)
with calls to this new helper, ensuring the helper can be invoked where the
local handled variable is visible or return a bool/modify handled via reference
as needed. Ensure the helper is declared in the test translation unit before use
and used for lines around the current lambda copies.
- Around line 1586-1589: Replace the direct state change
toggle_uv->setChecked(true) with a user-action simulation toggle_uv->click() so
the clicked signal handlers run consistently with earlier tests (see pb_switchUV
usage); keep edit_radius2->setValue(0.25) and edit_height->setValue(9.5) as-is,
but use toggle_uv->click() to ensure the toggled/clicked signals are emitted and
follow the established conditional click pattern in the test.
🪄 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: e7f57b52-192f-4263-b127-74f7bda22342
📒 Files selected for processing (2)
src/MeshImporterExporter_test.cppsrc/PrimitivesWidget_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/material_test.cpp (1)
7-9: Consider using a friend class or accessor interface for testing private members.The
#define private publichack enables test access but technically violates the One Definition Rule (ODR), which can cause undefined behavior in edge cases. A safer alternative is to declare the test class as afriendinmaterial.hor expose a test-only accessor interface.That said, this pattern is already established in this codebase (e.g.,
ModelDownloader_test.cpp) and is pragmatic for legacy Qt code where retrofitting friend declarations can be invasive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/material_test.cpp` around lines 7 - 9, The test currently uses the risky '#define private public' hack in material_test.cpp to access Material internals; instead, modify material.h to grant controlled test access by declaring the test class (e.g., MaterialTest or the exact test fixture name used in material_test.cpp) as a friend of class Material, or add a test-only accessor API (e.g., getTestInternalX methods behind a `#ifdef` TESTING flag) and remove the '#define private public' lines in material_test.cpp so tests call the friend or accessor instead; ensure the friend declaration or accessor names match the test fixture and that any test-only macros are documented.src/SDManager_test.cpp (1)
340-353: Assert the restore transition explicitly in this signal test.Line [351] changes the value back but doesn’t verify the resulting signal count, leaving that branch unasserted.
Proposed assertion tweak
TEST_F(SDManagerTest, SetAutoLoadModelEmitsSignalOnlyOnChange) { const bool original = manager->autoLoadModel(); QSignalSpy spy(manager, &SDManager::autoLoadModelChanged); manager->setAutoLoadModel(original); EXPECT_EQ(spy.count(), 0); manager->setAutoLoadModel(!original); EXPECT_EQ(spy.count(), 1); manager->setAutoLoadModel(original); + EXPECT_EQ(spy.count(), 2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SDManager_test.cpp` around lines 340 - 353, The test SDManagerTest::SetAutoLoadModelEmitsSignalOnlyOnChange sets the autoLoadModel back to its original value but doesn't assert the signal emission for that restore; update the test to call manager->setAutoLoadModel(original) and then assert the QSignalSpy spy (watching SDManager::autoLoadModelChanged) has incremented as expected (e.g., EXPECT_EQ(spy.count(), 2) if a second signal is expected) so the restore transition is explicitly verified.
🤖 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/SDManager_test.cpp`:
- Around line 413-455: The test LoadSettingsRestoresAdvancedFieldsAndLastModel
mutates the lastModel setting but doesn't restore it during cleanup; capture the
original last model string (e.g. auto originalLastModel =
manager->lastModelName()) before writing settings and, after the assertions,
restore it along with other fields using the manager API (e.g.
manager->setLastModelName(originalLastModel) or the existing setter used
elsewhere), then call saveSettings(); do the same fix in the other affected test
block (lines ~556-584) to avoid order-dependent state leakage.
---
Nitpick comments:
In `@src/material_test.cpp`:
- Around line 7-9: The test currently uses the risky '#define private public'
hack in material_test.cpp to access Material internals; instead, modify
material.h to grant controlled test access by declaring the test class (e.g.,
MaterialTest or the exact test fixture name used in material_test.cpp) as a
friend of class Material, or add a test-only accessor API (e.g.,
getTestInternalX methods behind a `#ifdef` TESTING flag) and remove the '#define
private public' lines in material_test.cpp so tests call the friend or accessor
instead; ensure the friend declaration or accessor names match the test fixture
and that any test-only macros are documented.
In `@src/SDManager_test.cpp`:
- Around line 340-353: The test
SDManagerTest::SetAutoLoadModelEmitsSignalOnlyOnChange sets the autoLoadModel
back to its original value but doesn't assert the signal emission for that
restore; update the test to call manager->setAutoLoadModel(original) and then
assert the QSignalSpy spy (watching SDManager::autoLoadModelChanged) has
incremented as expected (e.g., EXPECT_EQ(spy.count(), 2) if a second signal is
expected) so the restore transition is explicitly verified.
🪄 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: 45caf817-200d-442a-b8f2-f3aa08eb964a
📒 Files selected for processing (5)
src/MaterialEditorQML.cppsrc/ModelDownloader_test.cppsrc/SDManager_test.cppsrc/material.cppsrc/material_test.cpp
✅ Files skipped from review due to trivial changes (2)
- src/material.cpp
- src/MaterialEditorQML.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/material_test.cpp (1)
70-73: Exercise selection behavior without directly calling the slot.On Line 72, calling
on_listMaterial_itemSelectionChanged()directly bypasses signal/auto-connect wiring, so this test may miss UI-connection regressions.Proposed test tweak
materialWidget.SetMaterialList(QStringList() << "SelectableMaterial"); listWidget->setCurrentRow(0); - materialWidget.on_listMaterial_itemSelectionChanged(); + QCoreApplication::processEvents();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/material_test.cpp` around lines 70 - 73, The test currently calls the slot on_listMaterial_itemSelectionChanged() directly which bypasses the UI signal/slot wiring; instead remove that direct call and trigger the selection through the widget's public selection mechanisms so the signal is emitted and connected slots are exercised — e.g., use listWidget->setCurrentRow(0) or listWidget->setCurrentItem(...) (or QTest::mouseClick/QItemSelectionModel::setCurrentIndex) to select the item, then call QCoreApplication::processEvents() if needed to let the selectionChanged signal propagate and invoke materialWidget's connected slot (referencing materialWidget, listWidget, setCurrentRow, and on_listMaterial_itemSelectionChanged).
🤖 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/material_test.cpp`:
- Around line 70-73: The test currently calls the slot
on_listMaterial_itemSelectionChanged() directly which bypasses the UI
signal/slot wiring; instead remove that direct call and trigger the selection
through the widget's public selection mechanisms so the signal is emitted and
connected slots are exercised — e.g., use listWidget->setCurrentRow(0) or
listWidget->setCurrentItem(...) (or
QTest::mouseClick/QItemSelectionModel::setCurrentIndex) to select the item, then
call QCoreApplication::processEvents() if needed to let the selectionChanged
signal propagate and invoke materialWidget's connected slot (referencing
materialWidget, listWidget, setCurrentRow, and
on_listMaterial_itemSelectionChanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c34db476-2a63-44e3-bece-228bbd063e14
📒 Files selected for processing (5)
src/MeshImporterExporter_test.cppsrc/PrimitivesWidget_test.cppsrc/SDManager_test.cppsrc/material.hsrc/material_test.cpp
✅ Files skipped from review due to trivial changes (2)
- src/MeshImporterExporter_test.cpp
- src/SDManager_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/PrimitivesWidget_test.cpp
fernandotonon
force-pushed
the
codex/coverage-push-8
branch
from
March 28, 2026 05:26
4dd99a0 to
5c29ec3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ModelDownloader_test.cpp (1)
10-12: Consider using Qt's friend-based test access instead of the#define private publichack.This technique can theoretically cause ODR (One Definition Rule) violations if the class layout differs between translation units. For safer alternatives, consider:
- Adding
QTEST_FRIENDor afriend class ModelDownloaderTest;declaration inModelDownloader.h- Creating a test-only accessor class
That said, this pattern is commonly used in Qt test code and typically works fine in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ModelDownloader_test.cpp` around lines 10 - 12, Replace the "#define private public" hack in ModelDownloader_test.cpp with a proper friend-based test access: remove the define/undef lines and instead add a test friend declaration in ModelDownloader.h (e.g., add "friend class ModelDownloaderTest;" or the Qt helper "QTEST_FRIEND" inside the ModelDownloader class) or create a test-only accessor class that exposes the needed internals; update your test fixture class name (ModelDownloaderTest) references accordingly so the test can access private members without changing visibility globally.
🤖 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/SDManager_test.cpp`:
- Around line 453-460: The cleanup currently restores in-memory fields
(manager->setModelsDirectory, manager->setSettings, manager->setAutoLoadModel)
before calling manager->loadSettings(), which re-reads the test-mutated
QSettings and re-applies them; instead, restore QSettings first (use
settings.beginGroup("StableDiffusion"); settings.setValue("lastModel",
originalLastModel); settings.endGroup();) then call manager->loadSettings() once
to pull those restored values into memory, and finally call
manager->saveSettings() if needed; apply the same reorder to the other block
(lines around manager->loadSettings()/saveSettings()) so QSettings are fixed
before loading.
---
Nitpick comments:
In `@src/ModelDownloader_test.cpp`:
- Around line 10-12: Replace the "#define private public" hack in
ModelDownloader_test.cpp with a proper friend-based test access: remove the
define/undef lines and instead add a test friend declaration in
ModelDownloader.h (e.g., add "friend class ModelDownloaderTest;" or the Qt
helper "QTEST_FRIEND" inside the ModelDownloader class) or create a test-only
accessor class that exposes the needed internals; update your test fixture class
name (ModelDownloaderTest) references accordingly so the test can access private
members without changing visibility globally.
🪄 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: 6d2b2639-1c4b-4c6f-92a5-438d9e4927a8
📒 Files selected for processing (8)
src/MaterialEditorQML.cppsrc/MeshImporterExporter_test.cppsrc/ModelDownloader_test.cppsrc/PrimitivesWidget_test.cppsrc/SDManager_test.cppsrc/material.cppsrc/material.hsrc/material_test.cpp
✅ Files skipped from review due to trivial changes (6)
- src/material.cpp
- src/material.h
- src/material_test.cpp
- src/MeshImporterExporter_test.cpp
- src/MaterialEditorQML.cpp
- src/PrimitivesWidget_test.cpp
Comment on lines
+453
to
+460
| manager->setModelsDirectory(originalDir); | ||
| manager->setSettings(originalSettings); | ||
| manager->setAutoLoadModel(originalAutoLoad); | ||
| settings.beginGroup("StableDiffusion"); | ||
| settings.setValue("lastModel", originalLastModel); | ||
| settings.endGroup(); | ||
| manager->loadSettings(); | ||
| manager->saveSettings(); |
There was a problem hiding this comment.
Fix cleanup order: current restore sequence re-applies mutated QSettings values.
In both blocks, you restore in-memory fields and then call loadSettings() (Line [459], Line [593]), which pulls the test-mutated values back from QSettings. That makes cleanup ineffective and can leak state to later tests.
💡 Proposed fix (restore QSettings first, then load once)
@@
- manager->setModelsDirectory(originalDir);
- manager->setSettings(originalSettings);
- manager->setAutoLoadModel(originalAutoLoad);
settings.beginGroup("StableDiffusion");
+ settings.setValue("modelsDirectory", originalDir);
+ settings.setValue("width", originalSettings.width);
+ settings.setValue("height", originalSettings.height);
+ settings.setValue("steps", originalSettings.steps);
+ settings.setValue("cfgScale", originalSettings.cfgScale);
+ settings.setValue("seed", static_cast<qlonglong>(originalSettings.seed));
+ settings.setValue("negativePrompt", originalSettings.negativePrompt);
+ settings.setValue("sampleMethod", originalSettings.sampleMethod);
+ settings.setValue("threads", originalSettings.threads);
+ settings.setValue("gpuLayers", originalSettings.gpuLayers);
+ settings.setValue("autoLoadModel", originalAutoLoad);
settings.setValue("lastModel", originalLastModel);
settings.endGroup();
manager->loadSettings();
- manager->saveSettings();
@@
- manager->setModelsDirectory(originalDir);
- manager->setSettings(originalSettings);
- manager->setAutoLoadModel(originalAutoLoad);
settings.beginGroup("StableDiffusion");
+ settings.setValue("modelsDirectory", originalDir);
+ settings.setValue("width", originalSettings.width);
+ settings.setValue("height", originalSettings.height);
+ settings.setValue("steps", originalSettings.steps);
+ settings.setValue("cfgScale", originalSettings.cfgScale);
+ settings.setValue("seed", static_cast<qlonglong>(originalSettings.seed));
+ settings.setValue("negativePrompt", originalSettings.negativePrompt);
+ settings.setValue("sampleMethod", originalSettings.sampleMethod);
+ settings.setValue("threads", originalSettings.threads);
+ settings.setValue("gpuLayers", originalSettings.gpuLayers);
+ settings.setValue("autoLoadModel", originalAutoLoad);
settings.setValue("lastModel", originalLastModel);
settings.endGroup();
manager->loadSettings();
- manager->saveSettings();Also applies to: 587-594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SDManager_test.cpp` around lines 453 - 460, The cleanup currently
restores in-memory fields (manager->setModelsDirectory, manager->setSettings,
manager->setAutoLoadModel) before calling manager->loadSettings(), which
re-reads the test-mutated QSettings and re-applies them; instead, restore
QSettings first (use settings.beginGroup("StableDiffusion");
settings.setValue("lastModel", originalLastModel); settings.endGroup();) then
call manager->loadSettings() once to pull those restored values into memory, and
finally call manager->saveSettings() if needed; apply the same reorder to the
other block (lines around manager->loadSettings()/saveSettings()) so QSettings
are fixed before loading.
|
This was referenced Mar 30, 2026
4 tasks
5 tasks
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:
Notes:
Summary by CodeRabbit