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 synchronous behavior #676

Closed
eddyerburgh opened this issue Jun 1, 2018 · 8 comments · Fixed by #1062
Closed

Fix synchronous behavior #676

eddyerburgh opened this issue Jun 1, 2018 · 8 comments · Fixed by #1062

Comments

@eddyerburgh
Copy link
Member

eddyerburgh commented Jun 1, 2018

Before this library is ready for 1.0.0 we need to fix the synchronous behavior.

Context

By default Vue performs DOM updates asynchronously. When we created this library, we decided to make it perform updates synchronously, to improve the readability of tests.

The original way that we implemented synchronous DOM updates was by running the instance render watcher when wrapper method, like trigger or setProps changed instance properties.

There were two issues with that approach. First, if you made a change that occurred outside of Vue Test Utils, you would need to call an update function that ran watchers.

Second, there were bugs that were difficult to solve, such as watchers running when they should not.

In beta.12 we changed the approach by setting all watchers as synchronous. This approach has caused a number of bugs because watchers cannot complete the full lifecycle. Some of these bugs are very difficult to fix.

Solution

I've submitted a PR to Vue core that will allow us to set tests to run synchronously. This will fix all the current bugs with synchronous.

Problem

The problem is that this change will only be available in Vue > 2.6. This library supports all Vue 2.x versions, so we need to make a decision:

Do we continue with the current buggy synchronous behavior, or remove synchronous behavior for Vue < 2.6 and encourage users to use Vue.nextTick/ setTimeout to run tests?

What do people think?

@eddyerburgh eddyerburgh changed the title Fix to synchronous behavior Fix synchronous behavior Jun 1, 2018
@lmiller1990
Copy link
Member

lmiller1990 commented Jun 1, 2018

I think it would be valuable to see a few examples of which tests will need to be modified if the sync behavior is moved. I don't want to write Vue.$nextTick in all my tests, but if is just tests around components with $watch, for example, I think it's okay.

Buggy behavior is never fun to deal with, and since we are still in beta, I think we should work towards where we want the library to be, even if it means a change such as removing the sync behavior. As long as we provide a warning (and possibly a link to what changed, why, and how to fix tests that are failing due to the change) I think it's okay.

@38elements
Copy link
Contributor

38elements commented Jun 2, 2018

I think that it is better to improve sync mounting option and treat its unresolved issues as limitations which is described in document.

Or Providing mock Vue which is added async option by each version and setting alias vue at webpack is resolve this issue.

webpack.test.config.js

// ...
  resolve: {
     alias: {
         vue: '@vue/vue23-for-test-utils'
      }
   }
// ...

@AlbertBrand
Copy link

I'm currently maintaining a large-ish codebase with ~700 vue tests (jest + test utils beta12) and updating to beta 16 caused quite some strange behavior (still investigating if these are sync bugs and if I can make small repro cases). If my codebase is representative for the potential community I would definitively not introduce sync if it's partially broken (even when it's well documented). It will lead to confusion, lots of subtle/strange bugs that need quite an understanding of the internals of test-utils to solve and probably people not adopting/abandoning vue-test-utils. Better to create a v2.0 when Vue 2.6 is released that is not backwards compatible.

@eddyerburgh
Copy link
Member Author

eddyerburgh commented Jun 5, 2018

Yes, I think we should remove sync for all Vue versions < 2.6 when we reach 1.0.0.

We won't release 1.0 until Vue 2.6 is released, so that sync can carry on working as expected for users who upgrade to 2.6.

We can add warnings in the next beta release that sync will be removed to give users time to move tests that rely on synchronous behavior if they are unable to upgrade Vue versions.

There aren't any breaking API changes between Vue 2.x and 2.6, so most users will be able to update to the latest version of Vue and have sync work as expected.

@Samuelfaure
Copy link

Samuelfaure commented Jun 13, 2018

The update broke a number of my tests (about 15 out of 180, with jest) and I have currently no solutions but to make tests less readable or remove them waiting for the synchronous behavior to be coherent.

I'd have appreciated such a major change to wait, but now that it have been implemented, it might be better in the long run to stay with it, even if I'm suffering right now from it.

Although I do appreciate greatly the concept of not having to update() during my tests, maybe re-implementing the possibility to update() temporarily would be useful.

@xkjyeah
Copy link

xkjyeah commented Jul 3, 2018

I feel that this property is dangerous. The component should not be behaving differently in production and in test.

I was already introducing artificial delays in my tests so it doesn't significantly improve readability for me.

@lmiller1990
Copy link
Member

I ran into an issue with this recently in the case of a non sync test. I'll try and get a repro up, but it was something like

<template>
  <Child v-show="shouldShow" />
  <button @click="shouldShow = true"

data () {
  return {
    shouldShow: false

I was asserting find(Child).visible()).toBe(true) after doing trigger('click'), and I had to do setTimeout to get the correct result.

Not sure what was happening, will try and get a minimum repro up (pretty large codebase to pull it out from)

@eddyerburgh
Copy link
Member Author

@lmiller1990 Thanks but there's no need for a reproduction, we have several issues with reproduction related to the current synchronous implementation. They will be fixed when Vue 2.6 is released.

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.

6 participants