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

flushPromises from vue-test-utils must be called multiple times #1163

Closed
vincerubinetti opened this issue Mar 11, 2022 Discussed in #988 · 21 comments
Closed

flushPromises from vue-test-utils must be called multiple times #1163

vincerubinetti opened this issue Mar 11, 2022 Discussed in #988 · 21 comments
Labels
help wanted Extra attention is needed needs:triage Issues that have not been investigated yet.

Comments

@vincerubinetti
Copy link

vincerubinetti commented Mar 11, 2022

Discussed in #988

Please read discussion first for full details.

Originally posted by vincerubinetti November 17, 2021
I have a Vue and vue-test-utils project that I'm trying to use MSW with. I have unit tests for components that call api functions when mounted. Vue exports a function called flushPromises, which is intended to be a simple function you can await to make sure the api calls have returned and the component has finished rendering before continuing with the test.

import { flushPromises, mount } from "@vue/test-utils";

mount(someComponent, mountOptions);

await flushPromises();
await flushPromises(); // <-- need this extra call for MSW to work properly, but not axios-mock-adapter

// some assertions

The component that I'm testing looks like this:

export default defineComponent({
  data() {
    return {
      status: null
    };
  },
  async mounted() {
    this.status = "loading";
    try {
      this.blah = await axios.get(...blah...);
      this.status = null;
    } catch (error) {
      this.status = "error";
    }
  },
});

For some reason, with MSW, I need two flushPromises calls for my tests to pass. The odd thing is, I was using axios-mock-adapter before trying out MSW, and it was working with just one call. Can anyone think of any reason why MSW might be different?

Another funny thing is that MSW is working in my e2e tests in Cypress (setupWorker), but the problem I describe above is happening in my unit tests in Jest (setupServer), which is setup like this:

const server = setupServer(...handlers);
beforeAll(() => server.listen());
afterEach(() => server.resetHandlers());
afterAll(() => server.close());

Minimum reproducible example

Here is a barebones Vue CLI project with jest unit tests and cypress e2e tests set up:
https://github.com/vincerubinetti/msw-test (archived for posterity)

You can run the unit tests with yarn test:unit and the e2e (with gui) tests with yarn test:e2e. This demonstrates the behavior that msw needs two calls to flushPromises whereas axios-mock-adapter only needs one.

You can verify that msw and axios-mock-adapter are both correctly mocking the call by adding expect(wrapper.text()).toMatch("foo"); expect(wrapper.text()).toMatch("bar");.

@vincerubinetti vincerubinetti changed the title Flush promises not working with msw when trying to wait for Vue component to render With msw enabled, flushPromises from vue-test-utils must be called multiple times to wait for component render Mar 11, 2022
@kettanaito
Copy link
Member

kettanaito commented Mar 12, 2022

Hey, @vincerubinetti. Thanks for converting this into an issue.

Could you please remove the use of axios-mock-adapter from the reproduction repository? You must not use any other mocking tools but MSW—we cannot guarantee how those tools will work together. MSW provides all the mocking you need so you don't have to use any additional mocks/providers.

Please, if somebody has time to investigate this I'd be thankful. I don't currently have the capacity to look into this.

@kettanaito kettanaito added needs:triage Issues that have not been investigated yet. help wanted Extra attention is needed labels Mar 12, 2022
@kettanaito kettanaito changed the title With msw enabled, flushPromises from vue-test-utils must be called multiple times to wait for component render flushPromises from vue-test-utils must be called multiple times Mar 12, 2022
@vincerubinetti
Copy link
Author

axios-mock-adapter and msw are completely separated into their own unit tests. There should be no interaction.

However I've removed it anyway by simply yarn remove axios-mock-adapter && rm /tests/unit/axios-mock-adapter.spec.js.

I wanted to keep it in as proof that other solutions worked as expected. Now if you want that you will have to go back in the commit history.

@nirajfu3e
Copy link

