Skip to content

Strengthen fuzzy_match edge-case contracts in stringutil tests #35077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
May 27, 2026
Merged

Strengthen fuzzy_match edge-case contracts in stringutil tests #35077

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d97065
Initial plan
Copilot May 26, 2026
ea63063
test: add fuzzy match edge-case coverage
Copilot May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions pkg/stringutil/fuzzy_match_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,34 @@ func TestFindClosestMatches(t *testing.T) {
maxResults: 3,
want: nil,
},
{
name: "nil candidates returns nil",
target: "copilot",
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 27, 2026

Choose a reason for hiding this comment

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

[/tdd] This nil candidates case duplicates the contract already proven by "empty candidates returns nothing" — both paths hit the same range no-op.

💡 Suggestion

Either remove this case, or add a clarifying comment that distinguishes why nil is worth a separate assertion (e.g., to guard against panics from a nil check that doesn't exist today but could be added):

// nil is distinct from []string{} in Go; both must be safe to pass.
{
    name:       "nil candidates returns nil (not a panic)",
    ...
},

Without that rationale the reader may wonder why both exist.

candidates: nil,
maxResults: 3,
want: nil,
},
{
name: "maxResults zero returns nil",
target: "copiliot",
candidates: []string{"copilot", "claude"},
maxResults: 0,
want: nil,
},
{
name: "distance four candidate excluded",
target: "abc",
candidates: []string{"abx", "abcdefg"},
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 27, 2026

Choose a reason for hiding this comment

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

[/tdd] The test correctly excludes "abcdefg" but doesn't make the distance self-evident to a reader. Adding an inline comment documenting the exact distance makes this a stronger living specification.

💡 Suggestion 
{
    name:       "distance four candidate excluded",
    target:     "abc",
    // "abcdefg" has distance 4 (> maxDistance of 3), so only "abx" (distance 1) is returned.
    candidates: []string{"abx", "abcdefg"},
    maxResults: 3,
    want:       []string{"abx"},
},

This is especially useful because the maxDistance = 3 constant is const-local to the function body — a reader can't easily cross-reference the cutoff value.

maxResults: 3,
want: []string{"abx"},
},
{
name: "alphabetical tie breaking for equal distances",
target: "zzzz",
candidates: []string{"zzzb", "zzza"},
maxResults: 2,
want: []string{"zzza", "zzzb"},
},
}

for _, tt := range tests {
Expand All @@ -112,6 +140,8 @@ func TestLevenshteinDistance(t *testing.T) {
{name: "push vs pus", a: "push", b: "pus", want: 1},
{name: "contents vs scope typo", a: "contents", b: "cntents", want: 1},
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 27, 2026

Choose a reason for hiding this comment

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

[/tdd] Great documentation of the byte-vs-rune semantic. One follow-up worth considering: FindClosestMatches applies strings.ToLower (rune-aware) before calling LevenshteinDistance (byte-based), creating a mixed-semantics pipeline for non-ASCII input.

💡 Suggested additional test for FindClosestMatches

A companion test in TestFindClosestMatches would make this gap explicit:

{
    // strings.ToLower is rune-aware; LevenshteinDistance is byte-based.
    // A multibyte candidate may appear closer or farther than expected.
    name:       "multibyte candidate utf8 byte distance",
    target:     "e",
    candidates: []string{"é"},  // byte distance 2 — exceeds cutoff, excluded
    maxResults: 3,
    want:       nil,
},

This is not a bug introduced by this PR — it's pre-existing — but the byte-level test only covers LevenshteinDistance in isolation. A FindClosestMatches counterpart makes the end-to-end contract explicit.

{name: "completely different", a: "xyz", b: "abc", want: 3},
// LevenshteinDistance operates on bytes, not runes: "é" is two bytes in UTF-8.
{name: "multibyte utf8 compares bytes", a: "é", b: "e", want: 2},
}

for _, tt := range tests {
Expand Down
Loading