-
Notifications
You must be signed in to change notification settings - Fork 469
feat: suggest close matches using Levenshtein distance [POC] #836
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.
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
Changes from 5 commits
371923b
5dac03a
788dbc2
e69fd6b
b12cb29
cd54847
8ee261f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import { | ||
calculateLevenshteinDistance, | ||
getCloseMatchesByAttribute, | ||
} from '../close-matches' | ||
import {render} from './helpers/test-utils' | ||
|
||
describe('calculateLevenshteinDistance', () => { | ||
test.each([ | ||
['', '', 0], | ||
['hello', 'hello', 0], | ||
['greeting', 'greeting', 0], | ||
['react testing library', 'react testing library', 0], | ||
['hello', 'hellow', 1], | ||
['greetimg', 'greeting', 1], | ||
['submit', 'sbmit', 1], | ||
['cance', 'cancel', 1], | ||
['doug', 'dog', 1], | ||
['dogs and cats', 'dogs and cat', 1], | ||
['uncool-div', '12cool-div', 2], | ||
['dogs and cats', 'dogs, cats', 4], | ||
['greeting', 'greetings traveler', 10], | ||
['react testing library', '', 21], | ||
['react testing library', 'y', 20], | ||
['react testing library', 'ty', 19], | ||
['react testing library', 'tary', 17], | ||
['react testing library', 'trary', 16], | ||
['react testing library', 'tlibrary', 13], | ||
['react testing library', 'react testing', 8], | ||
['library', 'testing', 7], | ||
['react library', 'react testing', 7], | ||
[ | ||
'The more your tests resemble the way your software is used, the more confidence they can give you.', | ||
'The less your tests resemble the way your software is used, the less confidence they can give you.', | ||
8, | ||
], | ||
])('distance between "%s" and "%s" is %i', (text1, text2, expected) => { | ||
expect(calculateLevenshteinDistance(text1, text2)).toBe(expected) | ||
}) | ||
}) | ||
|
||
describe('getCloseMatchesByAttribute', () => { | ||
test('should return all closest matches', () => { | ||
const {container} = render(` | ||
<div data-testid="The slow brown fox jumps over the lazy dog"></div> | ||
<div data-testid="The rapid brown fox jumps over the lazy dog"></div> | ||
<div data-testid="The quick black fox jumps over the lazy dog"></div> | ||
<div data-testid="The quick brown meerkat jumps over the lazy dog"></div> | ||
<div data-testid="The quick brown fox flies over the lazy dog"></div> | ||
`) | ||
expect( | ||
getCloseMatchesByAttribute( | ||
'data-testid', | ||
container, | ||
'The quick brown fox jumps over the lazy dog', | ||
), | ||
).toEqual([ | ||
'The quick black fox jumps over the lazy dog', | ||
'The quick brown fox flies over the lazy dog', | ||
]) | ||
}) | ||
|
||
test('should ignore matches that are too distant', () => { | ||
const {container} = render(` | ||
<div data-testid="very-cool-div"></div> | ||
<div data-testid="too-diferent-to-match"></div> | ||
<div data-testid="not-even-close"></div> | ||
<div data-testid></div> | ||
<div></div> | ||
`) | ||
expect( | ||
getCloseMatchesByAttribute('data-testid', container, 'normal-div'), | ||
).toEqual([]) | ||
}) | ||
|
||
test('should ignore duplicated matches', () => { | ||
const {container} = render(` | ||
<div data-testid="lazy dog"></div> | ||
<div data-testid="lazy dog"></div> | ||
<div data-testid="lazy dog"></div> | ||
<div data-testid="energetic dog"></div> | ||
`) | ||
expect( | ||
getCloseMatchesByAttribute('data-testid', container, 'happy dog'), | ||
).toEqual(['lazy dog']) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import {makeNormalizer} from './matches' | ||
|
||
const initializeDpTable = (rows, columns) => { | ||
const dp = Array(rows + 1) | ||
.fill() | ||
.map(() => Array(columns + 1).fill()) | ||
|
||
// fill rows | ||
for (let i = 0; i <= rows; i++) { | ||
dp[i][0] = i | ||
} | ||
|
||
// fill columns | ||
for (let i = 0; i <= columns; i++) { | ||
dp[0][i] = i | ||
} | ||
return dp | ||
} | ||
|
||
export const calculateLevenshteinDistance = (text1, text2) => { | ||
const dp = initializeDpTable(text1.length, text2.length) | ||
|
||
for (let row = 1; row < dp.length; row++) { | ||
for (let column = 1; column < dp[row].length; column++) { | ||
if (text1[row - 1] === text2[column - 1]) { | ||
dp[row][column] = dp[row - 1][column - 1] | ||
} else { | ||
dp[row][column] = | ||
Math.min( | ||
dp[row - 1][column - 1], | ||
dp[row][column - 1], | ||
dp[row - 1][column], | ||
) + 1 | ||
} | ||
} | ||
} | ||
return dp[text1.length][text2.length] | ||
} | ||
|
||
const MAX_LEVENSHTEIN_DISTANCE = 4 | ||
|
||
export const getCloseMatchesByAttribute = ( | ||
attribute, | ||
container, | ||
searchText, | ||
{collapseWhitespace, trim, normalizer} = {}, | ||
) => { | ||
const matchNormalizer = makeNormalizer({collapseWhitespace, trim, normalizer}) | ||
const allElements = Array.from(container.querySelectorAll(`[${attribute}]`)) | ||
const allNormalizedValues = new Set( | ||
allElements.map(element => | ||
matchNormalizer(element.getAttribute(attribute) || ''), | ||
), | ||
) | ||
const iterator = allNormalizedValues.values() | ||
const lowerCaseSearch = searchText.toLowerCase() | ||
let lastClosestDistance = MAX_LEVENSHTEIN_DISTANCE | ||
let closestValues = [] | ||
|
||
for (let normalizedText; (normalizedText = iterator.next().value); ) { | ||
if ( | ||
Math.abs(normalizedText.length - searchText.length) > lastClosestDistance | ||
) { | ||
// the distance cannot be closer than what we have already found | ||
// eslint-disable-next-line no-continue | ||
continue | ||
} | ||
|
||
const distance = calculateLevenshteinDistance( | ||
normalizedText.toLowerCase(), | ||
lowerCaseSearch, | ||
) | ||
|
||
if (distance > lastClosestDistance) { | ||
// eslint-disable-next-line no-continue | ||
continue | ||
} | ||
|
||
if (distance < lastClosestDistance) { | ||
lastClosestDistance = distance | ||
closestValues = [] | ||
} | ||
closestValues.push(normalizedText) | ||
} | ||
return closestValues | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import {getCloseMatchesByAttribute} from '../close-matches' | ||
import {checkContainerType} from '../helpers' | ||
import {wrapAllByQueryWithSuggestion} from '../query-helpers' | ||
import {queryAllByAttribute, getConfig, buildQueries} from './all-utils' | ||
|
@@ -11,8 +12,24 @@ function queryAllByTestId(...args) { | |
|
||
const getMultipleError = (c, id) => | ||
`Found multiple elements by: [${getTestIdAttribute()}="${id}"]` | ||
const getMissingError = (c, id) => | ||
`Unable to find an element by: [${getTestIdAttribute()}="${id}"]` | ||
const getMissingError = ( | ||
c, | ||
id, | ||
{computeCloseMatches = false, ...options} = {}, | ||
nickserv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) => { | ||
const defaultMessage = `Unable to find an element by: [${getTestIdAttribute()}="${id}"]` | ||
|
||
const closeMatches = | ||
!computeCloseMatches || typeof id !== 'string' | ||
? [] | ||
: getCloseMatchesByAttribute(getTestIdAttribute(), c, id, options) | ||
Comment on lines
+22
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm concerned about this increasing the performance issues we already have with But perhaps my concern is unwarranted? Maybe this is faster than I think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good point. I tested a few times on my local running Not a reliable benchmark, but when false it finished consistently at around 20ms. When true finished at around 35ms. It increases with the number of elements found, but not by much. Might become slightly quicker with the recommended lib.
An alternative would be to throw functions for That might mean decoupling i.e: a Since that seems a bit involved, we could start with a config There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with giving it a trial run and seeing what real-world experience with it will be like, so defaulting to disabled makes sense to me. Let's try it out using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kentcdodds do you think we should start with the testId query or add this feature to all 10 queries? Wondering if i can break down the work somehow... Re. the performance concern. Maybe if we realise there is a huge performance impact we can skip the computation of close matches on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure. What does everyone else think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO if the default is false, we can give it a try in more queries, though, this seems to me like a configuration that shouldn't be set to true by default at all, especially since it has a performance impact. I see this as something that will not be helpful in CI for example and will only take more time so if the developer wants to opt in they will have an option to do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please try a benchmark again using the new |
||
|
||
return closeMatches.length === 0 | ||
? defaultMessage | ||
: `${defaultMessage}. Did you mean one of the following?\n${closeMatches.join( | ||
'\n', | ||
)}` | ||
} | ||
|
||
const queryAllByTestIdWithSuggestions = wrapAllByQueryWithSuggestion( | ||
queryAllByTestId, | ||
|
Uh oh!
There was an error while loading. Please reload this page.