@vincerubinetti Couldn't recreate your issue from your repo. It wasn't working even with two flushPromises but I had almost the exact same issue which I fixed using waitFor from vue-testing-library. You can do something similar with setTimeout if you don't want to use it although don't see why you wouldn't since it's just a wrapper.

Here is the complete code that works for me:

import { rest } from "msw";
import { setupServer } from "msw/node";
import App from "@/App.vue";
import {render, screen, waitFor} from '@testing-library/vue'
import '@testing-library/jest-dom'

const handler = rest.get("https://reqres.in/api/users", (req, res, ctx) =>
  res(ctx.status(200), ctx.json({ foo: "bar" }))
);
const server = setupServer(handler);
beforeAll(() => server.listen());
afterEach(() => server.resetHandlers());
afterAll(() => server.close());

test("Double flushPromises", async () => {
  render(App);

  await waitFor(() => {
        expect(screen.getByText('success')).toBeInTheDocument()
    })
});

@vincerubinetti
Copy link
Author

vincerubinetti commented Mar 13, 2022

Couldn't recreate your issue from your repo

What OS are you on? I've tested this on MacOS Monterey and Windows 10 (and Linux on GitHub Actions).

Also I'm using Node v16.

I fixed using waitFor from vue-testing-library

vue-test-utils doesn't seem to have an equivalent to this. I try to stick to libraries' recommended ways of doing things, and vue-test-utils recommends flushPromises.

I could probably write my own waitFor, but I also don't prefer that approach since it's less generic and you have to tie it to DOM state every time you call it.

@kettanaito
Copy link
Member

It's worth noting that it's unlikely that the issue can be solved by increasing the number of times one calls flushPromises. It may solve it as a by-product of some internal logic (I suspect simply be delaying the test execution, being a timeout).

It does look like the issue is in constructing the proper test. Specifically, awaiting asynchronous elements in the test. As vue-test-utils recommends using flushPromises, I expect that function gathers the pending promises on the page and awaits them. As HTTP requests are often represented as promises, that's how you ensure the network state is idle before proceeding with your test's assertions.

I suggest you look into how flushPromises is implemented/intended to work. What exactly does it flush? How does it collect those promises? Does it collect the one(s) from the HTTP requests when using MSW?

@vincerubinetti
Copy link
Author

vincerubinetti commented Mar 13, 2022

I suggest you look into how flushPromises is implemented/intended to work. What exactly does it flush? How does it collect those promises? Does it collect the one(s) from the HTTP requests when using MSW?

I appreciate that you're very busy and have limited time to look into this, but this has already be discussed at length and made clear in the original discussion. You yourself already looked at the implementation of flushPromises: it's literally just new Promise(resolve => window.setTimeout(resolve, 0)).

It may solve it as a by-product of some internal logic.
It does look like the issue is in constructing the proper test.

