Skip to content
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

Add async/retryable findBy* and findAllBy* version of the queries #203

Closed
kentcdodds opened this issue Feb 4, 2019 · 12 comments
Closed
Labels
enhancement New feature or request help wanted Extra attention is needed released

Comments

@kentcdodds
Copy link
Member

Describe the feature you'd like:

@alexkrolick has brought this up before and we ultimately decided to not go for it at the time, but I've been thinking about it more and I think it's time to revisit the decision.

There are some UI frameworks that are completely async. In my TestingJavaScript.com course when I show how to use dom-testing-library with these, I had to create a fire-event-async.js file to deal with those, but that wasn't enough for mithril's tests because mithril does not re-render on the next tick all the time (for who knows what reason 🤷‍♂️).

The findBy* and findAllBy* variants of the queries would return promises that resolve when the query is satisfied, or timeout after 3500ms or so.

Suggested implementation:

I'm thinking it'd be something like this:

const findByText = (...args) => waitForElement(() => getByText(...args))
const findAllByText = (...args) => waitForElement(() => getAllByText(...args))

Describe alternatives you've considered:

So far the alternative is to have people write this themselves:

const successMessage = await waitForElement(() => getByText(/.*success.*/i))
expect(successMessage).toHaveTextContent('Welcome back Nancy')

That's worked pretty well most of the time, but I think it would be nicer to write that like this:

const successMessage = await findByText(/.*success.*/i)
expect(successMessage).toHaveTextContent('Welcome back Nancy')

Teachability, Documentation, Adoption, Migration Strategy:

We do have query explosion going on and I think the docs could be improved slightly around this to document first the types of queries (label text, text, placeholder, etc), and then the variants (getBy*, queryAllBy*, etc.).

This would be a new feature, so no breaking change/migration strategy necessary.

@alexkrolick
Copy link
Collaborator

Feel free to pull in https://github.com/alexkrolick/dom-testing-addon-async

@kentcdodds
Copy link
Member Author

How has that lib been going @alexkrolick? Are you using it? Do you know of anyone else using it? What do you think?

@alexkrolick
Copy link
Collaborator

alexkrolick commented Feb 4, 2019

I used it in Puppeteer to simulate Cypress-like retries and it worked pretty well.

The tricky part was more dealing with dist and CJS/ES compatibility, and something about mutation observer being required in the node env when the queries were iterated to do the re-export, which wouldn't matter if it was in core.

@alexkrolick alexkrolick changed the title Add findBy* and findAllBy* version of the queries Add async/retryable findBy* and findAllBy* version of the queries Feb 4, 2019
@kentcdodds
Copy link
Member Author

I think that it's safe to say we can take a PR to add these queries. I may get to it eventually if nobody else does it first :)

@Tolsee
Copy link
Contributor

Tolsee commented Feb 26, 2019

Hey @kentcdodds, I can give a try to this(most probably this weekend).

@weyert
Copy link
Contributor

weyert commented Mar 7, 2019

What's the idea just copy over the files from the other repo? I can do that :D

@kentcdodds
Copy link
Member Author

What other files/repo?

@kentcdodds
Copy link
Member Author

Oh, no, this will be implemented as queries instead. Much better.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Mar 7, 2019

Yeah, create named functions in queries.js. Don't use the custom wait either, use waitForElement.

@weyert
Copy link
Contributor

weyert commented Mar 7, 2019

Ah okay, I will see if I find the time over the weekend/next week. If anyone wants to work on this don't hesitate to do so :)

@kentcdodds
Copy link
Member Author

kentcdodds pushed a commit that referenced this issue Mar 19, 2019
…ries (#224)

* feat(findBy*): adds findBy* query type allowing for async element queries

Closes #203

* undo something

* add a real async test
@kentcdodds
Copy link
Member Author

🎉 This issue has been resolved in version 3.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed released
Projects
None yet
Development

No branches or pull requests

4 participants