Repository navigation
Share mesh topology assertions across test suites - #149
Open
officialasishkumar wants to merge 1 commit into
Open
officialasishkumar wants to merge 1 commit into
officialasishkumar wants to merge 1 commit into
Conversation
Four suites carried their own copies of the same slicer-safety checks:
- assertClosedIndexedSurface (meshingOptions.test.ts) and
assertClosedVoxelSurface (voxelMesh.test.ts) were the same closed
index-topology check (two oppositely wound faces per edge, one
triangle fan per vertex, no degenerate or duplicate triangles). They
differed only in message labels and the final volume assertion.
- assertHealthyMesh was duplicated in meshing.test.ts and
flatPaint.test.ts; the meshing copy also checked cap winding.
- reportForMessage was identical in meshing.test.ts and
export3mf.test.ts, and flatPaint.test.ts had a shorter copy.
Move them into tests/helpers/meshAssertions.ts and import them from
each suite. Every assertion is kept:
- assertClosedIndexedSurface(mesh, { label, volume }) prefixes its
messages with the label and checks the exact enclosed volume when
one is given (the voxel cases), or a positive volume otherwise.
- assertHealthyMesh(label, mesh, { capWinding }) checks cap winding
only when asked, which every meshing.test.ts call does and the Flat
Paint calls do not, as before. Flat Paint failures now print the full
integrity report.
The other mesh checks in export3mf.test.ts, paletteProofGeometry.test.ts
and stackMatrixCalibration.test.ts stay where they are: they use a
different edge tolerance, read three.js geometry, or check per-cuboid
winding of parsed 3MF vertices, so they are not the same checks.
Fixes vycdev#129
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch has not been deployed
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
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.
Several test files carried their own copies of the same slicer-safety checks (#129). This moves them into
tests/helpers/meshAssertions.tsand imports them from each suite. Every existing assertion is kept, so coverage doesn't weaken.Shared helpers
assertClosedIndexedSurface(mesh, { label, volume })replacesassertClosedIndexedSurfaceinmeshingOptions.test.tsandassertClosedVoxelSurfaceinvoxelMesh.test.ts. Both checked that each edge has two faces with opposite winding, that each vertex has a single triangle fan, and that there are no degenerate or duplicate triangles. The label prefixes every message. Withvolume, the enclosed volume must match it, as the voxel cases required. Without it, the volume must be positive, as the meshing-options cases required.assertHealthyMesh(label, mesh, { capWinding })replaces the copies inmeshing.test.tsandflatPaint.test.ts. Cap winding is checked only when asked. Everymeshing.test.tscall asks for it and the Flat Paint calls don't, as before.reportForMessageandinspectCapWindingmove with them.reportForMessagewas identical inmeshing.test.tsandexport3mf.test.ts.flatPaint.test.tshad a shorter copy, so Flat Paint failures now print the full integrity report.Left as they are
As the issue suggests, I reviewed the related helpers and merged only the ones that really match:
assertStlLayerIsManifoldandassertExportLayerHasOutwardNormalsinexport3mf.test.tsuse a looser edge tolerance and assert each count separately. They now use the sharedreportForMessage.assertClosedPositiveMeshinpaletteProofGeometry.test.tsreads three.js geometry and checks edge counts only.assertClosedOutwardMatrixMeshesinstackMatrixCalibration.test.tsworks on parsed 3MF coordinates and checks winding per cuboid.No changelog entry, because this is a test-only refactor that doesn't change behavior or coverage.
Fixes #129
Validation:
npm test: 1112 passed, 1 skipped, 0 failed.node --testonmeshing,flatPaint,voxelMesh,meshingOptions, andexport3mf: 192 passed.npx tsc -p tsconfig.test.json --noEmit,npx eslint tests --max-warnings=0, and Prettier on the new helper andmeshing.test.ts: passed.