Skip to content

feat: add the value expected in getBy error messages #550

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
merged 5 commits into from
Oct 12, 2020
Merged

feat: add the value expected in getBy error messages #550

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
51d8b52
add the value expected in getBy error messages (e.g. "No instances fo…
Sep 21, 2020
677c5fe
updated a11y test
Sep 21, 2020
dd14187
rename fullNoInstanceError to getNoInstancesFoundMessage
sregg Sep 26, 2020
8613757
addressing PR comments
Sep 28, 2020
5e7c718
fixed lint error
Sep 28, 2020
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
75 changes: 52 additions & 23 deletions src/__tests__/a11yAPI.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ const TEXT_LABEL = 'cool text';
const TEXT_HINT = 'static text';
// Little hack to make all the methods happy with type
const NO_MATCHES_TEXT: any = 'not-existent-element';
const NO_INSTANCES_FOUND = 'No instances found';
const FOUND_TWO_INSTANCES = 'Expected 1 but found 2 instances';

const fullNoInstanceError = (name: string, value: string = NO_MATCHES_TEXT) => {
return `No instances found with ${name} '${value}'`;
};

const Typography = ({ children, ...rest }: any) => {
return <Text {...rest}>{children}</Text>;
};
Expand Down Expand Up @@ -73,7 +76,9 @@ test('getByA11yLabel, queryByA11yLabel, findByA11yLabel', async () => {
const button = queryByA11yLabel(/button/g);
expect(button && button.props.accessibilityLabel).toEqual(BUTTON_LABEL);

expect(() => getByA11yLabel(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getByA11yLabel(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityLabel')
);
expect(queryByA11yLabel(NO_MATCHES_TEXT)).toBeNull();

expect(() => getByA11yLabel(TEXT_LABEL)).toThrow(FOUND_TWO_INSTANCES);
Expand All @@ -83,7 +88,7 @@ test('getByA11yLabel, queryByA11yLabel, findByA11yLabel', async () => {
expect(asyncButton.props.accessibilityLabel).toEqual(BUTTON_LABEL);
await expect(
findByA11yLabel(NO_MATCHES_TEXT, waitForOptions)
).rejects.toThrow(NO_INSTANCES_FOUND);
).rejects.toThrow(fullNoInstanceError('accessibilityLabel'));

await expect(findByA11yLabel(TEXT_LABEL, waitForOptions)).rejects.toThrow(
FOUND_TWO_INSTANCES
Expand All @@ -98,12 +103,14 @@ test('getAllByA11yLabel, queryAllByA11yLabel, findAllByA11yLabel', async () => {
expect(getAllByA11yLabel(TEXT_LABEL)).toHaveLength(2);
expect(queryAllByA11yLabel(/cool/g)).toHaveLength(3);

expect(() => getAllByA11yLabel(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getAllByA11yLabel(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityLabel')
);
expect(queryAllByA11yLabel(NO_MATCHES_TEXT)).toEqual([]);

await expect(findAllByA11yLabel(TEXT_LABEL)).resolves.toHaveLength(2);
await expect(findAllByA11yLabel(NO_MATCHES_TEXT)).rejects.toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityLabel')
);
});

Expand All @@ -118,7 +125,9 @@ test('getByA11yHint, queryByA11yHint, findByA11yHint', async () => {
const button = queryByA11yHint(/button/g);
expect(button && button.props.accessibilityHint).toEqual(BUTTON_HINT);

expect(() => getByA11yHint(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getByA11yHint(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityHint')
);
expect(queryByA11yHint(NO_MATCHES_TEXT)).toBeNull();

expect(() => getByA11yHint(TEXT_HINT)).toThrow(FOUND_TWO_INSTANCES);
Expand All @@ -127,7 +136,7 @@ test('getByA11yHint, queryByA11yHint, findByA11yHint', async () => {
const asyncButton = await findByA11yHint(BUTTON_HINT);
expect(asyncButton.props.accessibilityHint).toEqual(BUTTON_HINT);
await expect(findByA11yHint(NO_MATCHES_TEXT, waitForOptions)).rejects.toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityHint')
);
await expect(findByA11yHint(TEXT_HINT, waitForOptions)).rejects.toThrow(
FOUND_TWO_INSTANCES
Expand All @@ -142,12 +151,14 @@ test('getAllByA11yHint, queryAllByA11yHint, findAllByA11yHint', async () => {
expect(getAllByA11yHint(TEXT_HINT)).toHaveLength(2);
expect(queryAllByA11yHint(/static/g)).toHaveLength(2);

expect(() => getAllByA11yHint(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getAllByA11yHint(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityHint')
);
expect(queryAllByA11yHint(NO_MATCHES_TEXT)).toEqual([]);

await expect(findAllByA11yHint(TEXT_HINT)).resolves.toHaveLength(2);
await expect(findAllByA11yHint(NO_MATCHES_TEXT)).rejects.toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityHint')
);
});

Expand All @@ -160,7 +171,9 @@ test('getByA11yRole, queryByA11yRole, findByA11yRole', async () => {
const button = queryByA11yRole(/button/g);
expect(button && button.props.accessibilityRole).toEqual('button');

expect(() => getByA11yRole(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getByA11yRole(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityRole')
);
expect(queryByA11yRole(NO_MATCHES_TEXT)).toBeNull();

expect(() => getByA11yRole('link')).toThrow(FOUND_TWO_INSTANCES);
Expand All @@ -169,7 +182,7 @@ test('getByA11yRole, queryByA11yRole, findByA11yRole', async () => {
const asyncButton = await findByA11yRole('button');
expect(asyncButton.props.accessibilityRole).toEqual('button');
await expect(findByA11yRole(NO_MATCHES_TEXT, waitForOptions)).rejects.toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityRole')
);
await expect(findByA11yRole('link')).rejects.toThrow(FOUND_TWO_INSTANCES);
});
Expand All @@ -182,13 +195,15 @@ test('getAllByA11yRole, queryAllByA11yRole, findAllByA11yRole', async () => {
expect(getAllByA11yRole('link')).toHaveLength(2);
expect(queryAllByA11yRole(/ink/g)).toHaveLength(2);

expect(() => getAllByA11yRole(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getAllByA11yRole(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityRole')
);
expect(queryAllByA11yRole(NO_MATCHES_TEXT)).toEqual([]);

await expect(findAllByA11yRole('link')).resolves.toHaveLength(2);
await expect(
findAllByA11yRole(NO_MATCHES_TEXT, waitForOptions)
).rejects.toThrow(NO_INSTANCES_FOUND);
).rejects.toThrow(fullNoInstanceError('accessibilityRole'));
});

// TODO: accessibilityStates was removed from RN 0.62
Expand All @@ -209,7 +224,9 @@ test.skip('getByA11yStates, queryByA11yStates', () => {
disabledSelected && disabledSelected.props.accessibilityStates
).toEqual(['selected', 'disabled']);

expect(() => getByA11yStates(NO_MATCHES_TEXT)).toThrow(NO_INSTANCES_FOUND);
expect(() => getByA11yStates(NO_MATCHES_TEXT)).toThrow(
fullNoInstanceError('accessibilityStates')
);
expect(queryByA11yStates(NO_MATCHES_TEXT)).toBeNull();
expect(queryByA11yStates([])).toBeNull();

Expand All @@ -224,7 +241,9 @@ test.skip('getAllByA11yStates, queryAllByA11yStates', () => {
expect(getAllByA11yStates('selected')).toHaveLength(3);
expect(queryAllByA11yStates(['selected'])).toHaveLength(3);

expect(() => getAllByA11yStates([])).toThrow(NO_INSTANCES_FOUND);
expect(() => getAllByA11yStates([])).toThrow(
fullNoInstanceError('accessibilityStates')
);
expect(queryAllByA11yStates(NO_MATCHES_TEXT)).toEqual([]);
});

Expand All @@ -244,7 +263,9 @@ test('getByA11yState, queryByA11yState, findByA11yState', async () => {
expanded: false,
});

expect(() => getByA11yState({ disabled: true })).toThrow(NO_INSTANCES_FOUND);
expect(() => getByA11yState({ disabled: true })).toThrow(
fullNoInstanceError('accessibilityState', '{"disabled":true}')
);
expect(queryByA11yState({ disabled: true })).toEqual(null);

expect(() => getByA11yState({ expanded: false })).toThrow(
Expand All @@ -261,7 +282,9 @@ test('getByA11yState, queryByA11yState, findByA11yState', async () => {
});
await expect(
findByA11yState({ disabled: true }, waitForOptions)
).rejects.toThrow(NO_INSTANCES_FOUND);
).rejects.toThrow(
fullNoInstanceError('accessibilityState', '{"disabled":true}')
);
await expect(
findByA11yState({ expanded: false }, waitForOptions)
).rejects.toThrow(FOUND_TWO_INSTANCES);
Expand All @@ -276,7 +299,7 @@ test('getAllByA11yState, queryAllByA11yState, findAllByA11yState', async () => {
expect(queryAllByA11yState({ selected: true })).toHaveLength(1);

expect(() => getAllByA11yState({ disabled: true })).toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityState', '{"disabled":true}')
);
expect(queryAllByA11yState({ disabled: true })).toEqual([]);

Expand All @@ -286,7 +309,9 @@ test('getAllByA11yState, queryAllByA11yState, findAllByA11yState', async () => {
await expect(findAllByA11yState({ selected: true })).resolves.toHaveLength(1);
await expect(
findAllByA11yState({ disabled: true }, waitForOptions)
).rejects.toThrow(NO_INSTANCES_FOUND);
).rejects.toThrow(
fullNoInstanceError('accessibilityState', '{"disabled":true}')
);
await expect(findAllByA11yState({ expanded: false })).resolves.toHaveLength(
2
);
Expand All @@ -306,7 +331,9 @@ test('getByA11yValue, queryByA11yValue, findByA11yValue', async () => {
max: 60,
});

expect(() => getByA11yValue({ min: 50 })).toThrow(NO_INSTANCES_FOUND);
expect(() => getByA11yValue({ min: 50 })).toThrow(
fullNoInstanceError('accessibilityValue', '{"min":50}')
);
expect(queryByA11yValue({ min: 50 })).toEqual(null);

expect(() => getByA11yValue({ max: 60 })).toThrow(FOUND_TWO_INSTANCES);
Expand All @@ -318,7 +345,7 @@ test('getByA11yValue, queryByA11yValue, findByA11yValue', async () => {
max: 60,
});
await expect(findByA11yValue({ min: 50 }, waitForOptions)).rejects.toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityValue', '{"min":50}')
);
await expect(findByA11yValue({ max: 60 }, waitForOptions)).rejects.toThrow(
FOUND_TWO_INSTANCES
Expand All @@ -333,15 +360,17 @@ test('getAllByA11yValue, queryAllByA11yValue, findAllByA11yValue', async () => {
expect(getAllByA11yValue({ min: 40 })).toHaveLength(1);
expect(queryAllByA11yValue({ min: 40 })).toHaveLength(1);

expect(() => getAllByA11yValue({ min: 50 })).toThrow(NO_INSTANCES_FOUND);
expect(() => getAllByA11yValue({ min: 50 })).toThrow(
fullNoInstanceError('accessibilityValue', '{"min":50}')
);
expect(queryAllByA11yValue({ min: 50 })).toEqual([]);

expect(queryAllByA11yValue({ max: 60 })).toHaveLength(2);
expect(getAllByA11yValue({ max: 60 })).toHaveLength(2);

await expect(findAllByA11yValue({ min: 40 })).resolves.toHaveLength(1);
await expect(findAllByA11yValue({ min: 50 }, waitForOptions)).rejects.toThrow(
NO_INSTANCES_FOUND
fullNoInstanceError('accessibilityValue', '{"min":50}')
);
await expect(findAllByA11yValue({ max: 60 })).resolves.toHaveLength(2);
});
Expand Down
10 changes: 7 additions & 3 deletions src/__tests__/render.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ test('getByText, queryByText', () => {
const sameButton = getByText('not fresh');

expect(sameButton.props.children).toBe('not fresh');
expect(() => getByText('InExistent')).toThrow('No instances found');
expect(() => getByText('InExistent')).toThrow(
"No instances found with text 'InExistent'"
);

const zeroText = getByText('0');

Expand Down Expand Up @@ -186,7 +188,7 @@ test('getByPlaceholderText, queryByPlaceholderText', () => {

expect(sameInput.props.placeholder).toBe(PLACEHOLDER_FRESHNESS);
expect(() => getByPlaceholderText('no placeholder')).toThrow(
'No instances found'
"No instances found with placeholder text 'no placeholder'"
);

expect(queryByPlaceholderText(/add/i)).toBe(input);
Expand Down Expand Up @@ -220,7 +222,9 @@ test('getByDisplayValue, queryByDisplayValue', () => {
const sameInput = getByDisplayValue(INPUT_FRESHNESS);

expect(sameInput.props.value).toBe(INPUT_FRESHNESS);
expect(() => getByDisplayValue('no value')).toThrow('No instances found');
expect(() => getByDisplayValue('no value')).toThrow(
"No instances found with display value 'no value'"
);

expect(queryByDisplayValue(/custom/i)).toBe(input);
expect(queryByDisplayValue('no value')).toBeNull();
Expand Down
24 changes: 19 additions & 5 deletions src/helpers/getByAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,12 @@ export const getByText = (instance: ReactTestInstance) =>
try {
return instance.find((node) => getNodeByText(node, text));
} catch (error) {
throw new ErrorWithStack(prepareErrorMessage(error), getByTextFn);
throw new ErrorWithStack(
`${prepareErrorMessage(error)} with text '${
typeof text === 'string' ? text : text.toString()
}'`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`${prepareErrorMessage(error)} with text '${
typeof text === 'string' ? text : text.toString()
}'`,
`${prepareErrorMessage(error)} with text '${text}'`,

We should probably be able to the if condition, sa template string should do the checks & toString() call for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first idea but I was getting this Flow error:

image

Cannot coerce `text` to string because  `RegExp` [1] should not be coerced.

Copy link
Contributor Author

@sregg sregg Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

${text.toString()}

seems to work
image

Would you be happy with that @mdjastrzebski?

Copy link
Member

@thymikee thymikee Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@sregg doing text.toString() or String(text) (generally safer) is the way to go here, yea 👍

Copy link
Member

Choose a reason for hiding this comment

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

Btw, I think that prepareErrorMessage function is a perfect place for handling this extra text. Similar to what you have in a test here: https://github.com/callstack/react-native-testing-library/pull/550/files#diff-4246008ab708750218ff9e41118401a1R14

getByTextFn
);
}
};

Expand All @@ -112,20 +117,29 @@ export const getByPlaceholderText = (instance: ReactTestInstance) =>
);
} catch (error) {
throw new ErrorWithStack(
prepareErrorMessage(error),
`${prepareErrorMessage(error)} with placeholder text '${
typeof placeholder === 'string' ? placeholder : placeholder.toString()
}'`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`${prepareErrorMessage(error)} with placeholder text '${
typeof placeholder === 'string' ? placeholder : placeholder.toString()
}'`,
`${prepareErrorMessage(error)} with placeholder '${placeholder}'`,

We should probably be able to the if condition, sa template string should do the checks & toString() call for us.

getByPlaceholderTextFn
);
}
};

export const getByDisplayValue = (instance: ReactTestInstance) =>
function getByDisplayValueFn(placeholder: string | RegExp) {
function getByDisplayValueFn(displayValue: string | RegExp) {
try {
return instance.find((node) =>
getTextInputNodeByDisplayValue(node, placeholder)
getTextInputNodeByDisplayValue(node, displayValue)
);
} catch (error) {
throw new ErrorWithStack(prepareErrorMessage(error), getByDisplayValueFn);
throw new ErrorWithStack(
`${prepareErrorMessage(error)} with display value '${
typeof displayValue === 'string'
? displayValue
: displayValue.toString()
}'`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`${prepareErrorMessage(error)} with display value '${
typeof displayValue === 'string'
? displayValue
: displayValue.toString()
}'`,
`${prepareErrorMessage(error)} with display value '${displayValue}'`,

We should probably be able to the if condition, sa template string should do the checks & toString() call for us.

getByDisplayValueFn
);
}
};

Expand Down
14 changes: 12 additions & 2 deletions src/helpers/makeQuery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ const makeQuery = <P: mixed, M: mixed>(
(node) => isNodeValid(node) && matcherFn(node.props[name], matcher)
);
} catch (error) {
throw new ErrorWithStack(prepareErrorMessage(error), getBy);
throw new ErrorWithStack(
`${prepareErrorMessage(error)} with ${name} '${
typeof matcher === 'string' ? matcher : JSON.stringify(matcher) || ''
}'`,
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for using JSON.stringify instead of toString like in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting this error if I use toString()

image

Cannot call `matcher.toString` because property `toString` is missing in  mixed

getBy
);
}
};

Expand All @@ -47,7 +52,12 @@ const makeQuery = <P: mixed, M: mixed>(
);

if (results.length === 0) {
throw new ErrorWithStack('No instances found', getAllBy);
throw new ErrorWithStack(
`No instances found with ${name} '${
typeof matcher === 'string' ? matcher : JSON.stringify(matcher) || ''
Copy link
Member

Choose a reason for hiding this comment

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

we have pretty-format in our deps, so let's use that with {min: true} option set: https://github.com/facebook/jest/tree/master/packages/pretty-format#usage-with-options

}'`,
getAllBy
);
}

return results;
Expand Down