This insinuates this issue is user error, or a problem with vue-test-utils. This is not so, and I've done everything I can to prove it. I've done a minimal reproducible example with the simplest tests possible -- one using msw (which doesn't work) and one using a similar tool (which works) -- reading and following relevant docs to the letter.

The issue is some peculiarity with timing/promises/something async in msw or one of its dependencies. 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.

I don't think this is super urgent -- it seems like a rare edge case -- but I've already done as much as I can. I've done my due diligence, and it's really up to someone in the msw team to look into.

I tried look at what I think is the relevant code, but I just don't have enough knowledge of the code base or node servers, and I've already spent too much "company time" on a small, hard-to-debug edge-case in one 3rd party library of many.

@nirajfu3e
Copy link

nirajfu3e commented Mar 13, 2022

Couldn't recreate your issue from your repo

What OS are you on? I've tested this on MacOS Monterey and Windows 10 (and Linux on GitHub Actions).

Also I'm using Node v16.

I fixed using waitFor from vue-testing-library

vue-test-utils doesn't seem to have an equivalent to this. I try to stick to libraries' recommended ways of doing things, and vue-test-utils recommends flushPromises.

I could probably write my own waitFor, but I also don't prefer that approach since it's less generic and you have to tie it to DOM state every time you call it.

I tested it on Node v16 and MacOS Monterey. I did notice that the Single flushPromises test failed while Double flushPromises passed. There were two ways I could get it working without additional libraries:

test("Single flushPromises", async () => {
  const wrapper = mount(App);

  await flushPromises();
  await new Promise(res => setTimeout(res, 0))

  expect(wrapper.text()).toMatch("success");
});

test("Double flushPromises", async () => {
  const wrapper = mount(App);

  await flushPromises();
  await new Promise(res => setTimeout(res, 0))

  expect(wrapper.text()).toMatch("success");
});

The other method was to use setTimeout in a slightly different manner:

test("Double flushPromises", async (done) => {
  const wrapper = mount(App);

  setTimeout(async () => {
      expect(wrapper.text()).toMatch("success");
      done()
  }, 50)
});

Both approach are essentially allowing the unresolved promises to run the .then and .finally handlers. I hope that helps in someway!

@vincerubinetti
Copy link
Author

I tested it on Node v16 and MacOS Monterey. I did notice that the Single flushPromises test failed while Double flushPromises passed.

Okay... so... you were able to reproduce? But only on MacOS I guess? You said before both failed. What OS was that?

await flushPromises();
await new Promise(res => setTimeout(res, 0))

Again... these are both equivalent. This is the same as calling flushPromises twice. So you are saying calling flushPromises twice is the solution, but that is the whole premise of the issue.

setTimeout(async () => {
expect(wrapper.text()).toMatch("success");
done()
}, 50)

This is quite hacky. An arbitrary timeout will be fragile/inconsistent, and we shouldn't have to rely on it. Nonetheless, I have also done something like this in my case:

export const flush = async () => {
  await flushPromises(); // per vue-test-utils
  await flushPromises(); // per msw issue #1163
  await new Promise(resolve => window.setTimeout(resolve, 100)); // for "good measure"/safety
};

But I don't like this.

@nirajfu3e
Copy link

I tested it on Node v16 and MacOS Monterey. I did notice that the Single flushPromises test failed while Double flushPromises passed.

Okay... so... you were able to reproduce? But only on MacOS I guess? You said before both failed. What OS was that?

await flushPromises();
await new Promise(res => setTimeout(res, 0))

Again... these are both equivalent. This is the same as calling flushPromises twice. So you are saying calling flushPromises twice is the solution, but that is the whole premise of the issue.

setTimeout(async () => {
expect(wrapper.text()).toMatch("success");
done()
}, 50)

This is quite hacky. An arbitrary timeout will be fragile/inconsistent, and we shouldn't have to rely on it. Nonetheless, I have also done something like this in my case:

export const flush = async () => {
  await flushPromises(); // per vue-test-utils
  await flushPromises(); // per msw issue #1163
  await new Promise(resolve => window.setTimeout(resolve, 100)); // for "good measure"/safety
};

But I don't like this.

I probably made some other error when running the tests. Once I recreated the test again from the repo without any changes, I managed to recreate it on my MacOS.

The solutions are indeed a bit hacky. I tried the flush-promises package and that still has the same issue. So it may well be a bug in msw.

@kettanaito
Copy link
Member

You yourself already looked at the implementation of flushPromises: it's literally just new Promise(resolve => window.setTimeout(resolve, 0)).

Apologies, I cannot keep track of the number of issues I'm looking into, investigating, and solving every day. Things will inevitably repeat, and I will inevitably ask repetitive questions.

If that's indeed what flushPromises does then the issue is more than clear: that is not how you await asynchronous logic in tests (or anywhere, for that matter). You have to use a proper await mechanism. I strongly suggest looking into @testing-library/react and its .findBy* API for inspiration. You may also raise this with the vue-test-utils.

Just as I suspected, including multiple flushPromises does nothing and simply prematurely solves an actual race condition in your test. I encourage you against using multiple calls of that utility.

But why does it work without MSW?

I assume when you say "without MSW" you imply you're using another API mocking library. In either way, it likely works as a false positive. MSW executes much more of the request client logic than an average API mocking library. The benefit of that is more reliable tests. Another benefit is that one correct decision forces you to make correct decisions everywhere across your stack. This seems like the case. Because MSW constructs and performs requests similar to your production app, it may include actual asynchronous code (instead of common mocked requests that resolve immediately)—the code you must properly await in your tests.

What we are discussing at length here is the way to correctly write test assertions for asynchronous code. This is a general practice and it is outside of this library's scope.

If you happen to find an issue in MSW, please don't hesitate to post it in this thread, I'll gladly look into it. Until then, the issue is closed.

@kettanaito
Copy link
Member

To give insights as to why I find flushPromises insufficient, it's effectively an alias for setImmediate. Awaiting a 0ms promise means that the event loop skips one iteration and gets resolved in the next one. This is often used to orchestrate callbacks in Node.js but it's not the way to await anything. In fact, it's not about awaiting at all, it's about breaking out of the current event loop step.

Proper awaiting in tests is often focused around assertions (flushPromises is a post-action step, given a conventional setup/action/assertion test structure). Because asserted elements are those you actually wish to await.

test('fetches user' async () => {
  render(<VueComponent />)

  await waitFor(() => {
    expect(ui.getByText('Hello world')).toBeVisible()
  })
})

I understand that different frameworks come with different sets of ecosystem tools and general recommendations in regards to various things. Constructing tests may be one of those things. Yet awaiting asynchronous operations is a general pattern that lies outside of any particular framework, and it is not achieved by skipping an event loop iteration.

@vincerubinetti
Copy link
Author

and simply prematurely solves an actual race condition

For a race condition, it has been extraordinarily consistent, across multiple OS's, new/old devices, and 100+ runs.

I asked the vue-test-utils folks about this. See vuejs/test-utils#137 I also looked into msw's compiled code, and it seems like every request is behind a setTimeout(func, 0) promise (or more, if using the delay() feature), which is basically like one instance of flushPromises. As such, their explanation for why you need two flushPromises makes more sense to me.

I assume when you say "without MSW" you imply you're using another API mocking library.

Again yes, axios-mock-adapter. Perhaps that library is mocking things synchronously somehow, leading to the difference.


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.

Ultimately it seems to come down to this. I'm begging for msw and vue-test-utils to just add a simple note somewhere. One sentence could've saved me hours and hours of discussion, investigating, making min-reproductions, etc.

I can see that you disagree with vue-test-utils's approach, but it is the default testing library for vue-cli projects (equivalent to the ubiquitous create-react-app). In other words, a ton of people use it. I would really appreciate it if you could just add a simple warning about using vue-test-utils with msw.

@kettanaito
Copy link
Member

Again yes, axios-mock-adapter. Perhaps that library is mocking things synchronously somehow, leading to the difference.

Yes, I think that's the case.

I've answered you at length in the linked issue but let me summarize briefly: I'm not against adding mentions to the documentation. But the mention here is not the solution. So instead of looking for a solution, you're just rushing me into documenting workarounds, which will confuse even more people.

I don't see anything wrong with MSW constructing a Promise around the response resolution logic. If anything, that is a natural thing to do to emphasize the asynchronous nature of the response. But what I do see is that vue-test-utils assumes any promise resolves in the next tick, which is a rather error-prone assumption on its own.

Incompatibility with other tools is not a reason to conclude there's an issue. Somehow that extra Promise has no effect on all requests clients and many testing frameworks I've tried. I assume vue-test-utils is the surface to address this simply by employing the elimination method.

@vincerubinetti
Copy link
Author

vincerubinetti commented Mar 15, 2022

Incompatibility with other tools is not a reason to conclude there's an issue.

I disagree. Your library is not implemented in the same way as other similar libraries, as that other thread pointed out. Consistency with developer expectations is something that should be considered, and executed when possible. I think most devs would expect there to be no promise wrapper if the delay is 0.

And yes, I agree, vue-test-utils "solution" of using flushPromises is an over-simplified and fragile one that makes assumptions about third party implementation details.

But still, I don't see that you lose barely anything by just conforming to other mocking libraries and not promise-ifying when delay is 0. But I concede that may unexpectedly break things for other people using msw.

Regardless, thanks for taking all the time to respond, though I wish you were more receptive to suggestion as well. vue-test-utils adding documentation will be a big help, and I agree the 2 flushPromises calls belongs in their documentation, not yours. Still, you should consider adding a generic note like "MSW requests are always wrapped in a promise, which may cause issues for testing libraries that rely on mocking libraries that immediately return."

I suspect I still wont change your mind though, and it's your library so it's fine. The vue people will save other people's headaches with their doc updates.


Edit: Revisiting this a year later. I think I pulled my punches too much. I'm shocked and dismayed by the attitude and arguments by the maintainer.

vue-test-utils must not care if a LIBRARY_A wraps a request in a promise

Very much disagree. But let's assume this is true. Then the corollary is that msw must not wrap something in a promise when there is no reason to.

If anything, that is a natural thing to do to emphasize the asynchronous nature of the response.

This is wishy washy at best. The response is not asynchronous (unless the user is using delay()) because it's not a real network request.

Would you consider it acceptable if some math library randomly wrapped an add() function in a promise? Imagine if its maintainer said "you must not make assumptions about our implementation details" and then you had to sleep for some arbitrary ms, or change your approach to using the function completely.

This is still objectively a bug and the only valid reason to keep it here is to not break existing dependent code. If that's why you wont change it then fine. If it's something else, then in my view you are being a stereotypically inflexible maintainer.

You shouldn't rely on third-party packages to flush promises for you. That brings too much implementation details to your tests and you start focusing on the wrong thing.

It is not for you to impose your purism on how other people write tests, especially so forcefully and dismissively.

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.

  1. It's not a hack, it's a reasonable expectation and thus a bug.
  2. Maybe that's not how YOU develop software, but software is supposed to be a collaborative effort with open minds. Everything in these two issues suggests to me that you do not have an open mind.

I have never used such forceful language in a GitHub issue before, and I do not like doing so, but I feel I have to. I feel that you are being forceful and not listening. I don't maintain any project this big, but my biggest has a few hundred stars though, and I would be ashamed if I acted this way in response to a user issue.

@vincerubinetti
Copy link
Author

vincerubinetti commented Mar 15, 2022

Summary, if I may, for anyone stumbling upon this in the future:

  • vue-test-utils implements flushPromises as a dead simple way to "wait for external promises to resolve"
  • but flushPromises makes a fairly important assumption about the external promises
  • most api mocking libraries conform to this assumption, but msw does not
  • it is not a race condition. given the current msw implementation, precisely 2 calls are needed.
  • msw dev will not change their implementation to conform (possibly justified if it will cause unexpected breaking changes)
  • vue-test-utils devs will add documentation update with a warning about vue-test-utils + msw

Feel free to edit this comment if any of this is incorrect.

@fazulk
Copy link

fazulk commented Mar 29, 2022

Something to note here- If you use flushPromises twice, it works, but if you introduce the msw feature ctx.delay(number) the test breaks. To get us up and running were using waitFor from vue-test-utils.

@kettanaito
Copy link
Member

I've just written a new Vue + MSW integration example and I highly recommend anybody reading this to use wait-for-expect to handle asynchronous code. You shouldn't rely on third-party packages to flush promises for you. That brings too much implementation details to your tests and you start focusing on the wrong thing.

import waitFor from 'wait-for-expect'

it('my test', async () => {
  await waitFor(() => {
    expect(someAsyncState).toBe(expectedResult)
  })
})

@adnan-razzaq
Copy link

any good solution for this problem?

@vincerubinetti
Copy link
Author

any good solution for this problem?

I wouldn't call them "good", but you can patch your node_modules with this fix or you can add two flushPromises calls and it should work.

@MnHung
Copy link

MnHung commented Dec 19, 2023

any good solution for this problem?

hihi, I came to here because of a scenario:

Testing GraphQL Queries with React Hooks and MSW

In this scenario, I have a React hook that queries data using GraphQL and is mocked with MSW. When testing with the renderHook function from @testing-library/react, I encountered issues with asynchronous behavior, especially when trying to add multiple tests based on data from MSW.

The Challenge:

While using flushPromises can work in some cases, it may not always be sufficient due to the unpredictable nature of asynchronous operations. In particular, it becomes challenging to determine the exact number of flushPromises calls needed.

let's see an example:

describe('given a `useTodosData` hook to query TODOs data, and tested with renderHook', {
  let result: {
    current: ITodo;
  };
  beforeEach(() => {
    const utils = renderHook(() => useTodosData());
    result = utils.result;
  });
});

then I can use waitFor and expect the hook return data from MSW

test('it should provide TODOs data from MSW', async () => {
  await waitFor(() => expect(result.current.data).toEqual(DATA_FROM_MSW));
});

but when I want to add more tests based on data from MSW, one flushPromises calls not always works, actually I cannot know how many flushPromises calls will be enough.

describe('when go to next tick', () => {
  beforeEach(async () => {
     await flushPromises();
  });
  test('then it should provide TODOs data from MSW, but not really', () => {
     expect(result.current.data).toEqual(DATA_FROM_MSW);
  });

 // add more tests
});

The Solution:

To address this, I introduced a hookHasChanged utility function. This function waits until the hook's state has changed, ensuring that our tests proceed only when the hook function has completed its asynchronous operations.

const hookHasChanged = async <
  T extends {
    current: unknown;
  },
>(
  result: T,
  timeLimit = 1000,
) => {
  const now = Date.now();
  const currentValue = result.current;
  while (Date.now() - now < timeLimit) {
    await flushPromises();
    if (result.current !== currentValue) {
      return true;
    }
  }
  return false;
};

Usage Example:

describe('when hook function has done', () => {
  beforeEach(async () => {
    await hookHasChanged(result, 500);
  });
  
  test('then it should provide TODOs data from MSW', () => {
    expect(result.current.data).toEqual(DATA_FROM_MSW);
  });
  
  // Additional tests can now be added seamlessly.
});

Note:

  • for safer, I added a time limit and let overtime test fail.
  • this solution works with testing-library/react v14

@MnHung
Copy link

MnHung commented Jan 3, 2024

please do not use the above code,

later I come out a new version, added a field so that we can wait for the specific field in the hook result changed:

type Options = {
  field?: string;
  timeLimit?: number;
};
export const hookHasChanged = async <
  T extends {
    current: any;
  },
>(
  result: T,
  option?: Options,
) => {
  const now = Date.now();
  const currentValue = result.current;
  const { field, timeLimit = 1000 } = option || {};
  if (field && !(field in result.current)) {
    throw new Error(`field [${field}] not found in result.current`);
  }
  while (Date.now() - now < timeLimit) {
    await delay();
    if (result.current !== currentValue) {
      if (!field) {
        return true;
      }
      if (!isEqual(result.current[field], currentValue[field])) {
        return true;
      } else {
        continue;
      }
    }
  }
  return false;
};

example:

  beforeEach(async () => {
    await hookHasChanged(result, { field: 'some-field-in-hook-return-value' });
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed needs:triage Issues that have not been investigated yet.
Projects
None yet
Development

No branches or pull requests

6 participants