-
Notifications
You must be signed in to change notification settings - Fork 274
feat(breaking): add text match options a.k.a string precision API #554
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 17 commits
34ffda8
d95ff2f
b616319
3857870
7c4c074
f9e7d8d
e3719bd
73c6aed
0884d86
c14446c
677a78e
cb6d122
b7f0b74
5f1d266
e5e977b
4118a23
4087aff
6d8caf4
5a8c62b
901d3e8
e1ad2b4
ff10a3a
6eb071a
3bafdff
d2f9f33
af62597
5aa5e50
864155a
ed34a69
223247b
bd17c86
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 |
---|---|---|
@@ -1,7 +1,7 @@ | ||
// @flow | ||
import React from 'react'; | ||
import { View, Text, TextInput, Button } from 'react-native'; | ||
import { render } from '..'; | ||
import { render, getDefaultNormalizer } from '..'; | ||
|
||
const MyComponent = () => { | ||
return <Text>My Component</Text>; | ||
|
@@ -50,3 +50,114 @@ test('supports a regex matcher', () => { | |
expect(getByTestId(/view/)).toBeTruthy(); | ||
expect(getAllByTestId(/text/)).toHaveLength(2); | ||
}); | ||
|
||
describe('Supports a TextMatch options', () => { | ||
test('getByText, getAllByText', () => { | ||
const { getByText, getAllByText } = render( | ||
<View> | ||
<Text testID="text">Text and details</Text> | ||
<Button | ||
testID="button" | ||
title="Button and a detail" | ||
onPress={jest.fn()} | ||
/> | ||
</View> | ||
); | ||
|
||
expect(getByText('details', { exact: false })).toBeTruthy(); | ||
expect(getAllByText('detail', { exact: false })).toHaveLength(2); | ||
}); | ||
|
||
test('getByPlaceholderText, getAllByPlaceholderText', () => { | ||
const { getByPlaceholderText, getAllByPlaceholderText } = render( | ||
<View> | ||
<TextInput placeholder={'Placeholder with details'} /> | ||
<TextInput placeholder={'Placeholder with a DETAIL'} /> | ||
</View> | ||
); | ||
|
||
expect(getByPlaceholderText('details', { exact: false })).toBeTruthy(); | ||
expect(getAllByPlaceholderText('detail', { exact: false })).toHaveLength(2); | ||
}); | ||
|
||
test('getByDisplayValue, getAllByDisplayValue', () => { | ||
const { getByDisplayValue, getAllByDisplayValue } = render( | ||
<View> | ||
<TextInput value={'Value with details'} /> | ||
<TextInput value={'Value with a detail'} /> | ||
</View> | ||
); | ||
|
||
expect(getByDisplayValue('details', { exact: false })).toBeTruthy(); | ||
expect(getAllByDisplayValue('detail', { exact: false })).toHaveLength(2); | ||
}); | ||
|
||
test('with TextMatch option exact === false text search is NOT case sensitive', () => { | ||
const { getByText, getAllByText } = render( | ||
<View> | ||
<Text testID="text">Text and details</Text> | ||
<Button | ||
testID="button" | ||
title="Button and a DeTAil" | ||
onPress={jest.fn()} | ||
/> | ||
</View> | ||
); | ||
|
||
expect(getByText('DeTaIlS', { exact: false })).toBeTruthy(); | ||
expect(getAllByText('detail', { exact: false })).toHaveLength(2); | ||
}); | ||
}); | ||
|
||
describe('Supports normalization', () => { | ||
test('trims and collapses whitespace by default', () => { | ||
const { getByText } = render( | ||
<View> | ||
<Text>{` Text and | ||
|
||
|
||
whitespace`}</Text> | ||
</View> | ||
); | ||
|
||
expect(getByText('Text and whitespace')).toBeTruthy(); | ||
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 like that one 👍 |
||
}); | ||
|
||
test('trim and collapseWhitespace is customizable by getDefaultNormalizer param', () => { | ||
const testTextWithWhitespace = ` Text and | ||
|
||
|
||
whitespace`; | ||
const { getByText } = render( | ||
<View> | ||
<Text>{testTextWithWhitespace}</Text> | ||
</View> | ||
); | ||
|
||
expect( | ||
getByText(testTextWithWhitespace, { | ||
normalizer: getDefaultNormalizer({ | ||
trim: false, | ||
collapseWhitespace: false, | ||
}), | ||
}) | ||
).toBeTruthy(); | ||
}); | ||
|
||
test('normalizer function is customisable', () => { | ||
const testText = 'A TO REMOVE text'; | ||
const normalizerFn = (textToNormalize: string) => | ||
textToNormalize.replace('TO REMOVE ', ''); | ||
const { getByText } = render( | ||
<View> | ||
<Text>{testText}</Text> | ||
</View> | ||
); | ||
|
||
expect( | ||
getByText('A text', { | ||
normalizer: normalizerFn, | ||
}) | ||
).toBeTruthy(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ test('queryByText not found', () => { | |
).toBeFalsy(); | ||
}); | ||
|
||
test('queryByText nested text across multiple <Text> in <Text>', () => { | ||
test('queryByText does not match nested text across multiple <Text> in <Text>', () => { | ||
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. So we're regressing this functionality to be compatible with Testing Library, which also doesn't support this? 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. Yes. I checked, testing library doesn't match in this case. 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. How does this PR change this functionality? Finding the text in nodes still recurses through children 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. 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 must be missing something but how does the logic added to 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. Yes, the change is to keep matching component with multiple children, but not 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. Ah, so if the test render looked like this: <Text>
<Text>Hello</Text>
World!
</Text> Would this then match the outer text? 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. This currently match is the current released version, but wouldn't match in my current implementation. It's the topic of discussion in #554 (comment) |
||
const { queryByText } = render( | ||
<Text nativeID="1"> | ||
Hello{' '} | ||
|
@@ -59,7 +59,7 @@ test('queryByText nested text across multiple <Text> in <Text>', () => { | |
</Text> | ||
); | ||
|
||
expect(queryByText('Hello World!')?.props.nativeID).toBe('1'); | ||
expect(queryByText('Hello World!')).toBe(null); | ||
}); | ||
|
||
test('queryByText with nested Text components return the closest Text', () => { | ||
|
@@ -72,6 +72,7 @@ test('queryByText with nested Text components return the closest Text', () => { | |
const { queryByText } = render(<NestedTexts />); | ||
|
||
expect(queryByText('My text')?.props.nativeID).toBe('2'); | ||
expect(queryByText('My text', { exact: false })?.props.nativeID).toBe('2'); | ||
thymikee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
|
||
test('queryByText with nested Text components each with text return the lowest one', () => { | ||
|
@@ -87,18 +88,26 @@ test('queryByText with nested Text components each with text return the lowest o | |
expect(queryByText('My text')?.props.nativeID).toBe('2'); | ||
}); | ||
|
||
test('queryByText nested <CustomText> in <Text>', () => { | ||
const CustomText = ({ children }) => { | ||
return <Text>{children}</Text>; | ||
}; | ||
test('queryByText with nested Text components: not-exact text match returns the most deeply nested common component', () => { | ||
const { queryByText: queryByTextFirstCase } = render( | ||
<Text nativeID="1"> | ||
bob | ||
<Text nativeID="2">My </Text> | ||
<Text nativeID="3">text</Text> | ||
</Text> | ||
); | ||
|
||
const { queryByText: queryByTextSecondCase } = render( | ||
<Text nativeID="1"> | ||
bob | ||
<Text nativeID="2">My text for test</Text> | ||
</Text> | ||
); | ||
|
||
expect(queryByTextFirstCase('My text')).toBe(null); | ||
expect( | ||
render( | ||
<Text> | ||
Hello <CustomText>World!</CustomText> | ||
</Text> | ||
).queryByText('Hello World!') | ||
).toBeTruthy(); | ||
queryByTextSecondCase('My text', { exact: false })?.props.nativeID | ||
).toBe('2'); | ||
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. Does seem strange a bit, that we use one component structure with the default Looks like we should do one of the following (in order of preference):
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 do overall believe that tests should be restructured, a lot of cases are tested only because the internal APIs are the same (a lot of edge cases for But this specific case makes sense, I'll follow your suggestion, I got a bit lazy and monkey patched some tests without really reconsidering them. 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. Those could contain additional |
||
}); | ||
|
||
test('queryByText nested deep <CustomText> in <Text>', () => { | ||
|
@@ -112,5 +121,28 @@ test('queryByText nested deep <CustomText> in <Text>', () => { | |
<CustomText>Hello</CustomText> <CustomText>World!</CustomText> | ||
</Text> | ||
).queryByText('Hello World!') | ||
).toBeTruthy(); | ||
).toBe(null); | ||
}); | ||
|
||
test('queryAllByText does not match several times the same text', () => { | ||
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. 👍 |
||
const allMatched = render( | ||
<Text nativeID="1"> | ||
Start | ||
<Text nativeID="2">This is a long text</Text> | ||
</Text> | ||
).queryAllByText('long text', { exact: false }); | ||
expect(allMatched.length).toBe(1); | ||
expect(allMatched[0].props.nativeID).toBe('2'); | ||
}); | ||
|
||
test('queryAllByText matches all the matching nodes', () => { | ||
const allMatched = render( | ||
<Text nativeID="1"> | ||
Start | ||
<Text nativeID="2">This is a long text</Text> | ||
<Text nativeID="3">This is another long text</Text> | ||
</Text> | ||
).queryAllByText('long text', { exact: false }); | ||
expect(allMatched.length).toBe(2); | ||
expect(allMatched.map((node) => node.props.nativeID)).toEqual(['2', '3']); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the second breaking change I mentioned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could provide an easy migration path by looking for
waitFor
options (timeout
,interval
) also in thequeryOptions
only forfindBy
queries and only till next major release. While displaying a console warning to inform the developer. Wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea that I missed, I fixed it in the latest commit d2f9f33