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

Helpers: nextTick/flushPromises #137

Closed
afontcu opened this issue Jun 4, 2020 · 28 comments · Fixed by #157
Closed

Helpers: nextTick/flushPromises #137

afontcu opened this issue Jun 4, 2020 · 28 comments · Fixed by #157

Comments

@afontcu
Copy link
Member

afontcu commented Jun 4, 2020

Follwing up on a discussion on Discord, I wanted to open this issue to gather feedback, experiences, and opinions on how to handle async testing and the usage of nextTick and flushPromises.

Differences between nextTick and flushPromises

I couldn't explain it any better than here: testing-library/react-testing-library#11

Current state of the art

We're currently returning nextTick on several methods.

I think I was expecting to get away with await wrapper.something() most of the time.

by @cexbrayat. Turns out you might need an indeterminate number of nextTick calls to get everything up to date, so you end up using flushPromises. Happened the same to me while writing docs for HTTP requests.

Also, there's this question by @JessicaSachs that is worth checking/considering:

nextTick might safely be able to be the default? I don't remember if this prohibits testing of async created hooks (I'm thinking in Vue 2)

What are the alternatives?

1. "automagically" call flushPromises

The pros is that users have one less thing to worry about – and a thing which is quite low-level.

On the other hand, we might lose the ability to test intermediate states (such as "Loading states") before the promise resolved/rejected. If that's the case, this is a no-go.

2. Leave it as is and rely on docs

Pros is that current implementation of VTU-next would work.

Cons is that we all know that explaining something in a documentation does not translate well with "our users will know about this and will take it into account" 😅

3. Create a tool (method? helper?) to wrap nextTick and flushPromises

Not sure about the shape of that tool, but something along the lines of waitFor. Other alternatives mentioned on Discord are wrapper.findAsync.


disclaimer: this issue is meant to serve as a place to discuss alternatives. After that, if we reach a consensus, I open to write a proper RFC with the suggested proposal 👍

@lmiller1990
Copy link
Member

I'm still thinking about this before I formulate my answer. My instinct is the "magic" is not ideal, unless it works 100% of the time.

I'm leaning towards a combination of (2) and (3). We can keep the existing behavior (no breaking change) but include some useful tools or additional methods to make things a bit better. Like how Vue is keeping the options API, but also giving us the Composition API.

I would like to explore something along the lines of waitFor. This seems ideal; no breaking change, and a nice, clean abstraction.

I wonder if @JessicaSachs has any ideas - she did a deep dive and explored a bunch of different frameworks as part of a project at Cypress.

@lmiller1990
Copy link
Member

I will also post this in the Testing channel in Vue land.

@lmiller1990
Copy link
Member

lmiller1990 commented Jun 5, 2020

For some perspective I just did something similar in testing-library and it looks like this:

// wait for appearance
await waitFor(() => {
  expect(getByText('lorem ipsum')).toBeInTheDocument()
})

// or even:
// Wait until the mock function has been called once.
await waitFor(() => expect(mockAPI).toHaveBeenCalledTimes(1))

// Also "findBy" queries retry until they find the element until timeout
const text = await findByText('lorem ipsum')

It still somewhat leaks implementation details - I needed to know that clicking delete course would trigger an async request.

I could be writing this un-idiomatically for testing-library.

@victortwc
Copy link

Would it make sense for the wrapper to expose an API so we can await when all promises are flushed?

e.g

wrapper.find(".button").trigger("click")

await wrapper.asyncDone() // or await wrapper.stable()

expect(getByText('lorem ipsum')).toBeInTheDocument()

@lmiller1990
Copy link
Member

Interesting - would this basically just be something like

wrapper.asyncDone = async function() {
  return Promise.all([ nextTick, flushPromises ])
}

Or something along these lines?

Writing something like this using the plugin system would be super easy.

@lmiller1990
Copy link
Member

lmiller1990 commented Jul 14, 2020

