Skip to content

Release Beta #458

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

Release Beta #458

merged 5 commits into from
Mar 12, 2020

Conversation

kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Mar 4, 2020

What: Release the beta branch as stable

Why: Because we have needed breaking changes

How: Please see the commits for granular changes

Checklist:

Please test this in your projects:

npm install @testing-library/dom@beta
# yarn add @testing-library/dom@beta

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 4, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8214a50:

Sandbox Source
friendly-jennings-2zz95 Configuration

@@ -61,7 +59,8 @@
"rules": {
"import/prefer-default-export": "off",
"import/no-unassigned-import": "off",
"import/no-useless-path-segments": "off"
"import/no-useless-path-segments": "off",
"no-console": "off"
Copy link
Contributor

@weyert weyert Mar 4, 2020

Choose a reason for hiding this comment

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

Any reason why you globally disable no-console-rule? I am curious if you couldn't disable for only the specific files?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did and it was annoying.

Copy link
Member

Choose a reason for hiding this comment

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

Once of those things where I'd like a different set of rules for local dev. Basically a set of rules for prod and dev. Because we do like to catch dangling debug statements but we do sometimes need them for debugging locally.

@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

Merging #458 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #458   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          22     22           
  Lines         404    420   +16     
  Branches       95    100    +5     
=====================================
+ Hits          404    420   +16
Impacted Files Coverage Δ
src/pretty-dom.js 100% <ø> (ø) ⬆️
src/helpers.js 100% <ø> (ø) ⬆️
src/events.js 100% <ø> (ø) ⬆️
src/wait.js 100% <100%> (ø) ⬆️
src/config.js 100% <100%> (ø) ⬆️
src/wait-for-dom-change.js 100% <100%> (ø) ⬆️
src/wait-for-element.js 100% <100%> (ø) ⬆️
src/role-helpers.js 100% <100%> (ø) ⬆️
src/wait-for-element-to-be-removed.js 100% <100%> (ø) ⬆️
src/queries/label-text.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 bb4a1ed...8214a50. Read the comment docs.

@kentcdodds kentcdodds force-pushed the beta branch 2 times, most recently from fd701b4 to 6bb57a9 Compare March 4, 2020 20:34
eps1lon
eps1lon previously approved these changes Mar 4, 2020
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Looks good. Only test that failed for us was due to A11yance/aria-query#42 which isn't harmful at all.

@timdeschryver
Copy link
Member

timdeschryver commented Mar 5, 2020

I encountered a small (non blocking) typing problem with Angular Testing Library, because @types/testing-library__dom hasn't been updated with the latest changes.

I'm not really comfortable with @types, but do they also publish beta versions or should we wait until this version gets released?

For the rest, the example tests in the repo, were all green. 👍

@kentcdodds
Copy link
Member Author

I don't know how DefinitelyTyped handles beta versions. I'm not going to bother. We'll be merging this before too long.

@kentcdodds
Copy link
Member Author

Just an update. I'll planning on working on this and doing the release next Tuesday or Wednesday. 👍👍

@kentcdodds
Copy link
Member Author

I'm ready to release this tomorrow. I'd like to get the green light from as many maintainers as possible. Please leave a review @testing-library/all-maintainers!

@kentcdodds
Copy link
Member Author

Keep in mind that we like to avoid major releases like this, so if there's anything you have in mind for a breaking change, please speak now or forever hold your peace... well, maybe not forever, but yeah.

@timdeschryver
Copy link
Member

👍 for Angular Testing Library

eps1lon
eps1lon previously approved these changes Mar 12, 2020
@afontcu
Copy link
Member

afontcu commented Mar 12, 2020

I'm giving it a go right now in Vue Testing Lib: testing-library/vue-testing-library#125

would appreciate your input on both the empty callback when using wait, and also the failing stopwatch tests (source). Not sure how to implement it now 🤔 (it's failing on empty callback for wait, but I guess I need to rethink the whole test case)

@benmonro
Copy link
Member

benmonro commented Mar 12, 2020

ok a few questions:

  1. Given that the no op option for wait was removed is there a work around for folks that have usages of it? That one seems like it would cause a lot of breaking tests that would have to be fixed one by one. Which is probably worth doing but my 2 cents would be to give a deprecation warning on that before removing it like the other async functions.

  2. Speaking of which, why get rid of wait for element and wait for element to be removed? I think they really improve the readability of tests. I get they can be done with wait or find* but for me it does improve the style of the test when you don't need to do an expect on those things.

  3. While I understand and agree w/ the reasoning behind deprecating waitForDomChange, I do find it to be a very useful debugging tool when writing/debugging tests. I have often found myself starting w/ that, and then getting rid of it. Much the same way I'll start with debug() and remove that as well. This is especially true when converting enzyme tests to RTL (of which we have many) Why not just keep waitForDomChange and then just adding a lint rule for it?

timdeschryver
timdeschryver previously approved these changes Mar 12, 2020
@kentcdodds
Copy link
Member Author

Alright, I'm working on this now: https://twitter.com/kentcdodds/status/1238197276465483776

@benmonro
Copy link
Member

@kentcdodds any thoughts on my questions before you release? :)

@kentcdodds
Copy link
Member Author

Yes, that's what I'm doing in the livestream 😉

@benmonro
Copy link
Member

damn i'm in meetings. i can't make it

@kentcdodds
Copy link
Member Author

i'm in meetings. i can't make it

Don't worry, what I meant was, I'm typing out the answer in the livestream :)

