lmiller1990 / vue-testing-handbook

A guide on testing Vue components and applications
https://lmiller1990.github.io/vue-testing-handbook/
876 stars 160 forks source link

Update all tests and exmaples to use latest vue-test-utils (beta 29) #124

Closed kolbma closed 5 years ago

kolbma commented 5 years ago

Hi,

the router.push() call in e.g. https://lmiller1990.github.io/vue-testing-handbook/vue-router.html#writing-the-test is async and returns a Promise.

So the expect afterwards may fail.

Should be something like await router.push().

lmiller1990 commented 5 years ago

Hm, can you tell me what version of vue-test-utils you are running? All the tests described in this guide are currently passing on CI.

There was a breaking change in vue-test-utils regarding sync/async test recently, read more here.

I think we will need to update all tests to use the latest version, and add some notes to let people know.

kolbma commented 5 years ago

Ok. Makes sense. I've just started with Quasar framework and @quasar/quasar-app-extension-testing-unit-jest requires "@vue/test-utils": "^1.0.0-beta.29"

Was a show stopper to catch this as cause of empty tags in my tests. ;-)

lmiller1990 commented 5 years ago

Glad we figured this out. I'm going to update this issue to be "update examples to use vue-test-utils beta-29, and add some notes to make sure all readers are using the latest version.

Thanks!

lmiller1990 commented 5 years ago

Looks like this is not the case - router.push is not async. The test passes even with @vue/test-utils beta 29!

kolbma commented 5 years ago

Come on @lmiller1990 . This is a typical race condition. It is async... look at the code https://github.com/vuejs/vue-router/blob/c0d3376f4e3527bd761bd325873366ed74f5736b/src/index.js#L153

lmiller1990 commented 5 years ago

@kolbma Sure. The test still passes under @vue/test-utils beta 29, though. I also did a console.log of the wrapper.html() - it is indeed updated. Looking more closely at the code you linked:

if (!onComplete && !onAbort && typeof Promise !== 'undefined') {
      return new Promise((resolve, reject) => {
        // This part is executed synchronously, I guess? No need to `await` for the routing to update.
        this.history.push(location, resolve, reject)
      })
    } else {
      this.history.push(location, onComplete, onAbort)
    }
  }

While it does return a Promise, it seems this.history.push is executed immediately (?). I believe you only need to do await if you want to verify or assert against something that occurs in the optional onComplete hook. What do you think? I'm happy to make the change to use await - I 'm just wondering where else I need to update now - from what I understand we need to be using await anywhere that makes a DOM change occur (which is basically every test?)

The beta 29 thread here is quite confusing too - do we need to do await vm.$nexTick() on every DOM change now?

kolbma commented 5 years ago

Hi @lmiller1990, a Promise is never ever running synchronously!

let p = router.push('/');
// The code of the Promise function could be executed here, but need not.

// We only have guarantee that it is executed if we use something like
p.then(() => { console.log('Promise did resolve history.push'); }
// or
await p;
console.log('Promise did also resolve history.push');

What it makes complicated is, there might be environments without Promise-support, and there the else-block of router.push() is executed synchronously and returns nothing.

To await vm.$nextTick():
It is documented at https://vuejs.org/v2/api/#Vue-nextTick

// modify data
vm.msg = 'Hello'
// DOM not updated yet
Vue.nextTick(function () {
  // DOM updated
})

// usage as a promise (2.1.0+, see note below)
Vue.nextTick()
  .then(function () {
    // DOM updated
  })

or

await Vue.nextTick();
// DOM updated
lmiller1990 commented 5 years ago

I understand a Promise is not run synchronously; what I don't fully understand is why this still passes when running yarn test:unit with the latest version of vue test utils. Promises are supported in the env. we are running in (node 10).

If I understood the change log correctly for vue-test-utils beta 28, and what you are saying this test should now be failing (it isn't). Why is it not failing? From what I understand, it should be failing (not a race condition, but consistently failing).

kolbma commented 5 years ago

This doesn't fail consistently. It depends on the JS engine pushing the Promise-function to queue and event loop handles the queue and how the context switching is done. If there is nothing else than the short test, it can be the only possibility to context switch to next. That's why it is "ok" in your unit test, but code is not correct any longer. But if there is more, there is a "race" problem of execution order.

lmiller1990 commented 5 years ago

Ok, that makes sense. I will update this to use await.

I'm still not sure which other test(s) should be updated - based on this understanding, it should be every test that relies on the DOM to rerender.

Edit: we should update all tests to do await vm.nextTick(). See here for a good explanation. Great find @kolbma.

lmiller1990 commented 5 years ago

I have updated the guide to include await wrapper.vm.$nextTick() where required here: https://github.com/lmiller1990/vue-testing-handbook/pull/130