I think the simplest solution without making significant changes would just be to re-export a simple flushPromises function from VTU. What does everyone think? import { flushPromises } from '@vue/test-utils? Rather than let this issue stay open forever, let's make a call and stick with it.

Alternatively, we leave it as-is and just document it 🤷‍♂️ I find myself using flush-promises a lot, even in other tests frameworks. We should include this in the docs, it's mentioned here and a few other places.

@afontcu
Copy link
Member Author

afontcu commented Jul 15, 2020

I'm in favor of reexporting given that flushPromises is quite a simple utility function so keeping it in sync won't be a trouble.

@vincerubinetti
Copy link

Brining this up here rather than creating a new issue since this one seems relevant and seems to include the library authors.

The author of the msw library (library for mocking api calls) asserts that flushPromises is not a robust way to await external async actions. See this issue here:

mswjs/msw#1163 (comment)

Does anyone have any thoughts about this?

vue-test-utils doesn't seem to have an equivalent to waitFor from testing-library/vue. Is there a workaround or a feature request issue for that?

@tlebel
Copy link

tlebel commented Mar 15, 2022

Does anyone have any thoughts about this?

vue-test-utils doesn't seem to have an equivalent to waitFor from testing-library/vue. Is there a workaround or a feature request issue for that?

Interesting question! Here are my two cents on this :)

I've never had the need for anything else than await $nextTick or await flushPromise with vue-test-utils – yet I never used msw.

First, we must understand that flushPromise works by forcing a macrotask loop cycle that will mandatorily come after the microtask is exhausted – and promises are all executed in the microtask queue. See https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

Then in this case, seems you have to call flushPromise twice with msw because: 1) you flush a promise in msw 2) you flush your promise in mounted hook.

I guess you'd have no flushPromise to do if the call to api was a function called on demand (as opposed to call made in vue lifecycle event), eg.:

async fetch() {
    this.status = "loading";
    try {
      this.blah = await axios.get(...blah...);
      this.status = null;
    } catch (error) {
      this.status = "error";
    }
  },

then

await fetch() 
// no flushPromise required 
expect(...).toBe( .... ) 

Given it's in created/mounted hooks, there is probably no escaping the double flushPromise with msw.

