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

fix: use flushPromise() instead of waitFor() #283

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hershelh
Copy link

@hershelh hershelh commented Sep 15, 2022

The current fireEvent() uses waitFor() after triggering event handler to ensure that the DOM is updated in a timely manner, passing in an empty function and awaiting it. await waitFor (() => {}) returns a promise that resolves shortly after. Since Vue also uses promises or other microtasks to update the DOM, when the microtask generated by the promise returned by calling waitFor() is executed by the event loop, the microtask of updating the DOM can also be executed. Users only needs to await fireEvent() to ensure the update of the DOM.

This seems very reasonable. But in some cases, such handling can cause some problems, causing the update of the DOM to be performed after the assertion written by the user.

The case is: some component libraries, like vant, encapsulate form that perform form validation before calling the event callback passed by the user when processing the submit event, and their function for validation will use nested promises to do check (usually a combination of promise and Promise.all()), which leads to a problem: when the microtask generated by waitFor() is executed, the microtask of Vue's update DOM has not been executed! So the user's assertion will be executed earlier than the task of updating the DOM, causing the test to fail.

This situation can be reproduced as follows:

const validate = () => {
  return new Promise((resolve) => {
    Promise.resolve().then(() => {
      resolve(1)
    })
  })
}

const submit = () => {
  validate().then(() => {
    // Simulate Vue to generate microtask that update the DOM
    Promise.resolve().then(() => {
      console.log('dom update')
    })
  })
}

const fireEvent = async () => {
  submit() // submit the form
  await Promise.resolve()// simulate `waitFor(() => {})`
}

const myTest = async () => {
  await fireEvent()
  
  console.log('assertion')
}

myTest()

I also provide a reproduction here.

Obviously, the above situation is not in line with the design purpose of fireEvent() and the expectations of users. Users who do not know the reason may wonder whether the implementation of their source code is wrong or whether there is a problem with the test code, but will not think it's a problem with fireEvent() itself, because the docs they've seen tells them that the function is guaranteed to update the dom.

For users, one solution is to use waitFor() to wrap the assertion and wait for the dom to update. This can let the test to pass, but also adds redundant code. It also makes users lose confidence in writing test code, because they can't be sure that the dom will update immediately after they call and await fireEvent().

So I think it is necessary to modify the internal implementation of fireEvent(). I use the flushPromise() exported by vue/test-utils instead of waitFor(). In addition to using promises, flushPromise() calls setImmediate/setTimeout to generate a macroTask that calls the resolve() method. Since the macroTask will be executed after the microTask, this ensures that all microTasks generated after the fireEvent() call, including DOM updates, will be executed before the macroTask. So replacing waitFor() with flushPromises() guarantees that dom updates can be done before the user's assertion.

@hershelh
Copy link
Author

@afontcu Hi! Could you please take a look? Thank you!

@afontcu
Copy link
Member

afontcu commented Sep 20, 2022

@afontcu Hi! Could you please take a look? Thank you!

Hi! I'm afk until October, will take a look then! The PR looks sensible but I think I'll need to take a closer look and probably issue a breaking change.

thank you!

@hershelh
Copy link
Author

Hi! I'm afk until October, will take a look then! The PR looks sensible but I think I'll need to take a closer look and probably issue a breaking change.

thank you!

Well, I see. Thank you very much for your reply! I hope it won't take up your time.

@hershelh
Copy link
Author

hershelh commented Jan 6, 2023

Hi. @afontcu

Excuse me. Do you have time to take a look at this?

@saifahn
Copy link

saifahn commented Jan 26, 2023

I have a question - why not use await findBy... instead of getBy... in the assertions after firing the event(s)?

@hershelh
Copy link
Author

I have a question - why not use await findBy... instead of getBy... in the assertions after firing the event(s)?

I already explained that because the documentation has promised that "await fireEvent() ensures the DOM is properly updated in response to an event in a test", but obviously there are some cases where it can't cover, and that's why we need to use a more safer API to ensure our assertions are called correctly after all micro tasks are completed.

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

Successfully merging this pull request may close these issues.

3 participants