test(coverage): quick wins for CLIPipeline/mainwindow/MaterialEditorQML - #266
Conversation
📝 WalkthroughWalkthroughThis PR extends test coverage across CLI pipelines and Qt UI components while refining CI/coverage configuration. Updates cover exclusion patterns in deployment workflows and Sonar analysis, adds negative-path test cases for CLI commands (cmdFix, cmdAnim, cmdLod), and introduces Qt file-system, AI/SD generation error-handling, and UI event tests with temporary file fixtures. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 0004ed959f
ℹ️ 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".
|
|
||
| editor->generateTextureFromPrompt("brushed steel", 512, 512); | ||
|
|
||
| ASSERT_GE(errorSpy.count(), 1); |
There was a problem hiding this comment.
generateTextureFromPrompt("brushed steel", ...) only emits sdGenerationError synchronously when SD is disabled or no SD model is loaded; when ENABLE_STABLE_DIFFUSION is on and a model has already been auto-loaded, the call takes the generation path and this assertion fails immediately even though nothing is wrong. This makes the test environment-dependent/flaky (similar to the LLM test right above, which already skips when a model is loaded), so the check should be conditioned on sdModelLoaded() or otherwise handle the loaded-model path explicitly.
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: 1
🧹 Nitpick comments (2)
src/mainwindow_test.cpp (1)
148-151: Update the adjacent outdated comment about keyReleaseEvent coverage.This new test now exercises
keyReleaseEventdirectly, so the nearby note saying it is only tested implicitly is stale and should be revised/removed for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow_test.cpp` around lines 148 - 151, Update the outdated comment near the KeyReleaseEvent tests to reflect that keyReleaseEvent is now covered directly by the new test KeyReleaseEventDoesNotCrash; locate the comment adjacent to the TEST_F(MainWindowTest, KeyReleaseEventDoesNotCrash) block and either remove or rewrite it to state that keyReleaseEvent is explicitly exercised by this test (reference keyReleaseEvent and the test name to find the spot).src/CLIPipeline_test.cpp (1)
1036-1045: PreferQDir::tempPath()over hardcoded/tmpin new tests.Using Qt temp-path APIs makes these cases more portable across non-Linux CI/dev environments.
Also applies to: 1656-1662
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1036 - 1045, Replace hardcoded "/tmp" paths in the tests (e.g., the TestArgv usages in TEST(CLIPipelineCmdAnimError, MergeModeWithoutOutputUsesDefaultOutputPath) and the earlier rename test) with Qt's QDir::tempPath() so the tests use a platform-appropriate temporary directory; update the TestArgv constructions and any string concatenation that builds filenames (references: TestArgv, CLIPipeline::cmdAnim, tests named CLIPipelineCmdAnimError and MergeModeWithoutOutputUsesDefaultOutputPath) to call QDir::tempPath() + "/" + "<unique_filename>" (and apply the same change to the other occurrences mentioned around lines 1656-1662).
🤖 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/MaterialEditorQML_test.cpp`:
- Around line 1979-1994: The test
GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel should be made
deterministic by skipping the "SD unavailable" assertions when Stable Diffusion
is present: check editor->stableDiffusionEnabled() at the start of the test and
call GTEST_SKIP() (or otherwise short-circuit the test) with an explanatory
message if true, otherwise keep the existing errorSpy assertions that expect
sdGenerationError after calling editor->generateTextureFromPrompt; this uses the
MaterialEditorQMLTest test, editor->generateTextureFromPrompt,
editor->stableDiffusionEnabled(), and the sdGenerationError signal to locate
where to add the guard.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 1036-1045: Replace hardcoded "/tmp" paths in the tests (e.g., the
TestArgv usages in TEST(CLIPipelineCmdAnimError,
MergeModeWithoutOutputUsesDefaultOutputPath) and the earlier rename test) with
Qt's QDir::tempPath() so the tests use a platform-appropriate temporary
directory; update the TestArgv constructions and any string concatenation that
builds filenames (references: TestArgv, CLIPipeline::cmdAnim, tests named
CLIPipelineCmdAnimError and MergeModeWithoutOutputUsesDefaultOutputPath) to call
QDir::tempPath() + "/" + "<unique_filename>" (and apply the same change to the
other occurrences mentioned around lines 1656-1662).
In `@src/mainwindow_test.cpp`:
- Around line 148-151: Update the outdated comment near the KeyReleaseEvent
tests to reflect that keyReleaseEvent is now covered directly by the new test
KeyReleaseEventDoesNotCrash; locate the comment adjacent to the
TEST_F(MainWindowTest, KeyReleaseEventDoesNotCrash) block and either remove or
rewrite it to state that keyReleaseEvent is explicitly exercised by this test
(reference keyReleaseEvent and the test name to find the spot).
🪄 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: 89276af9-2ee9-4fa6-a0a9-d59de833b04f
📒 Files selected for processing (5)
.github/workflows/deploy.ymlsonar-project.propertiessrc/CLIPipeline_test.cppsrc/MaterialEditorQML_test.cppsrc/mainwindow_test.cpp
| TEST_F(MaterialEditorQMLTest, GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel) { | ||
| QSignalSpy errorSpy(editor.get(), &MaterialEditorQML::sdGenerationError); | ||
|
|
||
| editor->generateTextureFromPrompt("brushed steel", 512, 512); | ||
|
|
||
| ASSERT_GE(errorSpy.count(), 1); | ||
| const QList<QVariant> args = errorSpy.takeFirst(); | ||
| ASSERT_EQ(args.size(), 1); | ||
| const QString message = args.at(0).toString(); | ||
|
|
||
| if (editor->stableDiffusionEnabled()) { | ||
| EXPECT_TRUE(message.contains("No SD model loaded") || message.contains("AI Settings")); | ||
| } else { | ||
| EXPECT_TRUE(message.contains("Stable Diffusion support is not enabled")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the SD unavailable-path assertion deterministic across environments.
This test hard-requires an error signal, but in environments where SD is fully available/configured, generateTextureFromPrompt(...) may not emit sdGenerationError, causing a false failure.
Proposed test hardening
TEST_F(MaterialEditorQMLTest, GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel) {
QSignalSpy errorSpy(editor.get(), &MaterialEditorQML::sdGenerationError);
editor->generateTextureFromPrompt("brushed steel", 512, 512);
- ASSERT_GE(errorSpy.count(), 1);
+ if (errorSpy.count() == 0 && !errorSpy.wait(300)) {
+ GTEST_SKIP() << "Stable Diffusion backend appears available/configured; unavailable-path assertion not applicable";
+ }
+ ASSERT_GE(errorSpy.count(), 1);
const QList<QVariant> args = errorSpy.takeFirst();
ASSERT_EQ(args.size(), 1);
const QString message = args.at(0).toString();
if (editor->stableDiffusionEnabled()) {
EXPECT_TRUE(message.contains("No SD model loaded") || message.contains("AI Settings"));
} else {
EXPECT_TRUE(message.contains("Stable Diffusion support is not enabled"));
}
}As per coding guidelines: "Features depending on optional components (e.g., local LLM / llama.cpp) may not be available in the test environment — guard with #ifdef ENABLE_LOCAL_LLM or skip gracefully".
📝 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.
| TEST_F(MaterialEditorQMLTest, GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel) { | |
| QSignalSpy errorSpy(editor.get(), &MaterialEditorQML::sdGenerationError); | |
| editor->generateTextureFromPrompt("brushed steel", 512, 512); | |
| ASSERT_GE(errorSpy.count(), 1); | |
| const QList<QVariant> args = errorSpy.takeFirst(); | |
| ASSERT_EQ(args.size(), 1); | |
| const QString message = args.at(0).toString(); | |
| if (editor->stableDiffusionEnabled()) { | |
| EXPECT_TRUE(message.contains("No SD model loaded") || message.contains("AI Settings")); | |
| } else { | |
| EXPECT_TRUE(message.contains("Stable Diffusion support is not enabled")); | |
| } | |
| } | |
| TEST_F(MaterialEditorQMLTest, GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel) { | |
| QSignalSpy errorSpy(editor.get(), &MaterialEditorQML::sdGenerationError); | |
| editor->generateTextureFromPrompt("brushed steel", 512, 512); | |
| if (errorSpy.count() == 0 && !errorSpy.wait(300)) { | |
| GTEST_SKIP() << "Stable Diffusion backend appears available/configured; unavailable-path assertion not applicable"; | |
| } | |
| ASSERT_GE(errorSpy.count(), 1); | |
| const QList<QVariant> args = errorSpy.takeFirst(); | |
| ASSERT_EQ(args.size(), 1); | |
| const QString message = args.at(0).toString(); | |
| if (editor->stableDiffusionEnabled()) { | |
| EXPECT_TRUE(message.contains("No SD model loaded") || message.contains("AI Settings")); | |
| } else { | |
| EXPECT_TRUE(message.contains("Stable Diffusion support is not enabled")); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialEditorQML_test.cpp` around lines 1979 - 1994, The test
GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel should be made
deterministic by skipping the "SD unavailable" assertions when Stable Diffusion
is present: check editor->stableDiffusionEnabled() at the start of the test and
call GTEST_SKIP() (or otherwise short-circuit the test) with an explanatory
message if true, otherwise keep the existing errorSpy assertions that expect
sdGenerationError after calling editor->generateTextureFromPrompt; this uses the
MaterialEditorQMLTest test, editor->generateTextureFromPrompt,
editor->stableDiffusionEnabled(), and the sdGenerationError signal to locate
where to add the guard.
|
1 participant
Summary
CLIPipeline_test.cppthat increase non-Ogre branch coverage:cmdFixexisting-invalid-file with--allcmdAnimrename/merge modes without explicit output pathcmdLod--info --jsonnonexistent-file path and invalid--countparsingMaterialEditorQML_test.cpp:getFileSizeString()KB/MB branchesgenerateMaterialFromPrompt,generateTextureFromPrompt, stop methods)mainwindow_test.cppcoverage tests for:keyReleaseEventopenRecentFileAIChatManager.cpp/.hSDManager.handSDWorker.hCoverage config updates
.github/workflows/deploy.yml(gcovrexcludes)sonar-project.properties(sonar.coverage.exclusions)Validation
cmake --build . --target UnitTests -j6QT_QPA_PLATFORM=offscreen ./bin/UnitTests --gtest_filter='CLIPipelineCmdFixError.ExistingInvalidFileWithAllFlagReturnsError:CLIPipelineCmdAnimError.RenameModeWithoutOutputUsesDefaultOutputPath:CLIPipelineCmdAnimError.MergeModeWithoutOutputUsesDefaultOutputPath:CLIPipelineCmdLodError.NonexistentFileWithInfoAndJsonMode:CLIPipelineCmdLodError.InvalidCountValueReportsModeError:MaterialEditorQMLTest.FileSystem_ListDirectoryFiltersOnlyImagesAndDirectories:MaterialEditorQMLTest.FileSystem_GetFileSizeStringCoversKbAndMbBranches:MaterialEditorQMLTest.GenerateMaterialFromPrompt_EmptyPromptEmitsError:MaterialEditorQMLTest.GenerateMaterialFromPrompt_NoModelLoadedEmitsError:MaterialEditorQMLTest.GenerateTextureFromPrompt_EmptyPromptEmitsError:MaterialEditorQMLTest.GenerateTextureFromPrompt_ReportsUnavailableBackendOrModel:MaterialEditorQMLTest.StopGenerationMethodsWithoutActiveJobsDoNotCrash:MainWindowTest.KeyReleaseEventDoesNotCrash:MainWindowTest.OpenRecentFileRemovesMissingPathFromSettings'QT_QPA_PLATFORM=offscreen ./bin/UnitTests --gtest_filter='CLIPipelineCmdFixError.*:CLIPipelineCmdAnimError.*:CLIPipelineCmdLodError.*:MaterialEditorQMLTest.Generate*:*FileSystem_GetFileSizeStringCoversKbAndMbBranches:*FileSystem_ListDirectoryFiltersOnlyImagesAndDirectories:MainWindowTest.KeyReleaseEventDoesNotCrash:MainWindowTest.OpenRecentFileRemovesMissingPathFromSettings'Notes
MainWindow(X11/Ogre parentWindowHandle issue), so the two newMainWindowTestcases are compiled and discovered but skipped locally. They are intended for CI environments whereMainWindowTestcurrently runs.Summary by CodeRabbit
Tests
Chores