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

Vue Test Utils 2.x API deprecations - Vue 3 support #151

Closed

Conversation

dobromir-hristov
Copy link
Contributor

@dobromir-hristov dobromir-hristov commented Mar 29, 2020

This PR aims to gather feedback on the deprecations we aim for in the new Vue Test Utils release, aimed at the upcoming Vue 3.

This is a complete rewrite in TS and gives the chance to slim down and improve the API and Documentation.

Rendered

Note

These are not final and are subject to change. If enough users give a valid reason, we can adjust the list.

```js
expect(wrapper.find('.notExisting')).toThrow()

const element = wrapper.find('.notExisting') // will throw error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that from a testing perspective this is lacking expressiveness: it's not clear what we are testing when we write expect(wrapper.find('.notExisting')).toThrow(), specially compared to expect(wrapper.find('.notExisting').exists()).toBe(false)

Why do you think throwing an error directly on find makes sense? I find the existing behavior of throwing when using any method that assumes the returned wrapper not to be empty like

let button = wrapper.find('.not-existing-button')
button.classes() // throws

more reasonable. With the given change, the test above does not need any change as it will just throw earlier, but I don't find reasonable having to change

expect(wrapper.find('.notExisting').exists()).toBe(false)

to

expect(() => wrapper.find('.notExisting')).toThrow()

BTW, we do need to create a function wrapper to use toThrow, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This was aimed at merging get and find.

get was added recently, as people decided throwing an error was better and more intuitive, than checking for exists all the time.

The VTU team is a bit uncertain about this.

Copy link
Member

@lmiller1990 lmiller1990 Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can live with keeping get (although I think just keeping find the same as using expect(wrapper.find('.notExisting').exists()).toBe(false) is fine). I don't think figuring out the problem when you call trigger (for example) on a wrapper since find failed is that difficult.


**is -** [Link](https://vue-test-utils.vuejs.org/api/wrapper/#is)

Not that much useful. Use `element.tagName` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about Components that do not have a name, how would it work?

Use custom matcher like [jest-dom](https://github.com/testing-library/jest-dom#tobeempty) on the element

```js
expect(wrapper.element).toBeEmpty()
Copy link
Member

@posva posva Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way we could write expect(wrapper).toBeEmpty()? (same for isVisible)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have to return the DOM node, instead of the VTU wrapper. So from what I saw, no.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But couldn't we make toBeEmpty() read the element property if what's passed is a wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we offer people to use jest-dom, I think no. As that library has no connection to Vue and the VTU at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because jest-dom must be added by the user? I thought that if it's added by VTU, we could override the custom matcher that is added by them

Copy link
Member

@lmiller1990 lmiller1990 Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is isEmpty just basically sugar for expect(wrapper.find('.foo').text())).toBe(null)? Or innerHTML? isEmpty is definitely ambiguous, at least to me - definitely in support of removal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's innerHTML === '' so I find it quite accurate but I personally don't use it often

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Let's see if any others find this method useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way we could write expect(wrapper).toBeEmpty()? (same for isVisible)

we'd need to extend expect to do so. This is the reason jest-dom exists as a standalone package, because it's not related to Vue Test Utils or other testing libraries, but to the test runner (in this case, Jest).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, if we want anything jest specific, it may be better on a different package

If you really need to update something after it mounts, just use the instance

