test(diff): address review feedback on QuickCheck suite - #4060
Merged
Conversation
Address the three Copilot review comments on #4047: - Clarify the range_text docstring: only non-empty header ranges are 1-based; an empty range is anchored to the line before it, so its rendered anchor may legally be 0 (e.g. `@@ -0,0 +1 @@` for an insertion at the start of a file). - Centralize the QuickCheck knobs as QC_COUNT / QC_MAX_SIZE / QC_LONG_MAX_SIZE constants used by every check call, so the suite's CI cost is tunable in one place. Values are unchanged (200 cases, sizes 100/200); the whole diff package tests in ~0.2-0.3s per target, so no coverage was reduced. - Replace the magic-number `prev` adjacency state in script_ok/lev_ok with a shared PrevEdit enum (ScriptStart/DeleteRun/InsertRun/ EqualRun/ReplaceRun) so the coalescing and Delete-before-Insert invariants are self-documenting. No behavioral change to what the properties assert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up refinement to the diff QuickCheck property-test suite, focusing on clarifying unified-diff header semantics and making the test invariants/configuration easier to maintain without changing what the properties assert.
Changes:
- Centralizes QuickCheck runtime knobs (
count,max_size) into shared constants used by all properties. - Replaces duplicated numeric “previous edit” state tracking with a shared
PrevEditenum for script adjacency/invariant checking. - Clarifies the
range_textdocumentation to explicitly allow0anchors for empty ranges at the start of a file.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Used to enforce the documented adjacency rules: adjacent edits of the | ||
| /// same kind must be coalesced, and a replacement is emitted as `Delete` | ||
| /// before `Insert`, so a delete may never directly follow an insert. | ||
| enum PrevEdit { |
Collaborator
Coverage Report for CI Build 6060Coverage remained the same at 90.666%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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.
Follow-up to #4047 (merged), addressing Copilot's three inline review comments on
diff/quickcheck_test.mbt:range_textdocstring — clarified that only non-empty header ranges are 1-based: an empty range is anchored to the line before it, so its rendered anchor may legally be0(e.g.@@ -0,0 +1 @@for an insertion at the start of a file).@quickcheck.checkcall now uses sharedQC_COUNT/QC_MAX_SIZEconstants (plusQC_LONG_MAX_SIZEfor the deliberately longer two-symbol repetition property), so the suite's cost is tunable in one place. The values are unchanged (200 cases; sizes 100/200) because measurement showed no runtime problem:moon test -p moonbitlang/core/diff(all 127 tests, warm build) runs in ~0.17s on wasm-gc, ~0.28s on js, and ~0.16s on native, both before and after this change — well under any CI concern, so coverage was not reduced.prevstate (0/1/2/3/4) duplicated acrossscript_okandlev_okis replaced by a sharedPrevEditenum (ScriptStart/DeleteRun/InsertRun/EqualRun/ReplaceRun), making the coalescing and Delete-before-adjacent-Insert invariants self-documenting.No change to what the properties assert; all 127 diff tests pass on wasm-gc, js, and native, and
moon info && moon fmtproduce no further changes.🤖 Generated with Claude Code