[codex] Increase coverage tests and fail CI on skipped tests - #253
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CI skipped-test detection and failing gating to the Linux unit-test runner, and expands unit tests across CLI/pipeline and MeshImporterExporter; also adds a defensive null-check when preloading Ogre meshes to avoid creating entities from failed loads. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 917-925: The current block treats any skipped test as a hard
failure (it calls return 1), which breaks legitimate GTEST_SKIP() use; modify
the logic in the snippet that uses count_skipped_in_xml, skipped_count,
SKIPPED_TESTS, test_name and output_file so skipped tests are not converted into
suite failures: remove the return 1 and instead echo a warning and increment
SKIPPED_TESTS (or, if you need stricter behavior, apply an allowlist check
similar to GL_CRASH_ALLOWLIST before returning non‑zero). Ensure the handler
only fails the job for non‑skipped errors while preserving the existing
SKIPPED_TESTS aggregation.
In `@src/CLIPipeline_test.cpp`:
- Around line 521-529: The test mutates persistent telemetry settings by calling
CLIPipeline::run which triggers SentryReporter::setEnabled(false); fix the test
by saving the original Sentry/enabled value from QSettings before invoking
CLIPipeline::run and restoring it after the EXPECT_EXIT (or explicitly call
SentryReporter::setEnabled(original) in test teardown), ensuring the persistent
QSettings key is returned to its prior state so subsequent tests are not
affected.
- Around line 513-514: The death tests set process-global state via
qputenv("QT_QPA_PLATFORM", ...) and GTEST_FLAG_SET(death_test_style, ...) before
EXPECT_EXIT and never restore them, which breaks later Ogre tests (tryInitOgre
used by CLIPipelineInitTest and CLIPipelineCmdTest); fix by saving the original
QT_QPA_PLATFORM value and the original GTEST_FLAG(death_test_style) before
setting, run the EXPECT_EXIT, and then restore the saved environment variable
and GTest flag immediately after the death test assertion so the parent process
state is unchanged for subsequent tests.
In `@src/MeshImporterExporter_test.cpp`:
- Around line 308-312: The test Importer_MissingMeshFile_IsIgnored uses a lax
assertion EXPECT_LE(Manager::getSingleton()->getSceneNodes().size(), 1) which
can hide a dangling scene node; update the test to assert strictly that no scene
nodes remain after MeshImporterExporter::importer is called with a missing file
by replacing the <=1 check with an exact zero check (use
Manager::getSingleton()->getSceneNodes().size() == 0 or the appropriate
EXPECT_EQ/ASSERT_EQ), keeping the existing call to
MeshImporterExporter::importer and the entities emptiness assertion.
🪄 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: 7fa3b4aa-46c4-4a2d-bf8f-2a4d016b25c9
📒 Files selected for processing (3)
.github/workflows/deploy.ymlsrc/CLIPipeline_test.cppsrc/MeshImporterExporter_test.cpp
| local skipped_count | ||
| skipped_count=$(count_skipped_in_xml "$output_file") | ||
| if [ "$skipped_count" -gt 0 ]; then | ||
| echo "ERROR: $test_name produced $skipped_count skipped test(s)." | ||
| SKIPPED_TESTS=$((SKIPPED_TESTS + skipped_count)) | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List tests that intentionally use GTEST_SKIP() today.
rg -n --glob 'src/**/*_test.cpp' '\bGTEST_SKIP\s*\(' | sed 's/:.*//' | sort | uniq -c | sort -nrRepository: fernandotonon/QtMeshEditor
Length of output: 1528
This turns intentional GTEST_SKIP() paths into hard CI failures.
The repo extensively uses GTEST_SKIP() for graceful degradation when Ogre/GL or other test prerequisites are unavailable — 40+ test files contain over 400 instances across the codebase (MCPServer_test.cpp alone has 92 skip calls). Treating every skipped testcase as a failed suite will make the job red for these valid graceful-degradation paths, not just broken tests. Per the coding guidelines, "Features depending on optional components (e.g., local LLM / llama.cpp) may not be available in the test environment — skip gracefully."
If you want a no-skip policy, scope it to suites that are mandatory in CI or maintain an allowlist similar to GL_CRASH_ALLOWLIST.
Also applies to: 976-983, 1021-1028, 1162-1165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy.yml around lines 917 - 925, The current block
treats any skipped test as a hard failure (it calls return 1), which breaks
legitimate GTEST_SKIP() use; modify the logic in the snippet that uses
count_skipped_in_xml, skipped_count, SKIPPED_TESTS, test_name and output_file so
skipped tests are not converted into suite failures: remove the return 1 and
instead echo a warning and increment SKIPPED_TESTS (or, if you need stricter
behavior, apply an allowlist check similar to GL_CRASH_ALLOWLIST before
returning non‑zero). Ensure the handler only fails the job for non‑skipped
errors while preserving the existing SKIPPED_TESTS aggregation.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/deploy.yml (1)
919-923:⚠️ Potential issue | 🟠 MajorDon’t turn repo-wide
GTEST_SKIP()paths into CI failures.
src/CLIPipeline_test.cppandsrc/MeshImporterExporter_test.cppstill have intentional skips for missing Ogre/GL or test data. Converting any skip intoreturn 1,FAILED_SUITES++, and a finalexit 1will make this job red for expected environment-dependent degradation paths instead of real regressions. KeepSKIPPED_TESTSfor reporting, or scope the hard-fail rule to an explicit allowlist of suites that must never skip.Based on learnings: Features depending on optional components (e.g., local LLM / llama.cpp) may not be available in the test environment — guard with
#ifdefENABLE_LOCAL_LLM or skip gracefully.Also applies to: 977-980, 1162-1165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 919 - 923, The workflow currently treats any skipped GoogleTest suite as a failure by converting skipped_count > 0 into return 1 / FAILED_SUITES++ / exit 1; update the logic in the blocks around the SKIPPED_TESTS handling so that skipped tests are only counted (increment SKIPPED_TESTS and echo a message) and do not cause return 1 or increment FAILED_SUITES, or alternatively implement an explicit allowlist of suites (e.g., "CLIPipeline_test.cpp" and "MeshImporterExporter_test.cpp") that are permitted to skip and only hard-fail skips outside that allowlist; locate the affected shell logic referencing SKIPPED_TESTS, skipped_count, return 1, FAILED_SUITES and adjust to preserve SKIPPED_TESTS reporting while avoiding exit 1 for expected environment-dependent skips.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/deploy.yml:
- Around line 919-923: The workflow currently treats any skipped GoogleTest
suite as a failure by converting skipped_count > 0 into return 1 /
FAILED_SUITES++ / exit 1; update the logic in the blocks around the
SKIPPED_TESTS handling so that skipped tests are only counted (increment
SKIPPED_TESTS and echo a message) and do not cause return 1 or increment
FAILED_SUITES, or alternatively implement an explicit allowlist of suites (e.g.,
"CLIPipeline_test.cpp" and "MeshImporterExporter_test.cpp") that are permitted
to skip and only hard-fail skips outside that allowlist; locate the affected
shell logic referencing SKIPPED_TESTS, skipped_count, return 1, FAILED_SUITES
and adjust to preserve SKIPPED_TESTS reporting while avoiding exit 1 for
expected environment-dependent skips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 154c2226-9d36-44b5-85f7-fde1080bbdb6
📒 Files selected for processing (1)
.github/workflows/deploy.yml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/MeshImporterExporter_test.cpp (1)
309-313:⚠️ Potential issue | 🟡 MinorTighten the missing
.meshassertion to catch leaked scene nodes.Line 312 still allows one leftover node via
EXPECT_LE(..., 1), which can hide partial-import leakage. Make this strict (isEmpty()orEXPECT_EQ(..., 0)).Suggested fix
TEST_F(MeshImporterExporterTest, Importer_MissingMeshFile_IsIgnored) { MeshImporterExporter::importer(QStringList{"/tmp/nonexistent_mesh_importer_12345.mesh"}); EXPECT_TRUE(Manager::getSingleton()->getEntities().isEmpty()); - EXPECT_LE(Manager::getSingleton()->getSceneNodes().size(), 1); + EXPECT_TRUE(Manager::getSingleton()->getSceneNodes().isEmpty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 309 - 313, The test Importer_MissingMeshFile_IsIgnored currently allows one leftover scene node which can mask leaks; update the assertion on Manager::getSingleton()->getSceneNodes() in the TEST_F to require zero nodes (e.g., replace EXPECT_LE(..., 1) with a strict check such as EXPECT_TRUE(...isEmpty()) or EXPECT_EQ(..., 0)) so MeshImporterExporter::importer failing on a missing .mesh file cannot leave any scene nodes behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 309-313: The test Importer_MissingMeshFile_IsIgnored currently
allows one leftover scene node which can mask leaks; update the assertion on
Manager::getSingleton()->getSceneNodes() in the TEST_F to require zero nodes
(e.g., replace EXPECT_LE(..., 1) with a strict check such as
EXPECT_TRUE(...isEmpty()) or EXPECT_EQ(..., 0)) so
MeshImporterExporter::importer failing on a missing .mesh file cannot leave any
scene nodes behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa0bbbc9-15e1-44ca-bee7-747a962c5d27
📒 Files selected for processing (1)
src/MeshImporterExporter_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MeshImporterExporter_test.cpp (1)
309-328: Use per-test unique missing paths to avoid rare/tmpcollisions.These tests are good, but fixed names under
/tmpcan become flaky if a file happens to exist. Prefer generating non-existent paths fromQTemporaryDir+ unique suffix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 309 - 328, Tests use hardcoded /tmp paths that can collide with real files; update each test (Importer_MissingMeshFile_IsIgnored, Importer_MissingMeshXmlFile_IsIgnored, Importer_MissingGenericFileWithAnimOutputs_IsIgnored) to create per-test unique non-existent paths by using QTemporaryDir (or QTemporaryFile::createUniqueFilename) and appending a unique suffix, then pass that generated path to MeshImporterExporter::importer and keep the same expectations; ensure the generated path is removed or guaranteed non-existent before calling importer so the test reliably exercises the "missing file is ignored" behavior.
🤖 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 309-328: Tests use hardcoded /tmp paths that can collide with real
files; update each test (Importer_MissingMeshFile_IsIgnored,
Importer_MissingMeshXmlFile_IsIgnored,
Importer_MissingGenericFileWithAnimOutputs_IsIgnored) to create per-test unique
non-existent paths by using QTemporaryDir (or
QTemporaryFile::createUniqueFilename) and appending a unique suffix, then pass
that generated path to MeshImporterExporter::importer and keep the same
expectations; ensure the generated path is removed or guaranteed non-existent
before calling importer so the test reliably exercises the "missing file is
ignored" behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f87a9726-3125-4518-8d4c-9c0b3c656f73
📒 Files selected for processing (2)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/EditorViewport_test.cpp (1)
24-34: Retry mechanism is sound for test stabilization.The MainWindow initialization retry (lines 24-34) is a solid approach. The
Manager::~Manager()destructor properly cleans up Ogre state with explicitmRoot->shutdown()anddelete mRootcalls, so subsequent retry attempts will have a clean Ogre environment. The bounded retry (3 attempts) withManager::kill()between attempts should effectively handle transient initialization failures in headless test environments.For improved diagnostics, consider logging the exception reason on failed attempts to help identify persistent issues:
Optional: Log exception info on retry
constexpr int kMaxMainWindowInitAttempts = 3; for (int attempt = 1; attempt <= kMaxMainWindowInitAttempts && !mainWindow; ++attempt) { try { mainWindow = new MainWindow(); - } catch (...) { + } catch (const std::exception& e) { + GTEST_LOG_(WARNING) << "MainWindow init attempt " << attempt << " failed: " << e.what(); + mainWindow = nullptr; + Manager::kill(); + QThread::msleep(150); + } catch (...) { + GTEST_LOG_(WARNING) << "MainWindow init attempt " << attempt << " failed with unknown exception"; mainWindow = nullptr; Manager::kill(); QThread::msleep(150); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditorViewport_test.cpp` around lines 24 - 34, The retry loop creating MainWindow swallows exceptions making diagnostics hard; modify the try/catch around new MainWindow() to capture the thrown exception (e.g., std::exception or std::current_exception()) and log its what()/message or reconstituted info before calling Manager::kill() and QThread::msleep(150), keeping the bounded attempts (kMaxMainWindowInitAttempts) and existing cleanup behavior intact so you still assert that mainWindow is non-null afterward.
🤖 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/EditorViewport_test.cpp`:
- Around line 24-34: The retry loop creating MainWindow swallows exceptions
making diagnostics hard; modify the try/catch around new MainWindow() to capture
the thrown exception (e.g., std::exception or std::current_exception()) and log
its what()/message or reconstituted info before calling Manager::kill() and
QThread::msleep(150), keeping the bounded attempts (kMaxMainWindowInitAttempts)
and existing cleanup behavior intact so you still assert that mainWindow is
non-null afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99eceee9-f92c-453e-96e7-0227147903a9
📒 Files selected for processing (1)
src/EditorViewport_test.cpp
|
1 participant
Summary
CLIPipelinetests for unknown command handling and rename-to-existing-animation error flowMeshImporterExportertests forglbshort alias, Ogre XML export/import round-trip, and duplicate node-name handling on scene importdeploy.ymlto fail the workflow when any test is skippedWhy
Validation
UnitTestslocallyQT_QPA_PLATFORM=offscreen build_cov_local/debug/UnitTests --gtest_filter='CLIPipelineRun.*:MeshImporterExporterStandaloneTest.FormatFileURI_GlbShortAlias' --gtest_death_test_style=threadsafeSummary by CodeRabbit
Tests
Bug Fixes
Chores