Re-thinking approach to async event handling

[nickserv]

See testing-library/dom-testing-library#616 (comment)

We've been discussing some differences in Testing Library behavior with asyncronous renders and data fetching. The official Vue Test Utils docs (which Vue Testing Library uses) recommend awaiting its events so the library could internally wait for Vue to finish rendering. However, User Event is used with some libraries like React and Angular that don't have async renders like this. This causes potential race conditions with the UI when awaiting things like fireEvent or userEvent.

I'm starting to feel that perhaps this test example shouldn't rely on the race condition being fixed. For example in a real world app, if a web app loads very quickly its loader may not be displayed (perhaps it's already cached or the website has a threshold to prevent it from loading). A real human manually testing a website wouldn't give up after not seeing the loader, they would just continue as long as the page changed and they would wait for a loader if they saw one. As a result, I don't think waitForElement is the right function to make sure a loader disappears, especially since frameworks like React may batch updates when async functions are mocked. What matters here is that the element isn't displayed, so .not.toBeInTheDocument() is more accurate and reliable.

I think that instead of trying to get User Event to run the original code without modifications, we should update waitForElement's error message for initial null elements to explain that the element may have already been removed and give alternatives like using Jest DOM's .not.toBeInTheDocument() instead, and potentially document other common issues. Then we'll be able to make User Event functionality async, which makes it more consistent both with real user behavior and some frameworks like Vue and Svelte which already deal with async renders in tests. It's also worth noting that because userEvent isn't async, its APIs are harder to wrap for async Vue tests and users have to learn more implementation details of Vue as a result (see testing-library/vue-testing-library#182).

CC @kentcdodds @afontcu @ITenthusiasm

[kentcdodds]

Thanks for bringing this up. I've never felt comfortable making all of these synchronous. The user never fires these events synchronously, so I don't like that these fire synchronously. But you explain the problem with the alternative really well. Specifically about asserting on the loading state. The suggestion is:

What matters here is that the element isn't displayed, so .not.toBeInTheDocument() is more accurate and reliable.

But that missing some important assertion. I think it's important that we assert that the loading state exists. As you point out, it's not reasonably possible to make this assertion if we make all these events fire asynchronously (which, for everyone else's benefit, is the reason user-event is still not built-into core: testing-library/dom-testing-library#616 (comment)).

Unless we can find a reasonable way to solve this problem, I don't know if we can proceed 😬

[nickserv]

Specifically about asserting on the loading state... But that missing some important assertion. I think it's important that we assert that the loading state exists.

I think that's the issue here. Expecting a loading state to appear is an implementation detail that shouldn't be tested. Sometimes there will be an intermediate loading state, and sometimes there won't. I would argue that the failure here is because the test is relying on this fragile assumption, since as I explained above the loader may not even be rendered or visible to the user in a production environment.

If the issue is that users need to test a loading state on a slow network, they can simulate that using mocks/timers/etc, but they shouldn't write their tests in such a way that depends on race conditions that do or do not display a loader. Also as far as I understand the Vue community (not just VTL but the official VTU) has been doing this for a while (the docs mention that you can mock a request and trigger assertions on next tick if you need to assert around the race condition).

[kentcdodds]

I think that's the issue here. Expecting a loading state to appear is an implementation detail that shouldn't be tested.

I think that's where we disagree. I don't find loading states to be an implementation detail at all. Loading states (like error states) are an important part of the user experience and should be tested. I can appreciate this statement though:

Sometimes there will be an intermediate loading state, and sometimes there won't.

I guess I'll have to spend some time thinking about this and determining how hard it is to test those loading states using explicit slow mocks/timers... Hmmmm.... I'm curious what others have to say here.

[nickserv]

What if we had a blog post with examples of various ways to deal with these situations, and then made the core events in userEvent (and possibly fireEvent for consistency) async? If we're concerned about back compatibility, we could publish prereleases first.

Another advantage to doing this sooner is that we wouldn't have to make a separate async version of the fireEvent/userEvent API for Vue and Svelte (which internally fire events asyncronously).

I guess I'll have to spend some time thinking about this and determining how hard it is to test those loading states using explicit slow mocks/timers...

If you'd like to give some examples of tests with these issues, we can start by sharing various solutions, and then figure out what we want to recommend in the documentation or blog.

[afontcu]

Thanks for bringing this up! I look forward the moment we have the sync/async situation sorted out, it looks like the missing piece for Testing Lib 😇

If you'd like to give some examples of tests with these issues, we can start by sharing various solutions, and then figure out what we want to recommend in the documentation or blog

I'd really appreciate that – I feel that having real-life examples will help us track down the issue and figure out if it's whether a docs/learning issue or a flaw in the design.

I can provide Vue implementations of these situations, so that we make sure we're also taking async, batched DOM updates into account.

[ITenthusiasm]

I don't think loading states are an implementation detail, but I strongly agree with encouraging people to control the delay via mocks/timers/etc. Hopefully that's easily said and done.

If you'd like to give some examples of tests with these issues, we can start by sharing various solutions, and then figure out what we want to recommend in the documentation or blog

I'm down for some fun problem solving too. I'd feel more comfortable contributing in the React or Vue world though. I've only scanned the other libraries.

[ph-fritsche]

Is there anything left to this that can not be covered by... ?

//... assert something ...
userEvent.anything()
//... assert synchronous state after the event - so anything that is changed right in the event handler ...
waitFor(() => {
//... assert anything happening on asynchronous state updates ...
})

Regarding the real world where a user can not possibly trigger the events synchronously:
What about looking at it the other way around? The asynchronous nature of events means there is no defined order of execution. An execution on every next step in the event loop is per definition not impossible although we know it won't happen.

So the user writing a test could write:

await userEvent.anything()
//... assert anything happening synchronously or asynchronously

While the asynchronous changes might have happened per specs, we know they probably won't have happened.

So the user could write every time:

await userEvent.anything()
await asyncWrapper(async () => await new Promise(res => setTimeout(res, 1000)))

He would end with the same result we started with: No guarantee that the events fired in that order or that timeframe, but it will 100% of the time just work.

[kentcdodds]

I'd have to test things out, but if there's a way to easily write a test for a loading state that disappears on the next tick of the event loop due to a mocked request then I'm satisfied.

[nickserv]

Unfortunately as far as I understand, frameworks like Vue and Angular trigger internal rerenders asyncronously, so I'm not sure if mocking requests would be enough to work around this issue for those frameworks. We could potentially add workarounds specifically for React though, thanks to internal mechanisms like act.

[kentcdodds]

Actually... thinking about it more, I think it could make sense if an HTTP mocking solution had a way to explicitly wait to resolve a request until specified. To borrow from the example given in my ship stopper comment, something like:

const unpause = server.pause()
await userEvent.click(screen.getByLabelText(/search/i))
const waitingForLoading = waitForElementToBeRemoved(() => screen.getByLabelText(/loading/i))
unpause()
await waitingForLoading

I mean, that's a fair amount of boilerplate, but it's not any more implementation detail-specific because we're already mocking requests, so we're already accepting the implementation detail of the fact that requests are fake. Embracing that further by allowing the ability to hold off on resolving responses is fine IMO (it's akin to asserting on mock functions).

I'd be curious what @kettanaito (author and maintainer of MSW) would have to say about an API like this (or maybe it already exists and I just don't know about it!)

[ph-fritsche]

const serverPromise = new Promise(resolve => {
  server.use(rest.post('/endpoint', (req, res, ctx) => {
    resolve()
    return res(ctx.status(500), ctx.json({ message: 'Internal Server Error' }))
  })
})
render(<Component/>)
// ... trigger the request ...
// ... do anything ...
await serverPromise
// ... do assertions ...

[kettanaito]

If I understand the problem correctly, the intended solution is to give the developer control over the loading state, more precisely the means to reliably keep the response as pending (to assert the loading state) and then to resume the response (to assert that the post-loading UI is correct).

Mocking an API takes away some network behaviors, such as server response time. This often results in mocked responses being instantaneous and causes another issue that there isn't a sufficient time frame to assert the loading state. I see this as a good thing, actually, because you should explicitly specify the response delay, thus, being in control over it:

server.use(
  rest.post('/search', (req, res, ctx) => {
    // This mocked response will take 1s to resolve.
    return res(ctx.delay(1000), ctx.json({...})
  })
)

Read more about ctx.delay.

That is a pattern I recommend for MSW users: set an explicit response delay with a sufficient value for the test to assert the loading state. It may even be useful to set an infinite loading, as in response is forever pending during a specific test:

return res(ctx.delay('infinite'))

Since the response delay is set once, you cannot change it amidst the test run. This means you cannot resume a response within a set delay window. That's why I think that the loading state and the success state should be covered in separate test scenarios, each being in precise control over the response time:

afterEach(() => server.resetHandlers())

it('renders a loading indicator while searching', async () => {
  server.use(rest.post('/search', (req, res, ctx) => res(ctx.delay('infinite'))))
  await userEvent.click(screen.getByRole('button', { name: 'Search' }))
  // assert the loading indicator is visible...
})

it('renders the search results', async () => {
  // no augmenting of the previously defined handler (no delay).
  await userEvent.click(screen.getByRole('button', { name: 'Search' }))
  // assert the search results in the UI...
})

Please let me know if I'm going to an entirely different direction here.

[kentcdodds]

Thanks for responding @kettanaito! That's an interesting take. I personally don't like splitting this up into multiple tests and would prefer the ability to tell the server to pause and then resume as mentioned in my previous comment, but I'm willing to be swayed to your way of thinking. I'd be interested to hear what others think.

[kettanaito]

I find both cases applicable. I like separating different UI behaviors even if they contribute to a single UX flow for the sake of decoupling those behaviors. Today I have a loading screen, tomorrow I move the data fetching to the server-side, hydrating the page with data in the initial load. I can safely remove the "loading state" test because I know that's the only thing it asserts.

At the same time, perhaps, it's worth exploring pause/resume network API in MSW. Exploration never hurts and may help us uncover some hidden gems. I'm not sure if pausing the entire server (network) is an expected thing. I'd pretty much like to give this control on a request/response basis, so it feels like a part of the request flow, not an extraneous API that interferes and controls the network.

[ph-fritsche]

@kentcdodds @nickmccurdy Could you have a look at this issue again?

When introducing setup (#743) in v14 we get the chance to shuffle some things around and return different versions of the APIs - versions closer to what we consider ideal without breaking existing code.
So what do you think the API should be? Or should we let the user decide and return either an asynchronous or synchronous version in setup?

(See also #659 (comment))

[kentcdodds]

I think this is a good opportunity to make things async 👍 the loading state testing methods just needs to be documented is all.

[github-actions]

🎉 This issue has been resolved in version 14.0.0-alpha.15 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[pleunv]

I seem to be running into this exact use-case now while attempting to upgrade to v14, in which all tests that verify an intermediate (loading) state now fail. I also don't really have a way to control the network mocking. Is there any documented approach to work around this? The only ways out I see right now is to either stop testing intermediate states (I'd rather not, they need to be verified) or to switch these tests back to fireEvent.

[nandanmen]

@pleunv (and others) I just ran into this problem today and my workaround was to not await userEvent and use a waitFor instead:

const mount = () => {
  render(<Component />);
  return userEvent.setup();
};

it('disables the button', async () => {
  const user = mount();
  user.click(screen.getByRole('button'));
  await waitFor(() => {
    expect(screen.getByRole('button')).toBeDisabled();  
  })
});

[ph-fritsche]

@narendrasss

You should await both. Otherwise the test might leak into following tests.

await Promise.all([
  user.click(element),
  waitFor(() => expect(element)).toBeDisabled(),
])

You should also call userEvent.setup before render. Although this makes no difference in almost all cases - if you hit one of the rare edge cases that might be a non-obvious issue.

[nandanmen]

Appreciate the response @ph-fritsche! Could you elaborate a bit on:

Otherwise the test might leak into following tests.

I understand not await-ing user.click means it might resolve after the test completes, but I'm not sure what the implications are and how it could affect other tests.

[ph-fritsche]

The library might continue to interact with the document after the test is completed.
With the click API and its current implementation this is very unlikely to cause a problem, but...
if you have one bad practice in your code, it's just a matter of time until it finds another equally harmless one to team up against the poor soul who is supposed to debug the resulting mess.

// let's assume React
test('testA', async () => {
  const onChange = jest.fn() // it's obvious which event triggers the handler
  // ... isn't it? https://ph-fritsche.github.io/blog/post/why-userevent
  const { user } = setup(<input onChange={onChange} defaultValue="foo"/>)

  await user.type(screen.getByRole('textbox'), 'bar')
  expect(screen.getByRole('textbox')).toHaveValue('foobar')

  // let's remove the focus from the input field
  user.click(document.body)
  expect(onChange).toBeCalled()

  await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('This is invalid.')
})

// If the `alert` was added on the `blur` event, everything is fine.
// If the `alert` was added earlier, there is a race condition with the pending `mousedown`.

// Add 1000 synchronous tests here...

test('testB', async () => {
  const { user } = setup(<button ref={el => el?.focus()}>This button is automatically focused when mounted</button>)

  // ...any `await` will trigger the bug here...

  expect(screen.getByRole('button')).toHaveFocus() // If this fails, have fun debugging the `button` component.
})

[jagregory]

Where's the documentation that @kentcdodds mentioned about testing loading states? I can't see it anywhere and this feature is already out in the wild.

Is @ph-fritsche's solution the correct way to test loading states?

[nico-madeira-deel]

My team had the same question as @jagregory while updating the dependencies on our project. Does anyone happen to have any news about the documentation? How can I help to deliver that? Thanks beforehand!

[felipecesr]

What if we had a blog post with examples of various ways to deal with these situations, and then made the core events in userEvent (and possibly fireEvent for consistency) async? If we're concerned about back compatibility, we could publish prereleases first.

Another advantage to doing this sooner is that we wouldn't have to make a separate async version of the fireEvent/userEvent API for Vue and Svelte (which internally fire events asyncronously).

I guess I'll have to spend some time thinking about this and determining how hard it is to test those loading states using explicit slow mocks/timers...

If you'd like to give some examples of tests with these issues, we can start by sharing various solutions, and then figure out what we want to recommend in the documentation or blog.

Hi @nickserv! Did you write the blog post? I'd like to see these examples.

[nickserv]

No I don't believe so, but the APIs and docs are now async.