feat(userEvent): build-in @testing-library/user-event #616
Conversation
|
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 480fe24:
|
| @@ -24,6 +24,7 @@ test('{esc} triggers typing the escape character', async () => { | |||
| Events fired on: input[value=""] | |||
|
|
|||
| focus | |||
| select | |||
There was a problem hiding this comment.
All of the select events are coming from calling setSelectionRange on the input...
Not sure how to avoid this but I think we should try... Will circle back on this when we do the audit.
|
Alright. Things are starting to look pretty good I think. What we need now is to do the correctness audit and to add a few more tests to get to 100% coverage. |
There was a problem hiding this comment.
There are too many testing utils for my taste. These tend to get bloated over time and in a couple of months nobody understands what a test is doing. I'd really appreciate the extra time to inline these. It costs more writing time but will pay off long term for readability.
src/user-event/.eslintrc
Outdated
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "rules": { | |||
| // the wait in the loop is intentional and we don't want to parallelize | |||
There was a problem hiding this comment.
the loop
Sounds like this should rather be an eslint-disable-next-line
There was a problem hiding this comment.
I copy/pasted that comment from one situation, but there are several. I couldn't think of a single situation in this directory where we'd want the parallization so it made sense to me to just disable it for this directory.
There was a problem hiding this comment.
But this will also apply to any future file/addition. It's intentional for the code right now. But nobody will remember this rule exists if they add new code or it isn't intentional anymore.
There was a problem hiding this comment.
I honestly can't imagine a situation where we'd want parallelization. The async nature of this directory is to simulate the user's behavior (user's can't do two things at once). I think it's more sensible to disable this rule in this directory rather than add comments everywhere. They're just noise and make people feel like they may be doing something wrong.
| @@ -58,7 +58,8 @@ | |||
| "import/prefer-default-export": "off", | |||
| "import/no-unassigned-import": "off", | |||
| "import/no-useless-path-segments": "off", | |||
| "no-console": "off" | |||
| "no-console": "off", | |||
| "no-func-assign": "off" | |||
There was a problem hiding this comment.
The rule seems fairly reasonable to me. Any reasons you can't use function expressions over function declarations?
There was a problem hiding this comment.
This rule bugs me a bit. I prefer this:
function foo() {
}
foo = wrap(foo)Over this:
const foo = wrap(function foo() { // don't want this to be anonymous because I want the name
})Applies to all HOCs I use in React (like React.memo). In here we have this wrapAsync function which wraps all of our utilities in the config's asyncWrapper and I just like the way it looks better when we reassign the function.
There was a problem hiding this comment.
Why do you prefer the first one? Because I definitely didn't know you can re-assign function declarations. I wouldn't think this is valid which is exactly the rationale of this rule. The second pattern looks fine to me.
There was a problem hiding this comment.
In any case: If you like it why not disable it in kcd-scripts and explain the rationale? If you only have a personal, stylistic preference I'd always favor the default ruleset which makes onboarding easier.
There was a problem hiding this comment.
I've been meaning to disable it in eslint-config-kentcdodds for a while now 😅 Just haven't gotten around to it.
There was a problem hiding this comment.
I didn't know that was possible either
kentcdodds
force-pushed
the
pr/user-event
branch
from
1282eae to
ffeed9a
Compare
|
I have some ideas on how we could simplify the test utils. Will work on that. |
|
@kentcdodds might be nice to create a codeshift to help folks migrate their code. |
|
A codemod would be super cool. I will not have the time to work on it unfortunately. But anyone's welcome to make one. So far, here are the differences:
|
|
@kentcdodds Is the plan still that |
|
|
|
oh... ok. curious what the rationale was for that? seems like it'd be a bit confusing to have 2 options there, no? |
|
Because there are some things that we can't really do within |
|
@benmonro Something I suggested was adding a drag-event, which isn't a separate event at this moment, but could be implemented with |
|
Perhaps a lint rule against things that can be done with |
kentcdodds
force-pushed
the
pr/user-event
branch
from
5fabd4b to
db80e27
Compare
I'll look into that |
|
@timdeschryver Don't know if it's possible, but I'm interested in learning how to create codemods and help you out on creating one |
|
@MichaelDeBoey it's not too hard, astexplorer.net is pretty great for building out codemods, lint rules, and really anything AST based. if you go there and choose 'jscodeshift' (which is what codemods are based on) From there it's up to you to write all the jscodeshift code. https://github.com/facebook/jscodeshift |
|
FWIW, I've tried jscodeshift multiple times and never could figure it out, so I use a babel plugin with babel-codemod instead (still recommend astexplorer though). |
Codecov Report
@@ Coverage Diff @@
## master #616 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 24 35 +11
Lines 565 956 +391
Branches 141 250 +109
==========================================
+ Hits 565 956 +391
Continue to review full report at Codecov.
|
|
Alrighty folks! This is officially ready for review (thank you to everyone who's already been involved so far). What remains is in the original comment (right now that's type definition updates and docs, both of which I'd really love help with!!). Regarding the types, I'm thinking I'd prefer to get this merged in before #614. I think that would be quickest and easiest. This is a bit of a shake-up (though luckily most of the changes are in the brand new If you'd like to help with types/docs/bugs, you can make a PR to this branch and I'll merge it in! I won't be working on anything this weekend (family time) but I may be able to slip in a review/merge here or there. I'd love to get this released early next week. Thanks! |
|
Tracking docs contribution: testing-library/testing-library-docs#490 |
| @@ -18,72 +18,78 @@ function fireEvent(element, event) { | |||
| }) | |||
| } | |||
|
|
|||
| const createEvent = {} | |||
| function createEvent( | |||
There was a problem hiding this comment.
No substantial changes here. This was necessary to make it easier to create regular window.Event events that still had the niceties that createEvent had to offer.
| @@ -1,350 +1,360 @@ | |||
| export const eventMap = { | |||
There was a problem hiding this comment.
Formatting change only.
|
Sorry for the massive ping (feel free to unsubscribe from this issue), but I just want to give a huge shout-out to all the contributors of Thank you contributors! 👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏 @Gpx @weyert @twhitbeck @MichaelDeBoey @michaellasky @shomalgan @calebeby @afontcu @skywickenden @bogdanbodnar @zperrault @FLGMwt @benmonro @GentlemanHal @YuanchengWu @maheshjag @jmcriffey @kandros @jordyvandomselaar @ilyamkin @klujanrosas @jsmapr1 @wachunga @bdh1011 @Meemaw @pomber @Raynos @skovy @vadimshvetsov @9still @rbusquet @dougbacelar @kayleighridd @malcolm-kee @kelvinlzhang @krzysztof-hellostudio @hontas @hudochenkov @nanivijay @tpict @nvh95 @nickmccurdy @timdeschryver (hope I didn't miss anyone 😅) |
|
👍 I use both packages in my tests and it makes sense for it to be a single package to install and import. |
I think the underlying problem is that all dispatches are awaited sequentially instead of once. The dispatched events shouldn't be awaited since that wouldn't be the case in a browser anyway. In react we should wrap all dispatches in a single act(() => {
fireEvent.mouseDown();
element.focus();
fireEvent.mouseUp();
element.click();
});instead of act(() => void fireEvent.mouseDown())
act(() => void element.focus())
act(() => void fireEvent.mouseUp())
act(() => void element.click())or for Promises: await Promise.all([fireEvent.mouseDown(), element.focus(), fireEvent.mouseUp(), element.click()]);instead of await fireEvent.mouseDown();
element.focus();
await fireEvent.mouseUp();
element.click(); |
|
This problem is not related to the react testing library. It is an inherit problem of the dom testing library. You can reproduce the bug with just the dom testing library. Here is a small code sandbox with @testing-library/dom only: https://codesandbox.io/s/lingering-firefly-rl9fn In any case
In that case, the dispatches being async is the underlying problem to this problem. Why do they need to be async? |
|
maybe is not the right solution, but if we save a DOM diff for every event between start and end of the event? Then in |
* Exit early from selectOption if select is disabled * fix(userEvent): Port test to dom-testing-library * fix(userEvent): Exit early if options are disabled Co-authored-by: Ben Dyer <[email protected]>
|
The rational for making it all async (and making every fireEvent async) is that a user cannot fire a In any case, I don't think that awaiting between fireEvents is the problem here because the click event we're listening for is the last one that's fired. We don't await anything after that. The only solution I can see as "working" is to make it all sync again, but then it's not really resembling the way the software is used and I'd like to do that... |
We cannot fire them sync as well. They are always consecutive. The issue right now is that we put each event in a separate tick of the (micro?)-task loop. We need an explainer for that. Does the ui-events spec define it that way? |
|
I don't think the spec needs to. How could a user possibly click their mouse fast enough to fire mousedown and mouseup on the same tick of the event loop? They couldn't. |
|
This may be one of the trade-offs we have to make on correctness to practicality though... |
Maybe for mousedown and mouseup. But right now you're putting every fired event in a separate tick without any justification. Just saying "you couldn't do it" is not an argument. |
|
We'd have the same problem for anyone testing a similar situation for typing. So if a loading indicator pops up while the user is typing, they would have this same problem with async type. |
|
I realize that it's possible the browser fires some events in the same event loop tick. I'm not sure how to determine that. But it really doesn't matter. Once we have to make anything async, then we'll have this problem. Remember that the click event is the one that we're listening for in my example and that's the last event that fires in the sequence. |
|
I'm just now realizing the extent of all this async wrapping. Why is focus async? Why do we fire focusIn and await it? (btw the whole implementation is only required for JSDOM. It makes more sense to upstream focusin/focusout). I don't feel comfortable merging user-event in its current state. There are just too many assumptions that may very well be correct but aren't documented in any way. This works for a personal library but not one where we want to share maintenance. |
This is of course a valid design decision and a killing argument for every counter argument argument that goes into the direction of making it synchronous. The thing that concerns me however is that tests never fully mimic the real world. The reason why we run into tgis bug here is because we have an unrealistically fast UI update from state A -> loading -> state B. In the real world, that would never happen in just one async tick. And of course, the bug is solved if you introduce a delay before generating the new random number of, say, 200ms. But that's also how network mocking usually works. You resolve the network request after just one async frame (or at least a very shorter time than it would usually take in the production app). So as long as we resolve network requests so fast, we cannot really affort to have event phases that take so long (compared to all the other async tasks that you have in your app). |
|
I'm beginning to feel that this needs more research before we can commit to bringing this into Dom testing library core. we can take the improvements that I've made in this PR and port them back to the original user event module and we'll wait on for the research before we bring it into core. |
|
I love feedback on that plan. Just give a thumbs up to that comment if you agree or comment with a argument against it if you disagree. |
|
I think this is not a good idea. Porting this to user-event does not make this type of bug go away. It will just lead to a very painful migration for the users of the library to the new version. And I think this would also be a thing that quite some users would run into. And there is nothing that the library can do to nudge the user into the right direction for finding the cause of this bug. Let alone how to solve it. |
|
Sorry, to be more clear, I'd move all my improvements I've made here back to user-event, without the "everything is async now" bit :) |
|
Ok, I'm going to port back my improvements (sans the async-ify everything) and we'll close this here. |
Then the question becomes, why not removing the async stuff and leaving it here? 🙂 |
|
Fair point. I guess it's because I don't think that it's right if we're not doing async and I don't want it in core. It's just not ready yet I think. |
|
Ok, it's official. We're not merging this right now. I've made all the same improvements that I worked on for the last two weeks to user-event itself. Except everything is sync (even I'm disappointed about this (especially since I know several frameworks need everything to be async, so this means we're fragmented), but this is the best we can do. |
|
Oh, and here's the PR: testing-library/user-event#348 |
|
Could we continue merging in |
|
I really don't think we should. I'd prefer to make it async somehow. That's more correct. But I'm out of bandwidth on this so I'm no longer going to be working on it 😫 |
What: This merges
@testing-library/user-eventinto@testing-library/dom.Why: Most of the time people should be using user event.
How:
The first commit was: copy/paste + slightly modify.
This is NOT yet ready to be merged! I have several breaking changes, bug fixes, and new features to add before we'll be able to release this.
I've opened the PR up just to get discussion going. Would love to hear people's thoughts.
Checklist:
I'll mention that I know and accept that there will be limitations to user-event as we bring this in. But there will always be limitations. Despite that I think people are better off using
userEventrather thanfireEventfor situations it works. Also, sometimes it will make more sense for people to give up onuserEventand instead use Cypress to test whatever it is they're trying to test.Current status (help wanted):