Skip to content
This repository has been archived by the owner on Jul 30, 2020. It is now read-only.

fix(getBy*): throw an error if more than one element is found #7

Merged
merged 5 commits into from Apr 25, 2019

Conversation

bcarroll22
Copy link
Collaborator

@bcarroll22 bcarroll22 commented Apr 19, 2019

This refactors queries to handle some abstraction, and also throws if more than one result is returned from getBy, findBy, and queryBy. This is done to maintain compatibility w/ dom-testing-library.

This also makes the value query better because it limits to only components that have a value prop.

For reference: DTL PR

Brandon Carroll and others added 3 commits April 18, 2019 16:48
BREAKING CHANGE: `queryBy` and `getBy` now throw when multiple elements are returned and prompt
users to use other queries if multiple results were intentional. This was done to remain in feature
parity with dom-testing-library.
@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (next@77155c2). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##             next     #7   +/-   ##
=====================================
  Coverage        ?   100%           
=====================================
  Files           ?     23           
  Lines           ?    277           
  Branches        ?     46           
=====================================
  Hits            ?    277           
  Misses          ?      0           
  Partials        ?      0
Impacted Files Coverage Δ
src/query-helpers.js 100% <100%> (ø)
src/queries/value.js 100% <100%> (ø)
src/queries/a11y-states.js 100% <100%> (ø)
src/queries/a11y-hint.js 100% <100%> (ø)
src/queries/a11y-label.js 100% <100%> (ø)
src/queries/test-id.js 100% <100%> (ø)
src/queries/placeholder.js 100% <100%> (ø)
src/queries/a11y-traits.js 100% <100%> (ø)
src/queries/text.js 100% <100%> (ø)
src/queries/a11y-role.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77155c2...94a1349. Read the comment docs.

src/__tests__/misc.js Outdated Show resolved Hide resolved
const getMultipleError = (c, hint) =>
`Found multiple elements with the accessibilityHint of: ${hint}`;
const getMissingError = (c, hint) =>
`Unable to find an element with the accessibilityHint of: ${hint}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These error messages are shared across almost everything - consider pulling out some helper so you can just do:

getMissingErrorMessage('accessibilityHint', hint)

For custom ones (like getByText), you can always just opt not to use the helper, or compose the helper with additional text.
Mostly I'm just worried about eventually introducing a typo, especially since the tests only partially check the error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me think about this one. I’ll definitely fix the other two, my only concern with this one is maintainability (with regards to keeping in parity with dom-testing-library). Whenever there’s a big refactor over there, it might make it hard to pull stuff over here.

Right now, it’s easy to keep up to date with the rest of the family because it’s so similar. I think at some point we’ll have to decide if we wanna split from *-testing-library on code style like this. I don’t think it’s bad so long as the API and feel for users is the same, it just might make it tough to keep up.

src/query-helpers.js Outdated Show resolved Hide resolved
@bcarroll22 bcarroll22 changed the base branch from master to next April 25, 2019 00:22
@bcarroll22 bcarroll22 changed the title throw an error if more than one element is found fix(getBy*): throw an error if more than one element is found Apr 25, 2019
@bcarroll22 bcarroll22 merged commit ebd4410 into next Apr 25, 2019
@bcarroll22 bcarroll22 deleted the pr/throw-get-by branch April 25, 2019 18:52
@bcarroll22
Copy link
Collaborator Author

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants