-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: User Event core code #1405
Conversation
return; | ||
} | ||
|
||
return Promise.all([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very confusing to me, I don't understand really why the advanceTimers function is used only for fake timers. Here we wait for two promises, one being useful in case fake timers are used and the other in case real timers are used, is this because we don't want to use jestFakeTimersAreEnabled
?
Also I don't really see the point of having advanceTimers
as an option, what could it be used for? We don't really support any other test runner aside from jest currently right? I don't really know what users are using as a test runner but I have the impression that in RN with all the required presets Jest is currently the only solution that's supported by most major libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is taken straight from actual User Event code: https://github.com/testing-library/user-event/blob/main/src/utils/misc/wait.ts
The way it works is that the first part: new Promise<void>(resolve => globalThis.setTimeout(() => resolve(), delay)),
works for both real and fake timers.
The second part config.advanceTimers(delay)
is configurable through userEvent.setup()
call. By default UserEvent has () => Promise.resolve()
there, which allows this function to work with real timers. Having it as an option allows to support different fake timer types, e.g. Sinon.JS and not just Jest ones.
My innovation in that regard is that I've created a universal advanceTimers
default function that works with both real timers and fake Jest timers, which I assume are used by majority of our users using fake timers at all.
Having that as an option allows other fake timers users, still to use them with our User Event. And there have been tickets from users using Sinon.JS as more powerful timers implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this seems reasonable if we want to support other fake timers than the ones from Jest. I'm curious though is there a way in user-event to configure the advanceTimers option globally or does it need to be done every time in the setup function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For global, RTL recommends putting User Event setup into custom render function:
// setup function
function setup(jsx) {
return {
user: userEvent.setup(),
...render(jsx),
}
}
test('render with a setup function', async () => {
const {user} = setup(<MyComponent />)
// ...
})
Source: https://testing-library.com/docs/user-event/intro#writing-tests-with-userevent
@pierrezimmermannbam I've discovered that RTL offers direct access to User Event functions (e.g.
I've added that to this PR. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1405 +/- ##
==========================================
+ Coverage 96.86% 96.88% +0.02%
==========================================
Files 51 65 +14
Lines 3410 3693 +283
Branches 507 538 +31
==========================================
+ Hits 3303 3578 +275
- Misses 107 115 +8
☔ View full report in Codecov by Sentry. |
@mdjastrzebski they support direct access but it seems to be only to ease migrations right? They recommend the usage of the methods returned by setup function so wouldn't it be best to have that as a single option so that we don't introduce breaking changes later on? |
Tbh this seems relatively minor. If TL/UE decides to remove this API that would be a breaking change, which would force a major release. Triggering a major release just for removing this API seems unlikely, more probably they would introduce some other breaking changes as well. That would probably require us to also increase the major version in order to remain compatible, which we could use to also remove the same API, in addition to bringing in other changes. |
Ok yeah that makes sense, all good for me then! |
This PR has been released in v12.1.3 🚢 |
Summary
This PR contain core User Event code to be used by both
type()
andpress()
functions. The API is based on latest User Event v14 API.This PR contains stub implementations for
type()
andpress()
, as well as some re-usable utils:wait()
based on User Event's implementationcreateEventLogger
testing util for inspecting the sequence of events.Some of this stuff is quite opinionated, so if you have ideas of alternative approaches, e.g. to testing, pls voice them so that we have a commonly agreed base for User Event code.
Test plan
Some new tests added, before extracting from #1396 had 100% code coverage.