Skip to content

Respect MaxDiff when ignoring slice order - #77

Open
fly1d wants to merge 1 commit into
go-test:mainfrom
fly1d:fix/unordered-maxdiff
Open

fly1d wants to merge 1 commit into
go-test:mainfrom
fly1d:fix/unordered-maxdiff

Conversation

@fly1d

@fly1d fly1d commented Oct 30, 2026

Copy link
Copy Markdown

FLAG_IGNORE_SLICE_ORDER can return more than MaxDiff differences: with
the default limit of 10, two disjoint eight-element slices return 16.

Check the positive budget in the value-count loop so both passes share the
limit, including differences already found in earlier fields. Zero and
negative limits retain their existing unordered behavior. The original
tests are unchanged; 11 appended cases cover the limit and its controls.
The pending Differ work in #71 also retains the uncapped counting loop.

Validation on Go 1.26.6, macOS arm64:

  • The same 11 cases on unchanged upstream: 6 cap failures, 5 controls pass;
    all 11 pass with the fix.
  • go build, full tests with coverage, go vet ./..., and
    go test -race ./... pass. Core package coverage is 100.0%.
  • Tracked Go formatting and git diff --check pass; modules and CI files
    are unchanged.

The existing Go test workflow filters master while this targets main;
these are local results, not a claim that the hosted Go/platform matrix ran.

Comment thread deep_test.go
a: []int{1, 2, 3}, b: []int{4, 5, 6},
},
{
name: "zero retains unordered behavior", limit: 0, wantCount: 6,

@daniel-nichter daniel-nichter Oct 30, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This raises another issue: MaxDiff <= 0 actually produces zero diffs. That's the original because behaviour because, unfortunately, I never specified what happens if it's <= 0. So line 520 should be

if len(c.diff) >= MaxDiff {

like original code, and this test changed to wantCount: 0. That would truly preserve original, undefined behaviour.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants