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

kentcdodds · 2019-03-19T22:37:09Z

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

Why: Closes #202

TL;DR: more explicit, more better.

How: Added some logic for the getBy queries to throw if the returned elements has length > 1

Checklist:

Documentation added to the docs site
Typescript definitions updated N/A
Tests
Ready to be merged

BREAKING CHANGE: All getBy and findBy query variants now will throw an error if more than one element is returned. If this is expected, then use getAllBy (or findAllBy) instead.

kentcdodds · 2019-03-19T22:37:45Z

Note: this PR is to the next branch because it's a breaking change.

I can't think of any other breaking changes I want to make at this time, so once this is merged, we'll go ahead and merge the next branch as well.

alexkrolick · 2019-03-19T23:05:38Z

src/queries.js

@@ -136,7 +137,6 @@ function queryAllByTitle(
 function queryByTitle(...args) {
  return firstResultOrNull(queryAllByTitle, ...args)


So the query APIs would still return the first one even if there are multiple?

Correct. Hmmm... Should we throw for this as well? Probably huh...

Not sure... querySelector vs querySelectorAll would just return the first item. Almost the only use case for this is asserting null anyways... so... 🤔

I could see it throwing as well. Maybe to clarify it all we could make a table:

Proposed

One Match Multiple Matches No Match Async?

getBy Return Throw Throw No

findBy Return Throw Throw Yes

queryBy Return Throw (?) Null No

getAllBy Array Array Throw No

findAllBy Array Array Throw Yes

queryAllBy Array Array Empty Array No

Current

One Match Multiple Matches No Match Async?

getBy Return First Throw No

findBy Return First Throw Yes

queryBy Return First Null No

getAllBy Array Array Throw No

findAllBy Array Array Throw Yes

queryAllBy Array Array Empty Array No

In this model does it make sense to have query and queryAll? Should there just be:

query
get
getAll
find
findAll?

To me that seems pretty consistent with like, the type of behavior you'd expect from a REST API for example. like /api/users is going to return a list of users even if there's only 1 (or 0). But if i say /api/users/1 or /api/users/1,2 i'm asking for specific things and I'll get a 404 if they're not found.

🙄 duh it was the first sentence, nvm i can't read apparently.

I feel like I'd like to see query continue to never throw. I think it's nice to have a set of searches that you know aren't going to throw in any circumstance, but then again it might catch some bugs people don't know about currently... I dunno 🤔

Do you see any chance that throwing might make it more confusing for knowing when to use query vs. get since they both have the potential to throw in this model?

If you think there could be more than one then use queryAll*

The primary use case for queryBy is for when you expect something not to exist and you want to check it returns null. The only I can think of it being disruptive to change it would be if you were doing something with .not, like

await wait(() => expect(queryByText('something')).not.toBeNull())

Which, in that case, would throw inside the wait and trigger a retry same as a failed expect. There might be some places where it's used differently/synchronously though.

The proposed error message makes it pretty clear what's going on if you see it, IMO. Could even link to the release notes for the major version.

sounds good to me, i can dig it. we can make this happen in native too 👍

alexkrolick · 2019-03-19T23:06:15Z

src/query-helpers.js

@@ -29,6 +29,13 @@ function getElementError(message, container) {
  return new Error([message, debugDOM(container)].filter(Boolean).join('\n\n'))
 }

+function getGetByElementError(message, container) {
+  return getElementError(
+    `${message}\n\n(If this is intentional, then use the \`*AllBy*\` variant of the query (like \`getAllByText\` or \`findAllByText\`)).`,


This error message is nice and clear and makes the breaking change pretty safe

Might be able to enhance it even more with another parameter like query, which contains the suffix, AllByText. Then this could be updated to:

`${message}\n\n(If this is intentional, then use the \`get${query}\` or \`find${query}\` variant of the query).`

bcarroll22 · 2019-04-18T21:43:44Z

I’ve pretty much got this ready in NTL, just waiting to merge/release until I know whether or not queryBy should throw as well 👍

alexkrolick · 2019-04-18T21:46:02Z

@kentcdodds how about releasing a beta release with queryBy throwing, then we can test it out on some codebases and see if it breaks a lot.

kentcdodds · 2019-04-18T21:52:42Z

I can do that 👍

kentcdodds · 2019-04-18T22:59:32Z

Sorry it's taking so long. I finally decided to look at this thing through the lens of abstraction (https://kcd.im/aha) and I think I've figured out a good way to avoid copy/paste issues and hopefully make abstraction worthwhile and make this easier to maintain. We'll see...

kentcdodds · 2019-04-18T23:20:33Z

Ok, feel free to look at the PR. It's kinda a big diff now, but I think it'll be a good thing for the codebase. I'd love to hear what people think.

I've also published an alpha:

yarn add --dev dom-testing-library@alpha

Let me know what you think!

alexkrolick · 2019-04-18T23:22:39Z

@kentcdodds the makeXQueries are an interesting abstraction - do you think you could simplify it even more to something like this:

const [getByText, getAllByText, findByText, findAllByText, queryByText]
  = buildQueries(queryAllByText, e => 'single error', e => 'multiple error')

kentcdodds · 2019-04-18T23:24:02Z

I had considered that as well. Maybe I'll give it a try 👍 I like what you've got there.

kentcdodds · 2019-04-18T23:24:29Z

I've just published an alpha of react-testing-library that upgrades dom-testing-library to the alpha as well:

yarn add --dev react-testing-library@alpha

kentcdodds · 2019-04-18T23:26:52Z

I just verified that my workshop code all still works with these changes so that's great :)

kentcdodds · 2019-04-18T23:37:33Z

Great idea on the buildQueries thing @alexkrolick 👍 I like it.

I'm a tiny bit worried this is over-abstraction. But I think it'll be better this way. I think we've had this copy/pasted code long enough to know what the right abstraction is here.

alexkrolick · 2019-04-18T23:52:38Z

I'm a tiny bit worried this is over-abstraction. But I think it'll be better this way. I think we've had this copy/pasted code long enough to know what the right abstraction is here.

I think it's fairly safe to assume we can continue abstracting the query types off of a base query- I considered it even back last year when adding the queryAll functions for the first time. It'll make it a lot simpler for people to build custom queries that implement all the query types.

kentcdodds · 2019-04-19T00:09:29Z

That's a good point!

bcarroll22 · 2019-04-19T00:39:08Z

Man. Oh. Man.

This is a good stuff. I think I need to digest it a bit more, but I believe this even helps us in native be more explicit and make the tests more closely resemble the way the platform works. To me it's a good balance, it abstracts where it makes sense, but it's also a good example of not abstracting when it makes sense not to.

This is awesome, I like it a lot.

bcarroll22 · 2019-04-19T01:13:31Z

Question.

When you were doing this, how did you decide when/when not to use the abstractions vs writing a custom implementation? For example, you limit what elements are queried for display-value but not for placeholder. Was there a reason for that?

kentcdodds · 2019-04-19T15:36:35Z

I dunno 🤷‍♂️

Closes #202 BREAKING CHANGE: All `getBy` and `findBy` query variants now will throw an error if more than one element is returned. If this is expected, then use `getAllBy` (or `findAllBy`) instead.

kentcdodds · 2019-04-25T18:45:14Z

Ok, I'm ready to make this happen :)

Closes #202 BREAKING CHANGE: All `getBy` and `findBy` query variants now will throw an error if more than one element is returned. If this is expected, then use `getAllBy` (or `findAllBy`) instead.

kentcdodds requested review from alexkrolick and wyze March 19, 2019 22:38

alexkrolick reviewed Mar 19, 2019

View reviewed changes

kentcdodds force-pushed the next branch from eb333a6 to 605b0d7 Compare March 20, 2019 22:06

kentcdodds force-pushed the pr/throw-get-by branch from f3d9b06 to af9df05 Compare March 20, 2019 22:08

kentcdodds force-pushed the next branch from e077240 to f792625 Compare March 20, 2019 23:08

kentcdodds force-pushed the pr/throw-get-by branch from af9df05 to 09826d5 Compare March 20, 2019 23:10

kentcdodds force-pushed the next branch from f792625 to ff00ee2 Compare April 17, 2019 14:39

kentcdodds force-pushed the pr/throw-get-by branch from d39b4ea to 410c6ed Compare April 17, 2019 14:41

bcarroll22 mentioned this pull request Apr 19, 2019

fix(getBy*): throw an error if more than one element is found testing-library/native-testing-library#7

Merged

kentcdodds force-pushed the next branch from ff00ee2 to 48b79d0 Compare April 25, 2019 18:44

Kent C. Dodds and others added 2 commits April 25, 2019 12:44

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

2c4f8ca

Closes #202 BREAKING CHANGE: All `getBy` and `findBy` query variants now will throw an error if more than one element is returned. If this is expected, then use `getAllBy` (or `findAllBy`) instead.

WIP: I forgot about this... Will come back later

9d5b32d

Kent C. Dodds added 4 commits April 25, 2019 12:44

more progress

3ecbb0a

fix version

7cdb55d

use buildQueries

0bdea33

whoops

78ec79c

kentcdodds force-pushed the pr/throw-get-by branch from a40e86d to 78ec79c Compare April 25, 2019 18:45

kentcdodds merged commit 12b0222 into next Apr 25, 2019

kentcdodds deleted the pr/throw-get-by branch April 25, 2019 18:46

		@@ -136,7 +137,6 @@ function queryAllByTitle(
		function queryByTitle(...args) {
		return firstResultOrNull(queryAllByTitle, ...args)

	One Match	Multiple Matches	No Match	Async?
getBy	Return	Throw	Throw	No
findBy	Return	Throw	Throw	Yes
queryBy	Return	Throw (?)	Null	No
getAllBy	Array	Array	Throw	No
findAllBy	Array	Array	Throw	Yes
queryAllBy	Array	Array	Empty Array	No

	One Match	Multiple Matches	No Match	Async?
getBy	Return	First	Throw	No
findBy	Return	First	Throw	Yes
queryBy	Return	First	Null	No
getAllBy	Array	Array	Throw	No
findAllBy	Array	Array	Throw	Yes
queryAllBy	Array	Array	Empty Array	No

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

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

Uh oh!

Conversation

kentcdodds commented Mar 19, 2019

Uh oh!

kentcdodds commented Mar 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkrolick Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkrolick Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkrolick Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcarroll22 commented Apr 18, 2019

Uh oh!

alexkrolick commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019

Uh oh!

alexkrolick commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019

Uh oh!

kentcdodds commented Apr 18, 2019 • edited by alexkrolick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexkrolick commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentcdodds commented Apr 19, 2019

Uh oh!

bcarroll22 commented Apr 19, 2019

Uh oh!

bcarroll22 commented Apr 19, 2019

Uh oh!

kentcdodds commented Apr 19, 2019

Uh oh!

kentcdodds commented Apr 25, 2019

Uh oh!

Uh oh!

alexkrolick Mar 19, 2019 •

edited

Loading

alexkrolick Mar 20, 2019 •

edited

Loading

alexkrolick Apr 16, 2019 •

edited

Loading

kentcdodds commented Apr 18, 2019 •

edited by alexkrolick

Loading

alexkrolick commented Apr 18, 2019 •

edited

Loading