Improve MeshImporterExporter test coverage to ~95%+ - #189
Merged
Conversation
Add 25 new tests covering previously untested code paths: - All 18 export format URI extensions (data-driven) - All 5 versioned Ogre Mesh exports (v1.0 through v1.10) - Import edge cases: empty list, empty string, camera configuration - Exporter errors: no entity on node, unknown format fallback to suffix - XML import error paths: invalid XML, missing <mesh> root, empty submeshes, missing material, missing faces, empty geometry, shared geometry, missing skeleton file, positions-only (no normals/UVs) - writeTestXMLFile() helper added to TestHelpers.h Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a large suite of MeshImporterExporter tests and a small TestHelpers file-write utility; also adds LCOV coverage exclusion markers and defensive checks in LLM-related files and a new MaterialEditorQML::stopAIGeneration() method. No public API surface removals or breaking changes. Changes
Sequence Diagram(s)(omitted — changes are test additions and coverage/guard annotations; no new multi-component runtime control flow warranting a diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
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: 1
🧹 Nitpick comments (1)
src/TestHelpers.h (1)
417-429: LGTM! Consider checking write result for robustness.The helper function is clean and useful for the XML import tests. For slightly more robust error detection, you could check the return value of
write():Optional improvement
static inline bool writeTestXMLFile(const QString& path, const QByteArray& content) { QFile f(path); if (!f.open(QIODevice::WriteOnly | QIODevice::Truncate)) return false; - f.write(content); + if (f.write(content) != content.size()) + return false; f.close(); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TestHelpers.h` around lines 417 - 429, The writeTestXMLFile helper currently opens QFile f and calls f.write(content) but ignores the return value; update writeTestXMLFile to capture qint64 bytesWritten = f.write(content), ensure bytesWritten equals content.size() (and that bytesWritten != -1), close the file, and return false if the write failed or was partial so the function only returns true on a complete successful write.
🤖 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 1197-1216: The test records camBefore (auto camBefore =
...getPosition()) but never uses it; either remove the unused variable or add an
assertion comparing pre- and post-import positions to ensure the camera moved.
Update the TEST_F MeshImporterExporterTest Importer_ConfiguresCameraAfterImport
to either delete the camBefore declaration or add a check like comparing
camBefore to camAfter (e.g., EXPECT_NE(camBefore, camAfter) or a directional
check) after calling MeshImporterExporter::importer(uri) to validate the camera
actually changed.
---
Nitpick comments:
In `@src/TestHelpers.h`:
- Around line 417-429: The writeTestXMLFile helper currently opens QFile f and
calls f.write(content) but ignores the return value; update writeTestXMLFile to
capture qint64 bytesWritten = f.write(content), ensure bytesWritten equals
content.size() (and that bytesWritten != -1), close the file, and return false
if the write failed or was partial so the function only returns true on a
complete successful write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57b5617c-778a-4ff7-8fa6-cf930e5e6a4c
📒 Files selected for processing (2)
src/MeshImporterExporter_test.cppsrc/TestHelpers.h
![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: 5f6d59050e
ℹ️ 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".
| { | ||
| auto mesh = createInMemoryTriangleMesh("VersionedMesh_" + suffix); | ||
| auto* node = Manager::getSingleton()->addSceneNode(("VersionedNode_" + suffix).c_str()); | ||
| auto* entity = sceneMgr->createEntity("VersionedEntity_" + suffix, mesh); |
There was a problem hiding this comment.
MeshImporterExporter::exporter resolves the entity by sceneNode->getName() and returns -1 when no entity with that name exists. In this helper, the node is named VersionedNode_* but the entity is created as VersionedEntity_*, so the new Exporter_OgreMeshV1_* tests fail before exercising any version-specific export path in environments where mesh-loading tests run.
Useful? React with 👍 / 👎.
- Entity names now match scene node names (use node->getName()) so the exporter's hasEntity() lookup succeeds on CI - Remove unused camBefore variable in camera config test - Check write() return value in writeTestXMLFile helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add LCOV_EXCL_START/STOP markers around code that requires a loaded LLM model, active generation, or downloaded GGUF file — none of which are available in CI. Preserves coverage for all currently tested paths (settings, validation, cleanup, error handling, initial state). Files affected: - LLMWorker.cpp: model load success path, unload, generate loop, initializeContext, cleanupContext, tokenize, detokenize - LLMManager.cpp: worker signal handlers, model load/generate success paths, auto-load - MaterialEditorQML.cpp: AI generation success path, stop generation, network AI callback, all onLLM* signal handlers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/LLMWorker.cpp (1)
365-378:⚠️ Potential issue | 🟠 MajorReturn on token-decode failure instead of falling through to completion.
Line 366 emits
generationError, but the function then reaches Line 377 and emitsgenerationCompletedwith partial output. That gives callers two terminal states for the same generation.🧩 Minimal fix
llama_batch nextBatch = llama_batch_get_one(&newToken, 1); if (llama_decode(m_ctx, nextBatch) != 0) { + llama_sampler_free(sampler); + m_isGenerating.store(false); emit generationError("Failed to decode token"); - break; + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LLMWorker.cpp` around lines 365 - 378, The token-decode failure path currently emits generationError but then continues to the normal completion path, causing both generationError and generationCompleted to be emitted; update the loop that calls llama_decode so that on a non-zero return you clean up (free the sampler via llama_sampler_free and set m_isGenerating.store(false)) and then return/exit immediately instead of falling through; ensure generationCompleted is only emitted on successful completion and keep references to llama_decode, generationError, generationCompleted, llama_sampler_free, m_isGenerating, generatedText and generatedTokens to locate and adjust the logic.src/MaterialEditorQML.cpp (1)
2563-2566:⚠️ Potential issue | 🟠 MajorComplete the stop flow by handling
generationStopped.
stopAIGeneration()only forwards the stop request. This class never subscribes toLLMManager::generationStopped, so a successful stop leaves the progress state stale and QML gets no terminal event.🔌 Suggested follow-up
// constructor connect(llmManager, &LLMManager::generationStarted, this, &MaterialEditorQML::onLLMGenerationStarted); connect(llmManager, &LLMManager::generationProgress, this, &MaterialEditorQML::onLLMGenerationProgress); connect(llmManager, &LLMManager::generationCompleted, this, &MaterialEditorQML::onLLMGenerationCompleted); connect(llmManager, &LLMManager::generationError, this, &MaterialEditorQML::onLLMGenerationError); +connect(llmManager, &LLMManager::generationStopped, this, &MaterialEditorQML::onLLMGenerationStopped); ... +void MaterialEditorQML::onLLMGenerationStopped() +{ + m_llmGenerationProgress = 0.0f; + emit llmGenerationProgressChanged(); + emit aiGenerationError("Generation stopped"); +}Add the matching declaration in the header, or emit a dedicated stopped signal if the QML side already expects one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialEditorQML.cpp` around lines 2563 - 2566, stopAIGeneration currently only calls LLMManager::instance()->stopGeneration() but never handles the completion event, so subscribe to LLMManager::generationStopped and update/clear the progress state when it fires: add a slot or handler in MaterialEditorQML (declare it in the header, e.g. onGenerationStopped or handleGenerationStopped) and connect LLMManager::generationStopped to that slot (or emit a dedicated QML-visible stopped signal from the slot) so QML receives the terminal event and the internal progress state is cleared/reset after stopAIGeneration() is called.src/LLMManager.cpp (1)
307-310:⚠️ Potential issue | 🔴 CriticalAvoid capturing
thisin queued worker lambdas.These functors run later on
m_workerThread. IfLLMManageris destroyed before delivery, the capturedthisbecomes dangling and the lambda dereferences freed state (m_settings,getOgre3DSystemPrompt(),m_worker).🛠️ Safer pattern
+#include <QPointer> ... - QMetaObject::invokeMethod(m_worker, [this, modelPath]() { - m_worker->setSettings(m_settings); - m_worker->loadModel(modelPath); + QPointer<LLMWorker> worker(m_worker); + const LLMSettings settings = m_settings; + QMetaObject::invokeMethod(m_worker, [worker, settings, modelPath]() { + if (!worker) { + return; + } + worker->setSettings(settings); + worker->loadModel(modelPath); }, Qt::QueuedConnection); ... - QMetaObject::invokeMethod(m_worker, [this, userPrompt]() { - m_worker->generate(getOgre3DSystemPrompt(), userPrompt); + QPointer<LLMWorker> worker(m_worker); + const QString systemPrompt = getOgre3DSystemPrompt(); + QMetaObject::invokeMethod(m_worker, [worker, systemPrompt, userPrompt]() { + if (!worker) { + return; + } + worker->generate(systemPrompt, userPrompt); }, Qt::QueuedConnection);Also applies to: 430-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LLMManager.cpp` around lines 307 - 310, The queued lambda currently captures this (and accesses m_settings, getOgre3DSystemPrompt(), and m_worker) which can dangle if LLMManager is destroyed; instead make local copies of all needed data (e.g., copy m_settings, modelPath, and the result of getOgre3DSystemPrompt()) and use a QPointer or raw pointer check to the target worker (e.g., QPointer<LLMWorker> wp = m_worker) captured by value in the lambda, then inside the lambda verify wp is not null before calling wp->setSettings(...) and wp->loadModel(...); apply the same pattern to the other queued lambda(s) that reference this (lines around the second instance invoking m_worker).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/LLMManager.cpp`:
- Around line 307-310: The queued lambda currently captures this (and accesses
m_settings, getOgre3DSystemPrompt(), and m_worker) which can dangle if
LLMManager is destroyed; instead make local copies of all needed data (e.g.,
copy m_settings, modelPath, and the result of getOgre3DSystemPrompt()) and use a
QPointer or raw pointer check to the target worker (e.g., QPointer<LLMWorker> wp
= m_worker) captured by value in the lambda, then inside the lambda verify wp is
not null before calling wp->setSettings(...) and wp->loadModel(...); apply the
same pattern to the other queued lambda(s) that reference this (lines around the
second instance invoking m_worker).
In `@src/LLMWorker.cpp`:
- Around line 365-378: The token-decode failure path currently emits
generationError but then continues to the normal completion path, causing both
generationError and generationCompleted to be emitted; update the loop that
calls llama_decode so that on a non-zero return you clean up (free the sampler
via llama_sampler_free and set m_isGenerating.store(false)) and then return/exit
immediately instead of falling through; ensure generationCompleted is only
emitted on successful completion and keep references to llama_decode,
generationError, generationCompleted, llama_sampler_free, m_isGenerating,
generatedText and generatedTokens to locate and adjust the logic.
In `@src/MaterialEditorQML.cpp`:
- Around line 2563-2566: stopAIGeneration currently only calls
LLMManager::instance()->stopGeneration() but never handles the completion event,
so subscribe to LLMManager::generationStopped and update/clear the progress
state when it fires: add a slot or handler in MaterialEditorQML (declare it in
the header, e.g. onGenerationStopped or handleGenerationStopped) and connect
LLMManager::generationStopped to that slot (or emit a dedicated QML-visible
stopped signal from the slot) so QML receives the terminal event and the
internal progress state is cleared/reset after stopAIGeneration() is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03697d6a-f4cd-4d80-ad6e-88c5109c9917
📒 Files selected for processing (3)
src/LLMManager.cppsrc/LLMWorker.cppsrc/MaterialEditorQML.cpp
|
This was referenced Mar 13, 2026
This was referenced Mar 23, 2026
This was referenced Mar 31, 2026
This was referenced Apr 8, 2026
This was referenced Apr 29, 2026
This was referenced May 9, 2026
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
MeshImporterExporter: versioned Ogre Mesh exports, XML import error paths, format URI edge cases, and exporter error handlingwriteTestXMLFile()helper toTestHelpers.hfor creating test XML filesTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
New Features