test: expand coverage with 103 new test cases - #516
Conversation
Add unit-test suites for previously untested pure-data and command-pattern classes, and expand the thin existing coverage on Euler / QtMeshCloudClient. New test files: - ScanConfig_test.cpp — YAML parser, variant-map loading, scope overrides - SubMeshTransform_test.cpp — per-submesh translate/scale/rotate/read/write - commands/ApplyMaterialCommand_test.cpp — redo/undo, multi-target, null - commands/BoneTransformCommand_test.cpp — redo/undo, bind mode, missing entity - MeshDecimatorController_test.cpp — singleton lifecycle, no-selection paths - ViewportTitleBar_test.cpp — buttons, title sync, action propagation - AppConsoleLog_test.cpp — install/attach/detach, stdio capture Expanded suites: - Euler_test.cpp: 5 → 30 cases (constructors, operators, normalize, limit, rotationTo) - QtMeshCloudClient_test.cpp: 5 → 14 cases (validation edge cases, apiBaseUrl env handling, missing-token paths) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds comprehensive GoogleTest coverage across nine modules: Euler rotations, submesh transforms, configuration parsing, viewport UI widgets, mesh decimation control, cloud API validation, console logging, and undo/redo command implementations. Total of ~1,880 lines of test code with Qt and Ogre test fixtures. ChangesComprehensive Test Suite Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
src/QtMeshCloudClient_test.cpp (1)
76-105: 💤 Low valueConsider using a test fixture for environment variable tests.
The current approach of calling
qputenvandqunsetenvwithin each test works but modifies global state without structured cleanup. If a test were to fail unexpectedly, subsequent tests could be affected by the lingering environment variable.A test fixture with
SetUp()andTearDown()methods would provide more robust cleanup:class QtMeshCloudClientApiBaseUrlTest : public ::testing::Test { protected: void SetUp() override { // Save original value originalValue = qgetenv("QTMESH_API_BASE"); hadOriginal = qEnvironmentVariableIsSet("QTMESH_API_BASE"); } void TearDown() override { // Restore original state if (hadOriginal) qputenv("QTMESH_API_BASE", originalValue); else qunsetenv("QTMESH_API_BASE"); } QByteArray originalValue; bool hadOriginal; };That said, the current implementation is acceptable since
EXPECT_EQdoesn't throw exceptions and cleanup will execute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/QtMeshCloudClient_test.cpp` around lines 76 - 105, Wrap the environment-dependent tests in a test fixture to save and restore QTMESH_API_BASE so global state is always restored: create a class QtMeshCloudClientApiBaseUrlTest : public ::testing::Test with protected members QByteArray originalValue and bool hadOriginal, implement SetUp() to capture originalValue = qgetenv("QTMESH_API_BASE") and hadOriginal = qEnvironmentVariableIsSet("QTMESH_API_BASE"), and implement TearDown() to restore with qputenv("QTMESH_API_BASE", originalValue) if hadOriginal or qunsetenv("QTMESH_API_BASE") otherwise; then convert the TEST cases (QtMeshCloudClientApiBaseUrl DefaultUrlWhenEnvUnset, EnvOverride, TrailingSlashesStripped, WhitespaceTrimmed) to TEST_F using this fixture and remove the manual qunsetenv calls from each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/MeshDecimatorController_test.cpp`:
- Around line 25-26: The test currently uses ASSERT_TRUE(tryInitOgre()) and
ASSERT_TRUE(canLoadMeshFiles()) which cause hard failures; change these to
runtime checks that call GTEST_SKIP() with a clear message when prerequisites
are missing (e.g., if (!tryInitOgre()) GTEST_SKIP() << "OGRE not available"; and
similarly for canLoadMeshFiles()), so MeshDecimatorController_test.cpp skips
tests gracefully under headless/Xvfb environments rather than failing the suite.
In `@src/ViewportTitleBar_test.cpp`:
- Around line 97-117: The tests call ViewportTitleBar::closeButton() and
::floatButton() and then click them without asserting the pointers are non-null;
add null assertions before clicking to avoid segfaults: in CloseButtonClosesDock
add ASSERT_NE(nullptr, bar.closeButton()) (or ASSERT_TRUE(bar.closeButton()))
before bar.closeButton()->click(), and in FloatButtonTogglesFloating add
ASSERT_NE(nullptr, bar.floatButton()) before bar.floatButton()->click(); keep
the rest of the test behavior unchanged so failures report clear test assertions
instead of crashes.
---
Nitpick comments:
In `@src/QtMeshCloudClient_test.cpp`:
- Around line 76-105: Wrap the environment-dependent tests in a test fixture to
save and restore QTMESH_API_BASE so global state is always restored: create a
class QtMeshCloudClientApiBaseUrlTest : public ::testing::Test with protected
members QByteArray originalValue and bool hadOriginal, implement SetUp() to
capture originalValue = qgetenv("QTMESH_API_BASE") and hadOriginal =
qEnvironmentVariableIsSet("QTMESH_API_BASE"), and implement TearDown() to
restore with qputenv("QTMESH_API_BASE", originalValue) if hadOriginal or
qunsetenv("QTMESH_API_BASE") otherwise; then convert the TEST cases
(QtMeshCloudClientApiBaseUrl DefaultUrlWhenEnvUnset, EnvOverride,
TrailingSlashesStripped, WhitespaceTrimmed) to TEST_F using this fixture and
remove the manual qunsetenv calls from each 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: adec0690-7573-43c5-9f13-b286d71cd06e
📒 Files selected for processing (9)
src/AppConsoleLog_test.cppsrc/Euler_test.cppsrc/MeshDecimatorController_test.cppsrc/QtMeshCloudClient_test.cppsrc/ScanConfig_test.cppsrc/SubMeshTransform_test.cppsrc/ViewportTitleBar_test.cppsrc/commands/ApplyMaterialCommand_test.cppsrc/commands/BoneTransformCommand_test.cpp
| ASSERT_TRUE(tryInitOgre()); | ||
| ASSERT_TRUE(canLoadMeshFiles()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how Ogre-gated tests handle unavailable runtime prerequisites.
rg -n --type=cpp -C3 'tryInitOgre\(|canLoadMeshFiles\(|GTEST_SKIP\('Repository: fernandotonon/QtMeshEditor
Length of output: 50383
🏁 Script executed:
# First, let's inspect MeshDecimatorController_test.cpp to see the context of lines 25-26
cat -n src/MeshDecimatorController_test.cpp | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 1573
🏁 Script executed:
cat -n src/MeshDecimatorController_test.cpp | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 1573
🏁 Script executed:
rg -n 'GTEST_SKIP' --type=cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
Replace hard failures with graceful skip for unavailable OGRE/mesh prerequisites.
Lines 25-26 currently fail the entire test suite when OGRE initialization or mesh loading is unavailable. Per coding guidelines, tests must work under Xvfb (headless X11) and skip gracefully when optional components are unavailable.
Suggested patch
- ASSERT_TRUE(tryInitOgre());
- ASSERT_TRUE(canLoadMeshFiles());
+ if (!tryInitOgre() || !canLoadMeshFiles()) {
+ GTEST_SKIP() << "Skipping MeshDecimatorController tests: OGRE/mesh prerequisites unavailable.";
+ }📝 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.
| ASSERT_TRUE(tryInitOgre()); | |
| ASSERT_TRUE(canLoadMeshFiles()); | |
| if (!tryInitOgre() || !canLoadMeshFiles()) { | |
| GTEST_SKIP() << "Skipping MeshDecimatorController tests: OGRE/mesh prerequisites unavailable."; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MeshDecimatorController_test.cpp` around lines 25 - 26, The test
currently uses ASSERT_TRUE(tryInitOgre()) and ASSERT_TRUE(canLoadMeshFiles())
which cause hard failures; change these to runtime checks that call GTEST_SKIP()
with a clear message when prerequisites are missing (e.g., if (!tryInitOgre())
GTEST_SKIP() << "OGRE not available"; and similarly for canLoadMeshFiles()), so
MeshDecimatorController_test.cpp skips tests gracefully under headless/Xvfb
environments rather than failing the suite.
There was a problem hiding this comment.
Declining this one — the project convention is ASSERT_TRUE(tryInitOgre()), never silent skip. From TestHelpers.h:182:
Test fixtures should use
ASSERT_TRUE(tryInitOgre())in CI — never skip silently.
Every existing Ogre-dependent suite follows this pattern (e.g. AddKeyframeCommand_test.cpp, TransformOperator_test.cpp, BoneDragRelease_test.cpp). The test_main also gates startup on a working tryInitOgre(), so a fail here only happens if something is wrong with the CI environment, which is what we want to catch.
There was a problem hiding this comment.
@fernandotonon, understood — thank you for the clear explanation. The ASSERT_TRUE(tryInitOgre()) pattern is the right approach here per the project's documented convention, and I'll defer to that going forward.
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 516
File: src/MeshDecimatorController_test.cpp:25-26
Timestamp: 2026-05-14T03:23:16.101Z
Learning: In the QtMeshEditor project (fernandotonon/QtMeshEditor), the convention documented in `TestHelpers.h` line 182 is that Ogre-dependent test fixtures must use `ASSERT_TRUE(tryInitOgre())` and `ASSERT_TRUE(canLoadMeshFiles())` in SetUp — never `GTEST_SKIP()` or silent skipping. A failure here intentionally signals a broken CI environment. Existing suites (e.g. `AddKeyframeCommand_test.cpp`, `TransformOperator_test.cpp`, `BoneDragRelease_test.cpp`) all follow this pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
![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: e87124d23c
ℹ️ 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".
|
|
||
| TEST(QtMeshCloudClientApiBaseUrl, DefaultUrlWhenEnvUnset) | ||
| { | ||
| qunsetenv("QTMESH_API_BASE"); |
There was a problem hiding this comment.
These tests modify the process-wide QTMESH_API_BASE environment variable but never restore its prior value, so any later test in the same UnitTests process can see a different API base than the one it started with. This introduces order-dependent failures when the suite is run in environments where QTMESH_API_BASE is preconfigured (or when another test expects it preserved). Please snapshot and restore the original value (e.g., with a scoped env helper) instead of unsetting globally.
Useful? React with 👍 / 👎.
CI fixes: - ScanConfig_test: PopulatesAllRuleFields was asserting the include list contained only the user's "**/*.fbx", but fromVariantMap auto-injects editor-only globs (tmd/rsd/ply) for non-Assimp formats. Switch to contains() checks so the test passes regardless of injection order. - ViewportTitleBar_test: FloatButtonTogglesFloating used a parentless QDockWidget which is permanently floating — toggling has no observable effect. Now uses a QMainWindow parent and verifies the round trip. Review feedback addressed: - ViewportTitleBar_test: add ASSERT_NE on closeButton()/floatButton() before clicking, so a constructor regression fails the test cleanly instead of segfaulting. - QtMeshCloudClient_test: snapshot QTMESH_API_BASE in SetUp and restore in TearDown so env-mutating tests don't leak state to later tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
_test.cppfiles for previously untested pure-data and command-pattern classes (ScanConfig, SubMeshTransform, ApplyMaterialCommand, BoneTransformCommand, MeshDecimatorController, ViewportTitleBar, AppConsoleLog)Target areas were chosen to maximize coverage gain without dragging in widget UI or live GL paths already excluded by
LCOV_EXCL_*markers. Pure-data classes (ScanConfig YAML/JSON parsing, Euler math, QtMeshCloudClient validation) run anywhere; the Ogre-dependent suites (SubMeshTransform, ApplyMaterialCommand, BoneTransformCommand, MeshDecimatorController) follow the existingtryInitOgre()+canLoadMeshFiles()pattern so they exercise on Linux CI under Xvfb.Test plan
cmake --build build_local --target UnitTests -j4clean (0 errors, 0 new warnings)strings build_local/bin/UnitTests | grep -E "(SubMeshTransform|ViewportTitleBar|...)"confirms new fixtures are registered🤖 Generated with Claude Code
Summary by CodeRabbit