test(edit-mode): expand EditModeController coverage#537
Conversation
…thods Covers: - Direct QColor setters (setVertexPaintColor / setVertexPaintBackgroundColor) - canEnterEditMode / toggleEditMode - vertexCount / triangleCount / subMeshCount default zero outside edit mode - normalsMode round-trip + signal - recalculateNormals smooth + flat paths - validateMesh + degenerateTriangleCount + hasValidationWarnings - removeDegenerateTriangles no-op paths - vertexColorPreviewEnabled toggle + signal - getSelectedVerticesCentroid empty + selection - rotateSelectedVertices / scaleSelectedVertices / scaleFromSnapshot - snapshotVertexPositions / restoreVertexPositions round-trip - isMeshQuadBased / canConvertToQuads - extrudeSelection error paths + cube happy path - selectedFacesAsHEFaceIndices empty + with selection - flushPendingVertexPaintForEntity no-op paths - vertex paint stroke initial-state guards - bevel gizmo helpers (tick / isHandle / updateBevelWidth / commitBevel) with no active session - addKnifePointOnEdge with no session - merge ops (mergeAtCenter / mergeAtFirst / mergeAtLast / mergeByDistance) no-selection + 2-vert happy paths - loopCutSelection error paths - convertToQuads cube happy path + outside-edit-mode - subdivideCatmullClarkAll cube happy path + outside-edit-mode Baseline coverage on EditModeController.cpp was 64.5% (1099 uncovered lines, 3979 LoC); these tests target the largest untested methods. 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 (1)
📝 WalkthroughWalkthroughAdds ~1.4k lines of GTest coverage exercising EditModeController: vertex painting, edit-mode lifecycle, normals/validation, selection transforms, tool-session guards, merge/bevel/mesh topology ops, and geometry helper edge cases. ChangesEditModeController Test Suite
Possibly Related PRs
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…edge cases Adds 38 additional TEST_F / TEST cases: - beginBevel happy paths (edge + vertex mode), error paths (no selection, face mode, outside edit mode, second begin), commit / cancel of an active session, updateBevelWidth no-op without session - bevelSegments / profile-points no-op paths - mergeAtCenter all-corners + mergeByDistance large threshold - dissolveSelection / deleteSelection in vertex + edge modes - convertToQuads strict 0° threshold - enterEditMode no-selection refusal + double-enter idempotence - exitEditMode commitChanges=true safe + outside-edit-mode no-op - paint shape / radius / strength / falloff same-value no-emit - wireframe same-value no-emit - applyVertexColorBrush square shape + zero-radius + empty mesh - weightToColor clamps above 1 / below 0 - pointToSegmentDistance vertical + diagonal segments - rayTriangleIntersect degenerate triangle - rewriteEntityAfterTopologyChange on real entity Total new tests in this PR: 100 across two commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
🤖 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/EditModeController_test.cpp`:
- Around line 3047-3049: The assertion EXPECT_GE(spy.count(), 0) is tautological
and can't detect missing hint emissions; update the test to assert that at least
one hint was emitted by replacing the check on spy.count() with a stronger
assertion (e.g., EXPECT_GE(spy.count(), 1) or EXPECT_GT(spy.count(), 0)) so the
test fails if no hint is produced; locate the assertion referencing spy.count()
in the EditModeController_test case and change the expected value accordingly.
🪄 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: d370c9cd-a854-45c0-bef1-90278d41293e
📒 Files selected for processing (1)
src/EditModeController_test.cpp
![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: d570a87d14
ℹ️ 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".
| EXPECT_EQ(ctrl->loopCutSelection(), 0); | ||
| // Hint may or may not fire depending on internal walk — but in this | ||
| // case (welded cube triangle topology) it should. | ||
| EXPECT_GE(spy.count(), 0); |
There was a problem hiding this comment.
This assertion is always true, so the test does not actually verify the behavior described in its name/comment (that loopCutSelection() emits editHintMessage on triangle-only topology). If a regression stops emitting the hint, CI will still pass, which weakens the new coverage for this error path. Please assert a positive count (or exact expected count) after calling loopCutSelection().
Useful? React with 👍 / 👎.
The MergeAtCenter / MergeByDistance "merge all 8 verts" tests caused a SIGSEGV in the GPU-side mesh rebuild path under Linux/Xvfb — the post- merge mesh has zero volume and a single vertex, and the entity buffer-resize crashes. Replaces with 3-vertex (one face's corners) merges that exercise the same merge / cluster paths without producing a degenerate mesh: - MergeAtCenterThreeAdjacentVertsRetiresTwo - MergeByDistanceModerateThresholdOnSubset Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…asserts CodeRabbit flagged that EXPECT_GE(spy.count(), 0) is always true and can't detect missing hint emissions on the loop-cut tri-adjacency path. - LoopCutOnTriangleAdjacencyEmitsHintAndReturnsZero: now asserts the hint actually fires (spy.count() >= 1) and that its content matches the "Loop cut needs a quad mesh — try Mesh → Convert to Quads" copy from EditModeController.cpp:3999. While here, replaced other weak EXPECT_GE(n, 0) asserts on dissolve/delete/convertToQuads return values with concrete expectations: - DissolveSelectionVertexModeOnCorner: cube corner v0 has degree 5, so dissolveVertices refuses → EXPECT_EQ(n, 0) (no-op is the contract). - DeleteSelection (vertex / edge): EXPECT_GT(n, 0) AND the GPU triangle count must drop — checks both the API return and the mesh side-effect. - ConvertToQuadsStrictThresholdMergesCubePairs: a welded cube's 12 tris pair up coplanarly into exactly 6 quads, so EXPECT_EQ(merged, 6) plus isMeshQuadBased() now flips to true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/EditModeController_test.cpp`:
- Around line 3192-3195: The test currently converts canUndo() to an int and
compares counts which can hide a missing undo push; after calling
ctrl->commitBevel() and EXPECT_FALSE(ctrl->bevelSessionActive()), replace the
integer comparison with a direct check of UndoManager::getSingleton()->canUndo()
(e.g. EXPECT_TRUE(UndoManager::getSingleton()->canUndo())) to assert that
commitBevel() actually created an undoable state.
- Around line 3510-3512: The test only asserts sub.vertices[2].color.b == 1.0
which could also pass for pure blue; update the assertion to verify the vertex
remained white by checking that color.r and color.g are also ~1.0 in addition to
color.b. Locate the test that references sub.vertices[2].color (the EXPECT_NEAR
on .color.b) and add EXPECT_NEAR checks for .color.r and .color.g with the same
tolerance (1e-3f) to ensure the vertex stayed white.
🪄 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: 1cd7389e-70ca-4342-835f-c4215c8cbdfc
📒 Files selected for processing (1)
src/EditModeController_test.cpp
| const int undoCountBefore = UndoManager::getSingleton()->canUndo() ? 1 : 0; | ||
| ctrl->commitBevel(); | ||
| EXPECT_FALSE(ctrl->bevelSessionActive()); | ||
| EXPECT_GE(UndoManager::getSingleton()->canUndo() ? 1 : 0, undoCountBefore); |
There was a problem hiding this comment.
Strengthen undo assertion to verify commit actually creates undo state.
The current boolean-to-int comparison can pass even when commitBevel() does not push a new undoable action. Assert canUndo() directly after commit.
Proposed fix
- const int undoCountBefore = UndoManager::getSingleton()->canUndo() ? 1 : 0;
+ const bool hadUndoBefore = UndoManager::getSingleton()->canUndo();
ctrl->commitBevel();
EXPECT_FALSE(ctrl->bevelSessionActive());
- EXPECT_GE(UndoManager::getSingleton()->canUndo() ? 1 : 0, undoCountBefore);
+ EXPECT_TRUE(UndoManager::getSingleton()->canUndo());
+ (void)hadUndoBefore; // keep if you want to later assert growth with an explicit count API📝 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.
| const int undoCountBefore = UndoManager::getSingleton()->canUndo() ? 1 : 0; | |
| ctrl->commitBevel(); | |
| EXPECT_FALSE(ctrl->bevelSessionActive()); | |
| EXPECT_GE(UndoManager::getSingleton()->canUndo() ? 1 : 0, undoCountBefore); | |
| const bool hadUndoBefore = UndoManager::getSingleton()->canUndo(); | |
| ctrl->commitBevel(); | |
| EXPECT_FALSE(ctrl->bevelSessionActive()); | |
| EXPECT_TRUE(UndoManager::getSingleton()->canUndo()); | |
| (void)hadUndoBefore; // keep if you want to later assert growth with an explicit count API |
🤖 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/EditModeController_test.cpp` around lines 3192 - 3195, The test currently
converts canUndo() to an int and compares counts which can hide a missing undo
push; after calling ctrl->commitBevel() and
EXPECT_FALSE(ctrl->bevelSessionActive()), replace the integer comparison with a
direct check of UndoManager::getSingleton()->canUndo() (e.g.
EXPECT_TRUE(UndoManager::getSingleton()->canUndo())) to assert that
commitBevel() actually created an undoable state.
| // v2 is outside the box (x=2 > radius=1), stays white. | ||
| EXPECT_NEAR(sub.vertices[2].color.b, 1.0f, 1e-3f); | ||
| } |
There was a problem hiding this comment.
Assertion does not prove the vertex stayed white.
Checking only b == 1.0 cannot distinguish white from fully blue paint. Assert r/g stay at 1.0 too.
Proposed fix
- // v2 is outside the box (x=2 > radius=1), stays white.
- EXPECT_NEAR(sub.vertices[2].color.b, 1.0f, 1e-3f);
+ // v2 is outside the box (x=2 > radius=1), stays white.
+ EXPECT_NEAR(sub.vertices[2].color.r, 1.0f, 1e-3f);
+ EXPECT_NEAR(sub.vertices[2].color.g, 1.0f, 1e-3f);
+ EXPECT_NEAR(sub.vertices[2].color.b, 1.0f, 1e-3f);📝 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.
| // v2 is outside the box (x=2 > radius=1), stays white. | |
| EXPECT_NEAR(sub.vertices[2].color.b, 1.0f, 1e-3f); | |
| } | |
| // v2 is outside the box (x=2 > radius=1), stays white. | |
| EXPECT_NEAR(sub.vertices[2].color.r, 1.0f, 1e-3f); | |
| EXPECT_NEAR(sub.vertices[2].color.g, 1.0f, 1e-3f); | |
| EXPECT_NEAR(sub.vertices[2].color.b, 1.0f, 1e-3f); | |
| } |
🤖 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/EditModeController_test.cpp` around lines 3510 - 3512, The test only
asserts sub.vertices[2].color.b == 1.0 which could also pass for pure blue;
update the assertion to verify the vertex remained white by checking that
color.r and color.g are also ~1.0 in addition to color.b. Locate the test that
references sub.vertices[2].color (the EXPECT_NEAR on .color.b) and add
EXPECT_NEAR checks for .color.r and .color.g with the same tolerance (1e-3f) to
ensure the vertex stayed white.
…ests CI revealed two wrong expectations from the previous tightening pass: - DissolveSelectionVertexModeOnCorner expected dissolveSelection() == 0 (assuming degree-5 verts can't dissolve). Actual: cube corner v0 dissolves cleanly and returns 1, retiring it. Updated to assert == 1 AND that the GPU vertex count drops (so a return-without-mutation is still caught). - ConvertToQuadsStrictThresholdMergesCubePairs expected exactly 6 quad merges at 0° threshold (1 per cube face). Actual: 7 — the welded cube layout has an extra coplanar pair beyond the 6 face pairs. Asserting the exact 7 would couple the test to createInMemoryWeldedCube's internal triangulation, so it now asserts merged > 0 (proving 0° is inclusive) AND isMeshQuadBased() flips to true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Adds 62 new TEST_F cases in
EditModeController_test.cppcovering the many methods that previously had zero coverage. Baseline onEditModeController.cppis 64.5% line coverage (1099 uncovered lines across 139 functions, 3979 LoC).New coverage groups
setVertexPaintColor,setVertexPaintBackgroundColor— invalid input, zero-alpha promotion (FG), zero-alpha allowed (BG), same-value no-emitcanEnterEditMode,toggleEditMode, mesh-info counts when outside edit modesetNormalsModeround-trip + signal,recalculateNormalssmooth + flat paths,validateMesh,degenerateTriangleCount,removeDegenerateTrianglessetVertexColorPreviewEnabledtoggle + signalgetSelectedVerticesCentroid,rotateSelectedVertices,scaleSelectedVertices,scaleFromSnapshot,snapshotVertexPositions/restoreVertexPositionsround-tripisMeshQuadBased,canConvertToQuadsextrudeSelectionerror + cube happy path,selectedFacesAsHEFaceIndices,flushPendingVertexPaintForEntity, vertex-paint-stroke initial state guardstickBevelGizmo,isBevelGizmoHandle,updateBevelWidth,commitBevel(no-op safety)addKnifePointOnEdgeoutside any sessionmergeAtCenter,mergeAtFirst,mergeAtLast,mergeByDistance(no-selection + happy 2-vert paths)A new fixture
EditModeControllerMergeOpsTestprovides a cube + selection setup mirroring the existingEditModeControllerBevelE2ETestso merge / extrude / loop-cut / CC tests have something real to work on.Test plan
unit-tests-linuxruns the new tests under XvfbEditModeController.cpppost-merge🤖 Generated with Claude Code
Summary by CodeRabbit