Skip to content

test(boolean): un-ignore recovered boolean tests#752

Merged
andymai merged 3 commits into
mainfrom
test/unignore-recovered-boolean-tests
Jun 9, 2026
Merged

test(boolean): un-ignore recovered boolean tests#752
andymai merged 3 commits into
mainfrom
test/unignore-recovered-boolean-tests

Conversation

@andymai

@andymai andymai commented Jun 9, 2026

Copy link
Copy Markdown
Owner

A full re-run of every ignored boolean test showed several now pass reliably — their ignore attributes were stale after recent determinism and same-domain fixes (#748 et al).

Changes

  • Un-ignores 9 tests across boolean_stress, boolean_edge_cases, boolean_invariants, coincident_proptest, and the lib boolean suite. Formerly-flaky compound-cut tests were gated on 10/10 passes in fresh processes before un-ignoring.
  • compound_cut_matches_sequential_3x3_grid: corrects its volume oracle (box_vol was 15·15·2=450 against an actual 10×10×2 box, so the < box*0.99 assertions could never fail) and keeps it ignored — with the honest oracle it flakes ~2/3 of runs (compound_cut multi-tool path produces non-manifold, non-deterministic, incorrect results #747).
  • Left ignored: compound_cut_matches_sequential_4x4_grid (borderline under-cut, intermittent) and staircase_fuse_with_cylinders (~125s, CI-time choice).

Verification

fmt, clippy -D warnings, boundary check, full operations lib + 4 integration targets green.

andymai added 2 commits June 9, 2026 12:11
The oracle compared against box_vol = 450 but the target is
make_box(10, 10, 2) with volume 200, so the < box*0.99 threshold
could never fail. With the constant corrected, the test fails in
roughly 2 of 3 fresh-process runs because both the compound and
sequential paths intermittently remove little or no material, so
the ignore returns with an accurate reason until #747 is resolved.
Copilot AI review requested due to automatic review settings June 9, 2026 20:20
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

WASM Binary Size

File main PR Delta
brepkit_wasm.wasm 4.70 MB 4.70 MB 0 KB (0%)

✅ No size change.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refreshes the boolean test suite by re-enabling previously-ignored tests that now pass reliably after recent determinism/same-domain fixes, and by correcting a stale volume oracle in a still-ignored flaky compound-cut test.

Changes:

  • Un-ignored 9 boolean-related tests across stress, edge-case, invariant, coincident-face proptests, and the boolean module’s internal test suite.
  • Corrected compound_cut_matches_sequential_3x3_grid’s box-volume oracle (while keeping the test ignored due to ongoing flakiness tracked in #747).
  • Preserved existing ignores for known-slow or still-flaky tests (e.g., 4×4 grid, staircase benchmark).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/operations/tests/coincident_proptest.rs Re-enables Z-rotation invariance proptest for coincident face-stack fusing.
crates/operations/tests/boolean_stress.rs Re-enables thin-wall cut and asymmetric fuse stress tests.
crates/operations/tests/boolean_invariants.rs Re-enables a cylinder-from-box cut volume invariant test.
crates/operations/tests/boolean_edge_cases.rs Re-enables sequential vertex-drift and alternating fuse/cut edge-case tests.
crates/operations/src/boolean/tests.rs Re-enables several boolean regression/compound-cut tests; corrects the 3×3 grid volume oracle while keeping it ignored.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/operations/src/boolean/tests.rs Outdated
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR un-ignores 9 boolean tests that were previously skipped due to GFA pipeline limitations or flakiness now resolved by earlier determinism fixes (#748), and corrects the volume oracle in the still-ignored 3\u00d73 compound-cut test (was 15\u00d715\u00d72=450, should be 10\u00d710\u00d72=200).

  • 9 tests un-ignored across boolean_stress, boolean_edge_cases, boolean_invariants, coincident_proptest, and the lib boolean suite \u2014 each verified as 10/10 passes in fresh processes before promotion.
  • 3\u00d73 oracle fix: box_vol was wrong by 2.25\u00d7, meaning the < box*0.99 assertion could never have failed even on a completely broken result; corrected and the test remains ignored per compound_cut multi-tool path produces non-manifold, non-deterministic, incorrect results #747.
  • Kept ignored: compound_cut_matches_sequential_4x4_grid (intermittent) and staircase_fuse_with_cylinders (~125 s runtime).

Confidence Score: 5/5

Safe to merge — all changes are test-only, removing stale ignore attributes and fixing an incorrect oracle constant.

No production logic is touched. The oracle fix for the 3×3 grid is correct, and the two nits are documentation/hardening concerns that do not affect correctness.

The 4×4 grid ignore message in crates/operations/src/boolean/tests.rs still carries a stale claim fixed for 3×3 but not updated for 4×4.

Important Files Changed

Filename Overview
crates/operations/src/boolean/tests.rs Corrects the 3×3 grid oracle, un-ignores three tests; the 4×4 ignore message still has a stale 'oracle passes on garbage' claim.
crates/operations/tests/coincident_proptest.rs Un-ignores prop_face_stack_z_rotation_invariant; 16-case proptest budget may not reliably hit the historically bad 3.98 rad angle.
crates/operations/tests/boolean_edge_cases.rs Clean un-ignores of test_sequential_boolean_vertex_drift and test_alternating_union_cut; no logic changes.
crates/operations/tests/boolean_invariants.rs Clean un-ignore of cut_cylinder_from_box_volume (formerly GFA limitation); test logic unchanged.
crates/operations/tests/boolean_stress.rs Clean un-ignores of thin_wall_cut and fuse_asymmetric_boxes; no test logic changes.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/operations/src/boolean/tests.rs:1721-1725
The `#[ignore]` reason on the 4×4 grid still says "the `` `< box*0.99` oracle passes on garbage``," but that bug only existed in the 3×3 grid — the 4×4 oracle was already correct (`20.0 * 20.0 * 2.0 = 800` matches `make_box(20, 20, 2)`). This was fixed for the 3×3 test in this PR; the 4×4 message was not updated and will mislead anyone reading it when evaluating whether to un-ignore the test.

```suggestion
#[ignore = "flaky — multi-tool compound/sequential cuts through the mesh-boolean \
            fallback are non-deterministic across processes (seed-dependent vertex \
            welding) and under-cut faceted re-input; borderline under-cut intermittent. \
            Tracked in #747; revisit under the GFA rewrite."]
fn compound_cut_matches_sequential_4x4_grid() {
```

### Issue 2 of 2
crates/operations/tests/coincident_proptest.rs:57-72
**Proptest case budget may not cover the historically bad angle**

The whole proptest block runs `with_cases(16)`, so `prop_face_stack_z_rotation_invariant` gets 16 draws from `[0, 2π)`. The old ignore described up to ~15% volume drift specifically at angles like 3.98 rad. With 16 uniform samples across a range of 6.28, the probability of landing within ±0.05 of that angle is ≈ 25% per run — meaning ~75% of CI runs will never exercise the historically worst input. A dedicated deterministic regression case at `angle = 3.98_f64` alongside the proptest would give permanent assurance that the specific failure is gone.

Reviews (2): Last reviewed commit: "test(boolean): correct tool description ..." | Re-trigger Greptile

@andymai andymai enabled auto-merge (squash) June 9, 2026 20:38
@andymai andymai merged commit 1496d49 into main Jun 9, 2026
18 checks passed
@andymai andymai deleted the test/unignore-recovered-boolean-tests branch June 9, 2026 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants