-
Notifications
You must be signed in to change notification settings - Fork 274
fix(breaking): correct faulty implementation of finding by name #62
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 all commits
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 |
---|---|---|
|
@@ -57,7 +57,7 @@ test('getByTestId, queryByTestId', () => { | |
const component = getByTestId('bananaFresh'); | ||
|
||
expect(component.props.children).toBe('not fresh'); | ||
expect(() => getByTestId('InExistent')).toThrow(); | ||
expect(() => getByTestId('InExistent')).toThrow('No instances found'); | ||
|
||
expect(getByTestId('bananaFresh')).toBe(component); | ||
expect(queryByTestId('InExistent')).toBeNull(); | ||
|
@@ -76,7 +76,8 @@ test('getByName, queryByName', () => { | |
sameButton.props.onPress(); | ||
|
||
expect(bananaFresh.props.children).toBe('not fresh'); | ||
expect(() => getByName('InExistent')).toThrow(); | ||
expect(() => getByName('InExistent')).toThrow('No instances found'); | ||
expect(() => getByName(Text)).toThrow('Expected 1 but found 3'); | ||
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. fixed errors btw, so it doesn't incorrectly say "No instances found" when there's more than 1 |
||
|
||
expect(queryByName('Button')).toBe(button); | ||
expect(queryByName('InExistent')).toBeNull(); | ||
|
@@ -89,7 +90,7 @@ test('getAllByName, queryAllByName', () => { | |
expect(text.props.children).toBe('Is the banana fresh?'); | ||
expect(status.props.children).toBe('not fresh'); | ||
expect(button.props.children).toBe('Change freshness!'); | ||
expect(() => getAllByName('InExistent')).toThrow(); | ||
expect(() => getAllByName('InExistent')).toThrow('No instances found'); | ||
|
||
expect(queryAllByName('Text')[1]).toBe(status); | ||
expect(queryAllByName('InExistent')).toHaveLength(0); | ||
|
@@ -104,7 +105,7 @@ test('getByText, queryByText', () => { | |
const sameButton = getByText('not fresh'); | ||
|
||
expect(sameButton.props.children).toBe('not fresh'); | ||
expect(() => getByText('InExistent')).toThrow(); | ||
expect(() => getByText('InExistent')).toThrow('No instances found'); | ||
|
||
expect(queryByText(/change/i)).toBe(button); | ||
expect(queryByText('InExistent')).toBeNull(); | ||
|
@@ -115,7 +116,7 @@ test('getAllByText, queryAllByText', () => { | |
const buttons = getAllByText(/fresh/i); | ||
|
||
expect(buttons).toHaveLength(3); | ||
expect(() => getAllByText('InExistent')).toThrow(); | ||
expect(() => getAllByText('InExistent')).toThrow('No instances found'); | ||
|
||
expect(queryAllByText(/fresh/i)).toEqual(buttons); | ||
expect(queryAllByText('InExistent')).toHaveLength(0); | ||
|
@@ -126,7 +127,9 @@ test('getByProps, queryByProps', () => { | |
const primaryType = getByProps({ type: 'primary' }); | ||
|
||
expect(primaryType.props.children).toBe('Change freshness!'); | ||
expect(() => getByProps({ type: 'inexistent' })).toThrow(); | ||
expect(() => getByProps({ type: 'inexistent' })).toThrow( | ||
'No instances found' | ||
); | ||
|
||
expect(queryByProps({ type: 'primary' })).toBe(primaryType); | ||
expect(queryByProps({ type: 'inexistent' })).toBeNull(); | ||
|
@@ -137,7 +140,9 @@ test('getAllByProp, queryAllByProps', () => { | |
const primaryTypes = getAllByProps({ type: 'primary' }); | ||
|
||
expect(primaryTypes).toHaveLength(1); | ||
expect(() => getAllByProps({ type: 'inexistent' })).toThrow(); | ||
expect(() => getAllByProps({ type: 'inexistent' })).toThrow( | ||
'No instances found' | ||
); | ||
|
||
expect(queryAllByProps({ type: 'primary' })).toEqual(primaryTypes); | ||
expect(queryAllByProps({ type: 'inexistent' })).toHaveLength(0); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// @flow | ||
import React from 'react'; | ||
import { | ||
Image, | ||
Text, | ||
TextInput, | ||
Modal, | ||
View, | ||
ScrollView, | ||
ActivityIndicator, | ||
TouchableOpacity, | ||
} from 'react-native'; | ||
|
||
import { render } from '..'; | ||
|
||
class RNComponents extends React.Component<*> { | ||
render() { | ||
return ( | ||
<View> | ||
<Modal visible> | ||
<ScrollView> | ||
<Image /> | ||
<TextInput value="value" /> | ||
<TouchableOpacity onPress={() => {}}> | ||
<Text>t1</Text> | ||
<Text>t2</Text> | ||
<Text>t3</Text> | ||
</TouchableOpacity> | ||
<ActivityIndicator show /> | ||
</ScrollView> | ||
</Modal> | ||
</View> | ||
); | ||
} | ||
} | ||
|
||
test('getByName smoke test to see how unstable it gets', () => { | ||
const { getByName } = render(<RNComponents />); | ||
expect(() => getByName('Image')).toThrow(); // – doesn't have displayName set properly | ||
getByName('TextInput'); | ||
expect(() => getByName('Modal')).toThrow(); // – doesn't have displayName set properly | ||
getByName('View'); | ||
getByName('ScrollView'); | ||
expect(() => getByName('ActivityIndicator')).toThrow(); // – doesn't have displayName set properly | ||
getByName('TouchableOpacity'); | ||
}); | ||
|
||
test('getAllByName smoke test to see how unstable it gets', () => { | ||
const { getAllByName } = render(<RNComponents />); | ||
|
||
const [t1, t2, t3] = getAllByName('Text'); | ||
expect(t1.props.children).toBe('t1'); | ||
expect(t2.props.children).toBe('t2'); | ||
expect(t3.props.children).toBe('t3'); | ||
}); |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// @flow | ||
export class ErrorWithStack extends Error { | ||
constructor(message: ?string, callsite: Function) { | ||
super(message); | ||
if (Error.captureStackTrace) { | ||
Error.captureStackTrace(this, callsite); | ||
} | ||
} | ||
} | ||
|
||
export const createLibraryNotSupportedError = (error: Error) => | ||
new Error( | ||
`Currently the only supported library to search by text is "react-native".\n\n${ | ||
error.message | ||
}` | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,42 @@ | ||
// @flow | ||
import * as React from 'react'; | ||
import ErrorWithStack from './errorWithStack'; | ||
import prettyFormat from 'pretty-format'; | ||
import { ErrorWithStack, createLibraryNotSupportedError } from './errors'; | ||
|
||
const getNodeByName = (node, name) => | ||
node.type.name === name || | ||
node.type.displayName === name || | ||
node.type === name; | ||
const filterNodeByType = (node, type) => node.type === type; | ||
|
||
const getNodeByText = (node, text) => | ||
(getNodeByName(node, 'Text') || getNodeByName(node, 'TextInput')) && | ||
(typeof text === 'string' | ||
? text === node.props.children | ||
: text.test(node.props.children)); | ||
const filterNodeByName = (node, name) => | ||
typeof node.type !== 'string' && | ||
(node.type.displayName === name || node.type.name === name); | ||
|
||
const getNodeByText = (node, text) => { | ||
try { | ||
// eslint-disable-next-line | ||
const { Text, TextInput } = require('react-native'); | ||
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. As I understand, we make 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. There may be different text primitives in different environments (like ReactVR). I stand that it's safer to assert on right primitives instead of using string-based comparisons, where the display name may change over time. |
||
return ( | ||
(filterNodeByType(node, Text) || filterNodeByType(node, TextInput)) && | ||
(typeof text === 'string' | ||
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. Wish I could, but Flow looses type information because of fn extraction :< |
||
? text === node.props.children | ||
: text.test(node.props.children)) | ||
); | ||
} catch (error) { | ||
throw createLibraryNotSupportedError(error); | ||
} | ||
}; | ||
|
||
const prepareErrorMessage = error => | ||
// Strip info about custom predicate | ||
error.message.replace(/ matching custom predicate[^]*/gm, ''); | ||
|
||
// TODO: deprecate getByName(string | type) in favor of getByType(type) | ||
export const getByName = (instance: ReactTestInstance) => | ||
function getByNameFn(name: string | React.ComponentType<*>) { | ||
try { | ||
return instance.find(node => getNodeByName(node, name)); | ||
return typeof name === 'string' | ||
? instance.find(node => filterNodeByName(node, name)) | ||
: instance.findByType(name); | ||
} catch (error) { | ||
throw new ErrorWithStack(`Component not found.`, getByNameFn); | ||
throw new ErrorWithStack(prepareErrorMessage(error), getByNameFn); | ||
} | ||
}; | ||
|
||
|
@@ -27,7 +45,7 @@ export const getByText = (instance: ReactTestInstance) => | |
try { | ||
return instance.find(node => getNodeByText(node, text)); | ||
} catch (error) { | ||
throw new ErrorWithStack(`Component not found.`, getByTextFn); | ||
throw new ErrorWithStack(prepareErrorMessage(error), getByTextFn); | ||
} | ||
}; | ||
|
||
|
@@ -36,7 +54,7 @@ export const getByProps = (instance: ReactTestInstance) => | |
try { | ||
return instance.findByProps(props); | ||
} catch (error) { | ||
throw new ErrorWithStack(`Component not found.`, getByPropsFn); | ||
throw new ErrorWithStack(prepareErrorMessage(error), getByPropsFn); | ||
} | ||
}; | ||
|
||
|
@@ -45,15 +63,19 @@ export const getByTestId = (instance: ReactTestInstance) => | |
try { | ||
return instance.findByProps({ testID }); | ||
} catch (error) { | ||
throw new ErrorWithStack(`Component not found.`, getByTestIdFn); | ||
throw new ErrorWithStack(prepareErrorMessage(error), getByTestIdFn); | ||
} | ||
}; | ||
|
||
// TODO: deprecate getAllByName(string | type) in favor of getAllByType(type) | ||
export const getAllByName = (instance: ReactTestInstance) => | ||
function getAllByNameFn(name: string | React.ComponentType<*>) { | ||
const results = instance.findAll(node => getNodeByName(node, name)); | ||
const results = | ||
typeof name === 'string' | ||
? instance.findAll(node => filterNodeByName(node, name)) | ||
: instance.findAllByType(name); | ||
if (results.length === 0) { | ||
throw new ErrorWithStack(`Components not found.`, getAllByNameFn); | ||
throw new ErrorWithStack('No instances found', getAllByNameFn); | ||
} | ||
return results; | ||
}; | ||
|
@@ -62,7 +84,10 @@ export const getAllByText = (instance: ReactTestInstance) => | |
function getAllByTextFn(text: string | RegExp) { | ||
const results = instance.findAll(node => getNodeByText(node, text)); | ||
if (results.length === 0) { | ||
throw new ErrorWithStack(`Components not found.`, getAllByTextFn); | ||
throw new ErrorWithStack( | ||
`No instances found with text: ${String(text)}`, | ||
getAllByTextFn | ||
); | ||
} | ||
return results; | ||
}; | ||
|
@@ -71,7 +96,10 @@ export const getAllByProps = (instance: ReactTestInstance) => | |
function getAllByPropsFn(props: { [propName: string]: any }) { | ||
const results = instance.findAllByProps(props); | ||
if (results.length === 0) { | ||
throw new ErrorWithStack(`Components not found.`, getAllByPropsFn); | ||
throw new ErrorWithStack( | ||
`No instances found with props:\n${prettyFormat(props)}`, | ||
getAllByPropsFn | ||
); | ||
} | ||
return results; | ||
}; | ||
|
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 looks much better C: