Skip to content

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

Merged
merged 3 commits into from
Nov 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 7 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ This library is a replacement for [Enzyme](http://airbnb.io/enzyme/).
```jsx
import { render, fireEvent } from 'react-native-testing-library';
import { QuestionsBoard } from '../QuestionsBoard';
import { Question } from '../Question';

function setAnswer(question, answer) {
fireEvent.changeText(question, answer);
}

test('should verify two questions', () => {
const { getAllByName, getByText } = render(<QuestionsBoard {...props} />);
const allQuestions = getAllByName('Question');
const allQuestions = getAllByName(Question);

setAnswer(allQuestions[0], 'a1');
setAnswer(allQuestions[1], 'a2');
Expand Down Expand Up @@ -84,7 +85,7 @@ Deeply render given React element and returns helpers to query the output.
```jsx
import { render } from 'react-native-testing-library';

const { getByTestId, getByName /*...*/ } = render(<Component />);
const { getByTestId, getByText /*...*/ } = render(<Component />);
```

Returns a `RenderResult` object with following properties:
Expand All @@ -104,13 +105,13 @@ type ReactTestInstance = {
};
```

### `getByName: (name: string | React.ComponentType<*>)`
### `getByName: (name: React.ComponentType<*>)`

A method returning a `ReactTestInstance` with matching name – may be a string or React Element. Throws when no matches.
A method returning a `ReactTestInstance` with matching a React component type. Throws when no matches.

### `getAllByName: (name: string | React.ComponentType<*>)`
### `getAllByName: (name: React.ComponentType<*>)`

A method returning an array of `ReactTestInstance`s with matching name – may be a string or React Element.
A method returning an array of `ReactTestInstance`s with matching a React component type.

### `getByText: (text: string | RegExp)`

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"jest": "^23.6.0",
"metro-react-native-babel-preset": "^0.49.0",
"react": "16.6.1",
"react-native": "^0.57.3",
"react-native": "^0.57.5",
"react-test-renderer": "16.6.1",
"release-it": "^7.6.2",
"strip-ansi": "^5.0.0",
Expand Down
8 changes: 4 additions & 4 deletions src/__tests__/__snapshots__/shallow.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ exports[`shallow rendering React Test Instance 1`] = `
`;

exports[`shallow rendering React elements 1`] = `
<Component>
<Component
<View>
Copy link
Collaborator

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:

<Text
testID="text-button"
>
Press me
</Component>
</Component>
</Text>
</View>
`;
19 changes: 12 additions & 7 deletions src/__tests__/render.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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');
Copy link
Member Author

Choose a reason for hiding this comment

The 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();
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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);
Expand Down
55 changes: 55 additions & 0 deletions src/__tests__/renderRNComponents.test.js
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');
});
2 changes: 1 addition & 1 deletion src/debug.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @flow
/* eslint-disable no-console */
import * as React from 'react';
import prettyFormat, { plugins } from 'pretty-format'; // eslint-disable-line import/no-extraneous-dependencies
import prettyFormat, { plugins } from 'pretty-format';
import shallow from './shallow';
import render from './render';

Expand Down
2 changes: 1 addition & 1 deletion src/fireEvent.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @flow
import ErrorWithStack from './helpers/errorWithStack';
import { ErrorWithStack } from './helpers/errors';

const findEventHandler = (element: ReactTestInstance, eventName: string) => {
const eventHandler = toEventHandlerName(eventName);
Expand Down
9 changes: 0 additions & 9 deletions src/helpers/errorWithStack.js

This file was deleted.

16 changes: 16 additions & 0 deletions src/helpers/errors.js
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
}`
);
66 changes: 47 additions & 19 deletions src/helpers/getByAPI.js
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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I understand, we make this require call here to make lib compatible with other environments?

Copy link
Member Author

Choose a reason for hiding this comment

The 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'
Copy link
Collaborator

Choose a reason for hiding this comment

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

typeof something === 'string' is used couple of times throughout the code - can you extract it to method?

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
}
};

Expand All @@ -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);
}
};

Expand All @@ -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);
}
};

Expand All @@ -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;
};
Expand All @@ -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;
};
Expand All @@ -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;
};
Expand Down
Loading