```js
wrapper.vm.field = 'updated field'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think setData makes sense because we can await setData() whereas with wrapper.vm.field = 'updated' we need to do await nextTick() after it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the use cases for arbitrarily updating a component's data?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use case is usually unit testing when the data is set from something that is mocked so it's impossible to trigger but an integration test is too much for the test in question (could be some displaying or anything)

Also interactions that are too hard/verbose to test with clicks and/or changing input's values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of the first case where mocking something makes it impossible to trigger the interaction? I'm not sure I fully understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A router guard that sets data is one example

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a guard can set data inside of it

beforeRouteUpdate(to, from, next) {
  this.things = 'stuff'
  next()
}

but we don't necessarily have a way to call that guard

The clicking x times example works as well, others might be related to global events like scrolling, or other things that depend on a device and that are not always easy to mock. When these native calls change the data of the component, you need a way to do it yourself to simulate the native behavior.

Flexibility in VTU is crucial because people write their code in very different ways and we still want them to be able to test it as long as it's not an antipattern, even when they are not using the best practice. This will make the experience with VTU less frustrating because when given a complicated interaction to recreate, it's still possible to write a test for it.

Since we allow setting data directly into a component, setData makes sense to make the test writing easier by automatically waiting for one tick.

Copy link
Member

@lmiller1990 lmiller1990 Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afontcu for the 100 clicks case....

mount(Foo, {
  data() {
    return { count: 99 }
  }
})

wrapper.find('button').click() // 100 times!

We will keep support for the data mounting option. This is just referring to setData - maybe that wasn't clear. I don't see this as a use case for setData

Re @posva (the guard example) that is an interesting example. I still don't really see how setData solves this problem - you still have no way to call that hook. Is the assumption in this example that setting this.things triggers a watcher to make an API call or something? Can you provide a full example (one .vue file and one .spec.js file) that shows how setData helps with router guards?

Flexibility in VTU is crucial because people write their code in very different ways and we still want them to be able to test it as long as it's not an antipattern, even when they are not using the best practice.

I agree 100%! VTU should let people test the components they write, regardless of style. That's why I'm interesting in a full example for the router guard you gave above, I don't see how setData solves the problem you are describing.

global events like scrolling

I think this is outside of the scope of VTU... can jsdom even scroll? I think it's important to remember VTU is not a one stop solution to all your testing needs, in the same way Vue does not solve every front-end problem - it's just one piece of the puzzle.

Since we allow setting data directly into a component, setData makes sense to make the test writing easier by automatically waiting for one tick

I'm not sure I understand what you are saying here. Normally you would wait for the next tick with await nextTick()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmiller1990 About the 100 click cases, you are over simplifying things:
You are assuming the scenario that needs to be tested only needs the 100 value in data but there might be other things that need to happen in a specific order before setting the value to 100 to be able to test a scenario. This is where the flexibility of setting the data directly is important. The only point of setData is to also return a promise that calls nextTick so we can write

await setData({ thing: 'stuff' })

instead of

wrapper.vm.thing = 'stuff'
await nextTick()

And this is considering we will still be able to do wrapper.vm.thing = 'stuff'

That's why I'm interesting in a full example for the router

I can't share them, it's something I've come by with many clients. The problem appears whenever the component has a side effect that is outside of it (like changing the url) and that side effect trigger a listener or hook inside the component (like a route guard). That hook changes data and that data then has an impact on the rendered template, which is what we assert against. The code inside the guard that matters is setting the data, the rest are side effects like fetching an API. But again, like with the 100 click example, you sometimes need other things to happen before. The problem comes when there is no way to trigger the hook with VTU because it's not a native Vue feature like a watcher.

I think this is outside of the scope of VTU...

Exactly, and that's when we need changing data, just like the router example. The point is not to be able to recreate the scroll and any other case, because that would be impossible. Instead we go around it because we still need to unit test some of the stuff.

To make my point clear: if we allow directly changing data on the instance (which we should because of the points mentioned above and also it was included in VTU for a reason), we should allow a function like setData for it to be consistent with other set* functions that return a call to nextTick.

I don't understand what in the point above, you are trying to discuss here

Copy link
Member

@lmiller1990 lmiller1990 Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for detailed reply @posva . I appreciate your insight.

setData (and other methods) should definitely return nextTick - that would be very useful.

Regarding what I am trying to discuss: I am trying to discuss the necessity of setData (since I do not think it is a useful method). I think some others agreed, that's why we put it in this RFC for a potential deprecation.

Some other people think it is an important API. The main case I see is the one you described relating to things jsdom might not handle as well (like scrolling). For this reason and based on this discussion, I think we should not deprecate it.

I don't see a good way to implement it in Vue 3 yet (I could be missing something), but based on this it seems we need to work something out if this is an important feature to a lot of users.

Copy link
Member

@lmiller1990 lmiller1990 Apr 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone following this discussion, the outcome, based on the above comments, is that setData will continue to be supported.

```
**setProps -** [Link](https://vue-test-utils.vuejs.org/api/wrapper/#setprops)

Overriding props after the component is mounted is a hack, which introduced lots of errors, especially with watchers in VTU Beta.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really unfixable? setProps is really useful in my experience because it allows to make sure a component adapts to changing props while it's mounted.
Having a native API that is convenient, even if it's different from regular mount or shallowMount to expose that behavior with a parent and having direct access to the tested component and not caring about the parent, would be very helpful. It would be better to be able to keep setProps altogether. It also faces the same problem as setData, we need to wait a tick

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I thought (and still think) this API isn't ideal for most use-cases, but there is some occasions where it's useful, so it will continue to be supported.

We found a clean way to implement it and it's available in the current VTU next alpha. The bugs from beta as also mostly solved. You may now do await wrapper.setProps to get the re-render.


Rarely used, no benefits.

**props -** [Link](https://vue-test-utils.vuejs.org/api/wrapper/#props)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes we want to test a component in isolation without testing its children. In this case the component’s boundary ends at the props it is passing its children, so shallow mounting and then calling this method to check the child component props is very useful.

@dobromir-hristov dobromir-hristov changed the title Vue Test Utils 1.x API deprecations - Vue 3 support Vue Test Utils 2.x API deprecations - Vue 3 support Apr 18, 2020
@dobromir-hristov
Copy link
Contributor Author

Closed in favor of a more refined version here #161

Thank you all for the input.

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.

5 participants