@benmonro
Copy link
Member

ok posted some comments in the youtube comments

@kentcdodds
Copy link
Member Author

For the empty callback with wait the solution is quite simple:

- await wait()
+ await wait(() => {})

So that can be a find/replace update, and then the recommendation would be for people to go through and put assertions in that callback instead of having an empty callback.

As for waitForElementToBeRemoved, that is not getting deprecated or removed. But waitForDomChange is removed because that's basically what wait does now, and waitForElement is what find* queries do and if you can't use a query, then wait does that now.

So the solution for those will be:

- const element = await waitForElement(() => container.querySelector('.loading'))
+ const element = await wait(() => container.querySelector('.loading'))

- await waitForDomChange()
+ await wait(() => {})

- await waitForDomChange(mutationObserverOptions)
+ await wait(() => {}, mutationObserverOptions)

So that's actually pretty straightforward. Most of those should be find/replace. The idea is, because wait now supports mutation observer (as well as the interval), we no longer need these one-offs. Also waitForDomChange was testing implementation details (the user doesn't care about any old DOM change, they care about specific DOM changes), and the wait with a no-op callback is also implementation details (because the user doesn't care how many ticks of the event loop something takes, they just care about waiting for something specific to happen).

Regarding readability, I don't think they're any more readable personally. Especially since you shouldn't be using waitForElement very often (you should use a query). In the above diff, I don't see waitForElement being any more understandable than the wait version.

waitForElementToBeRemoved is kept around because it's harder to implement what it does in userland. So it provides some actual utility.

For waitForDomChange I'm not a huge fan of including things and linting against them. debug is special because it logs something to the terminal that people wouldn't need a lint rule to tell them to remove. waitForDomChange is something that you could use to make your test pass, so you may decide to use it to do that. If your really like it for your workflow, then you can copy/paste what we have now and move it to your project, but I really don't want to encourage anyone to rely on specific and single DOM changes in their tests without waiting for a specific thing to happen (ie an assertion to pass).

@benmonro
Copy link
Member

I guess one thing i'm hung up on is the naming, instead of wait what about naming it waitFor

I've been teaching folks on my team not to use wait because of the fact that it doesn't use mutation observers. now i'm going to have to teach them to use it. waitFor also solves the readability issue imo

@kentcdodds
Copy link
Member Author

The library we used to use was called waitForExpect and I didn't like that because it was too specific to expect. waitFor sounds pretty good and I don't really have arguments against it. I'd really like to hear what other people have to say about it.

Maybe I'll do a twitter poll too 😅

@kentcdodds
Copy link
Member Author

Hmmm... What if we alias wait to waitFor and then in the next major version bump we switch it?

I don't really like that but it's an idea.

@benmonro
Copy link
Member

not sure how i feel about the alias. like we're already breaking changes anyway. people are already going to have to replace waitForElement and waitForDomChange.

@kentcdodds
Copy link
Member Author

Yeah, let's not do the alias... I'm leaning toward renaming to waitFor though... Just not excited about the fallout... Don't want to rock the boat too much for something small like this.

@kentcdodds
Copy link
Member Author

Alright, I'm not excited about making this change, but I think it's for the best, so we'll be renaming wait (as it is in this PR) to waitFor (also, here's the poll where waitFor is coming out with a big lead anyway: https://twitter.com/benmonro/status/1238209158622535680)

@kentcdodds
Copy link
Member Author

Some people are suggesting: await until(() => expect(foo).toBe(true)).

Curious what others have to say. I'm still leaning toward waitFor...

@benmonro
Copy link
Member

I don't hate until, but I think i still prefer waitFor as that's more in line w/ the other helpers & history of the library

kentcdodds and others added 5 commits March 12, 2020 15:24
Closes #430

BREAKING CHANGE: This drops support for Node 8. Node 10 or greater is now required.
Closes #372

BREAKING CHANGE: If you used the `selector` option in `ByLabelText` queries, then you will probably need to update that code to be able to find the label you're looking for.
Closes #376
Closes #416

BREAKING CHANGE: wait is now deprecated in favor of waitFor (which has basically the same API except it requires a callback and it also accepts optional mutation observer arguments).
BREAKING CHANGE: `waitForElement` is deprecated in favor of `find*` queries or `wait`.
BREAKING CHANGE: `waitForDomChange` is deprecated in favor of `wait`
BREAKING CHANGE: default timeout for async utilities is now 1000ms rather than 4500ms. This can be configured: https://testing-library.com/docs/dom-testing-library/api-configuration
Closes #413
Closes #357

BREAKING CHANGE: MutationObserver is supported by all major browsers and recent versions of JSDOM. If you need, you can create your own shim (using @sheerun/mutationobserver-shim) and attach it to the window.
@kentcdodds kentcdodds dismissed stale reviews from timdeschryver and eps1lon via 8214a50 March 12, 2020 21:31
@kentcdodds kentcdodds merged commit 1b711a4 into master Mar 12, 2020
@kentcdodds kentcdodds deleted the beta branch March 12, 2020 21:51
@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@timdeschryver
Copy link
Member

it should be possible to have eslint fix these occurrences of wait and waitForDomChanges to make the upgrade path easier. See testing-library/eslint-plugin-testing-library#86

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

Successfully merging this pull request may close these issues.

7 participants