And since vue lifecycle hooks are non-blocking (ie making them async don't block execution of the next life cycle step) I don't see how we could even have a waitFor function since it needs something to wait for. When mounting a vue component, I don't see how this would be possible.

@vincerubinetti
Copy link

  1. you flush a promise in msw

Interesting. Were you just making an assumption here or did you look at the msw code? Because I looked at the compiled msw code and did indeed find that every request is essentially behind a setTimeout(func, 0) promise, which is essentially a flushPromises. So that makes sense.

The msw author seemed to think needing 2 flushPromises was just a coincidence and due to race conditions... but for a race condition, it was extraordinarily consistent (testing on multiple OS's, a variety of new/old machines, 100+ times). Your explanation makes more sense.


it needs something to wait for. When mounting a vue component, I don't see how this would be possible.

This is a tangent/separate issue, but... I don't get this. To be clear, I'm referring to these features from testing-library. If they're able to do it with Vue, why can't vue-test-utils? Doesn't that feature simply periodically check any kind of assertion and wait for it to be true (with a max timeout of course)?

@tlebel
Copy link

tlebel commented Mar 15, 2022

Interesting. Were you just making an assumption here or did you look at the msw code? Because I looked at the compiled msw code and did indeed find that every request is essentially behind a setTimeout(func, 0) promise, which is essentially a flushPromises. So that makes sense.

Assumption, from making a lot of tests with mocks 😅 I sometimes had to call flushPromise twice for similar scenarios. Usually, I try to refactor things to avoid that. Or comment on why I can't!

The msw author seemed to think needing 2 flushPromises was just a coincidence and due to race conditions... but for a race condition, it was extraordinarily consistent (testing on multiple OS's, a variety of new/old machines, 100+ times). Your explanation makes more sense.

I don't think it's not a coincidence 😅

it needs something to wait for. When mounting a vue component, I don't see how this would be possible.

This is a tangent/separate issue, but... I don't get this. To be clear, I'm referring to these features from testing-library. If they're able to do it with Vue, why can't vue-test-utils? Doesn't that feature simply periodically check any kind of assertion and wait for it to be true (with a max timeout of course)?

Testing library have a slightly different philosophy on tests, where the focus is on the DOM (and I agree that makes lot of sense). They do wrapper for common frontend frameworks (eg.: vue, react) that will use vue-test-utils under the hood but expose a different set of tool aimed at inspecting DOM. And when you focus on DOM you don't have a choice but developing those kind utility functions. It's a close step to e2e testing but without loading all the things in a browser.

See philosophy here:

If it relates to rendering components, then it should deal with DOM nodes rather than component instances, and it should not encourage dealing with component instances.

@vincerubinetti
Copy link

Testing library have a slightly different philosophy on tests,

I don't think it's not a coincidence 😅

So... two flushPromises working is maybe a coincidence, and thus not reliable... and vue-test-utils wont implement a function similar to waitFor due to philosophy differences...

So what is the recommended solution for this msw issue then? Use an arbitrary timeout and hope that msw has mocked the api call by then?

FWIW I don't really like the idea of waitFor either, as it seems to more tightly couple your test with specific implementation details (e.g. waiting for a div with a certain class name to appear to check that an external api mocking library has done its task).

@tlebel
Copy link

tlebel commented Mar 15, 2022

So... two flushPromises working is maybe a coincidence, and thus not reliable... and vue-test-utils wont implement a function similar to waitFor due to philosophy differences...

Just to be clear: I'm not speaking for vue-test-utils team here!
I was following this issue because it's a thing I've encountered a lot and was interested in.

And about the philosophy: we have to keep in mind that there is no testing-library without vue-test-utils since it relies on it. It's just that testing-library went a step further than offering the essentials you need to run unit tests with vue.

FWIW I don't really like the idea of waitFor either, as it seems to more tightly couple your test with specific implementation details (e.g. waiting for a div with a certain class name to appear to check that an external api mocking library has done its task).

Yeah, you can't do just those type of tests. I see it more as an additional tool in your unit testing tool belt. Each tools has its uses.

@vincerubinetti
Copy link

Sorry, I thought you were a maintainer. That was the other person commenting above you.

So I guess maybe the "solution" is to also install testing-library... just to use the waitFor function. That seems to be what the msw dev thinks is the right way to do things. But honestly I'm not going to install yet another package just for asserting api calls. I think at this point I'd honestly rather just throw up my hands and do an arbitrary timeout.

@cexbrayat
Copy link
Member

Thanks for the follow-up on this.

The thing is flushPromises works well enough, except for msw. Maybe we just need a better flushPromises? We just re-export the one provided by flush-promises as it does a fairly good job. Except if you want to use msw (which you don't have to, as you can use other libs, or manually mock the API call).

But are there any alternatives to flushPromises() out there that would be better?

@vincerubinetti
Copy link

Having to call flushPromises twice wouldn't be a big deal as long as it was noted, and as long as we're sure that will always work, and isn't just a coincidence/race condition.

It'd be really nice for someone with expert understanding of javascript async/await/event loop (definitely not me) to chime in and concur with msw dev's assessment that it's a race condition. I'm personally still not convinced. See my comments above. I'm curious if the issue would still happen if msw immediately returned the mocked response on these lines of code instead of always returning it after a setTimeout 0.

@cexbrayat
Copy link
Member

You can probably manually patch msw in your node_modules to experiment if that changes something.

But I tend to agree that msw probably does something a bit extraordinary as this issue only arises with this library...

@vincerubinetti
Copy link

vincerubinetti commented Mar 15, 2022

You read my mind, was doing that as you commented. And I was able to. The local file I patched was this one:

image

As the file is, I needed two flushPromises for my tests to pass.

After I patched the file to simply return transformedResponse immediately, rather than new Promise(resolve => window.setTimeout(() => resolve(transformedResponse), 0), I only needed one flushPromises for my tests to pass.

I also did some conspicuous console.logs from the file to ensure that was the node_modules/msw file being run from jest.

Idk man, this really seems like an msw bug (it should not promisify the return if delay is 0), and not a race-condition. Anyone willing to concur? The msw dev seems annoyed with me at this point and resistant to considering it an msw bug. If someone else besides them says it's a race condition and I'm just an idiot, I'll finally shut up and just do my hacky solution.

edit: @kettanaito please look. I can show you the exact modifications I made to the compiled msw code.

@tlebel
Copy link

tlebel commented Mar 15, 2022

Idk man, this really seems like an msw bug (it should not promisify the return if delay is 0), and not a race-condition. Anyone willing to concur? The msw dev seems annoyed with me at this point and resistant to considering it an msw bug. If someone else besides them says it's a race condition and I'm just an idiot, I'll finally shut up and just do my hacky solution.

In my book you are absolutely right. It comes down in understanding micro vs macro tasks in JS:
https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

Goes back to:

Then in this case, seems you have to call flushPromise twice with msw because: 1) you flush a promise in msw 2) you flush your promise in mounted hook.

In more precise terms, your first flushPromise flushes this Promise, and the second flushes your axios call (this.blah = await axios.get(...blah...);)

EDIT: Important note though – This Promise is certainly there for a reason I can't comment on. I have not dug into that code. We'd have to explore why it's there. I know msw is popular in the world of storybook. There, from my experience with storybook, it's quite possible it is required. I've had to use setTimeout in some scenarios with storybook in the past.

@vincerubinetti
Copy link

vincerubinetti commented Mar 15, 2022

EDIT: Important note though – This Promise is certainly there for a reason

Yeah, that's the same promise I'm talking about removing if the delay is 0.

If msw cannot remove that without breaking things, then my comment I made before is applicable:

It may be that the way msw is implemented is inherently incompatible with flushing promises in this way, and if that's the case it should be noted somewhere.

And I'm begging msw and or vue-test-util to just make a note of this somewhere that two flushPromises are required.

Edit: That promise might just be there to support msw's delay function though. Hoping it's not actually needed when delay is 0, and that the promise is just there in both cases (0 or > 0) for consistency/cleaner code.

@kettanaito
Copy link

This promise is implemented to support custom delays. It doesn't matter if we remove it or not, the issue would still be there for people using custom delays. For consistency's sake, I'm keeping that Promise wrapper for both delayed and not delayed responses. HTTP requests as asynchronous in nature so there should be no difference whether MSW wraps them in Promises or not—you are not interacting with those promises directly.

I'd love to find a proper solution for this. Managing internal library details as well as including an arbitrary number of flushPromises is not a proper solution.

@cexbrayat
Copy link
Member

@kettanaito If people are using custom delays then they are probably ready to have some kind of mechanism to wait for the delay in their tests no? But it is surprising to have to do the same when you're writing a unit test with no delay in my opinion.

It looks like similar libraries like axios-mock-adapter use a timeout only if a delay is present, otherwise, they return directly https://github.com/ctimmerm/axios-mock-adapter/blob/master/src/utils.js#L117 Others like jest-mock-axios do not bother handling a custom delay.

I can totally understand that you don't want to change your implementation. In that case we can amend the documentation to explain that people using msw need 2 flushPromises.

@kettanaito
Copy link

If you're performing an asynchronous action (and HTTP request is an async action), you must await it regardless if it's delayed by third-party means. The fact that conventional mocking tools introduce a black hole in the place of the request client call, which results in an immediate resolution of any request promise, doesn't mean that's the correct approach. Skipping an event loop cycle is not the way to await asynchronous code, neither it is sensible to assume such code is going to execute at any particular cycle at all.

In that case we can amend the documentation to explain that people using msw need 2 flushPromises.

MSW has nothing to do with you requiring two flushPromises calls. The function itself is a prerequisite of a third-party tool (vue-test-utils) and thus this documentation belongs in that tool, if at all. We work hard to remain library- and framework-agnostic, which includes documentation. We won't include general nature instructions around how to structure your tests or how to await for asynchronous logic—that knowledge is implied and lies outside of the library's scope.

I'm open to addressing this on the library's side if somebody is able to explain how an internal detail (a Promise we construct) is a responsibility of third-party tools. If we can actually analyze the problem and find the solution, and not a workaround, it will be published in the next release.

@kettanaito
Copy link

And yet there is a difference. And several people in this thread seem to disagree with you.

I'm not speaking of behavioral differences. It's clear to me that MSW doesn't play nicely with vue-test-utils. I'm referring to a third-party tooling depending on implementation details of other tooling. vue-test-utils must not care if a LIBRARY_A wraps a request in a promise, or in a dozen of promises.

Why though. If it's not satisfying some necessary requirement, this is just stubbornness.

You must understand my stubbornness then. The code we add to the library is going to be used by hundreds of thousands of people. Yes, I'm particularly stuborn and picky when it comes to any change because I will be maintaining that change for an extended period of time. That alone is a sufficient reason to refuse any workarounds and hacks in order to satisfy particular use cases. That is not how software is built, generally.

@cexbrayat
Copy link
Member

Maybe my comment was misleading, I was talking about our (VTU) documentation.

As I said, it's ok if you have a strong opinion on how that needs to be handled in your library: the point is the setTimeout with a 0 delay triggers the need for a second flushPromises, and other libs made different choices making it unnecessary. I understand that as a maintainer you don't want to change that and introduce potential breaking changes (it's the same for in VTU). We just need to inform developers using msw with VTU that a workaround exists in our docs. If anyone wants to open a PR on VTU docs, we'll gladly review it 👍

@kettanaito
Copy link

How can you guarantee that there are no more pending promises on the second tick? In other words, if we introduce an extra Promise on top, now people will need three flushPromises calls. I think that is an issue indicating that flushPromises either assumes too much or does not fulfill its specification.

May I ask what benefit flushPromises brings compared to something like waitFor?

asyncCode()
await waitFor(() => uiElement)

Note that this is not a matter of stylistic choice. I find the waitFor pattern much beneficial to developers as they are forced to await user-facing logic and not internal logic (like what promises are present, flushed, or pending—things the user, for whom we are writing tests, doesn't care about).

@cexbrayat
Copy link
Member

We don't guarantee anything: in practice, as most mocking libs have just one promise to return the result, developers rely on flushPromises, and that works fine. Same if you manually mock: you'll usually use Promise.resolve(). The only issues we had about that are people trying to use msw: they just need to know that flushPromises is needed twice in that case, or that they can use other mocking libs. Maybe the practice is different in other ecosystems. The other one I know well is Angular, where you mock API calls in unit tests by using an Observable that returns immediately as well.

waitFor is possible and is offered by some higher-level testing libraries that are built on top of VTU, and developers are more than welcome to use them.

But most of us do not need it, as we want to mock API calls without delays and return the response directly, without having to wait for anything because that's not what we're trying to test.

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 16, 2022

We would probably include waitFor if Vue Testing Library didn't already do that (which extends and builds on top of Test Utils, like React Testing Library builds on @react/test-utils. In general, I think Testing Library with the waitFor method is probably a more ergonomic alternative to Vue Test Utils for cases like this.

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

Successfully merging a pull request may close this issue.

7 participants