Skip to content

Feature: Allow find and findAll to work with tags #695

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

Closed
lmiller1990 opened this issue Jun 9, 2018 · 24 comments
Closed

Feature: Allow find and findAll to work with tags #695

lmiller1990 opened this issue Jun 9, 2018 · 24 comments

Comments

@lmiller1990
Copy link
Member

lmiller1990 commented Jun 9, 2018

What problem does this feature solve?

More intuitive API.

What does the proposed API look like?

There is an open PR to add this API:

wrapper = shallowMount(ComponentWithChild)

expect(wrapper.find("child-component").exists()).toBe(true)

Which lets users do find("tag") rather than find({ name }) or find(Component).

There is some objection (see #692 and it's previous PR, #681 ). Here is my thoughts why I like this better than the existing APIs:

  1. name API: ({ name: ... }) doesn't feel very intuitive. We allow querySelector like syntax for find, ({ name }) doesn't conform to this. Also, name is not always the same as the tag, and has nothing to do with what is actually rendered, which is what find is used to assert.

  2. component API: find(Component) is okay, but I don't really like having to import every component I want to find to every test. It's just a bunch of boilerplate. It's easy enough to make shallowMount and mount global, in the same way describe is, but this is not an option for components.

The least amount of typing and reduced boilerplate is find({ name }), but I don't like this that much. If the name is different for the component (it shouldn't be, but it happens) I have to go and find the .vue file, and check the name. Then if someone else looks at the test, they will likely have the same thought (we are looking for ({ name: foo-component }), but actually this component only renders <bar-component>).

We can still continue to allow for { name: 'foo' } to be used, as well. I am not proposing to remove and existing feature, simply extend it.

I don't want to drag this out for too long, if people have too strong an opinion we can just drop it. That's my thoughts.

@eddyerburgh
Copy link
Member

I think this is an intuitive way of finding a component, although I'd like to hear what other people think.

@lmiller1990
Copy link
Member Author

lmiller1990 commented Jun 15, 2018

I have a new use case for this. When I stub an external dependency (at work we are using something called vue-flatpickr which wraps a javascript datepicker):

const wrapper = shallowMount(Foo, {
  stubs: {
    flatpickr: true
  }
})

I cannot use { name: 'flatPickr' } to select the component (I guess the name is something else, no way to know). I could go and look into node_modules and see the correct name, but I don't want to look at some external dependency just to be able to assert it was rendered. For now we did

const wrapper = shallowMount(Foo, {
  stubs: {
    flatpickr: "<div id='flat' />"
  }
})

and find("#flat"), which is more than I would like to type. I could use name by doing something like

const stub = name => ({ name, render: h => h("div" })

or something. We proposed this in an issue a while ago as a helper function, but it would nicer to use something like find('flatpickr-stub') which vue-test-utils makes for us.

@eddyerburgh
Copy link
Member

This would also be useful for components like RouterLink that aren't exported by the library that adds them. Currently we include a RouterLinkStub to select a RouterLink, but I don't think this is intuitive:

import { RouterLinkStub } from '@vue/test-utils'

const wrapper = mount(TestComponent, {
  stubs: {
    RouterLink: RouterLinkStub
  }
})

wrapper.find(RouterLinkStub)

If we selected by tag, you could do this:

const wrapper = mount(TestComponent)
wrapper.find('router-link')

@38elements
Copy link
Contributor

Since the case you mentioned exists in only using mount(),
I think using CSS Selector is appropriate.

@lmiller1990
Copy link
Member Author

lmiller1990 commented Jun 24, 2018

RouterLinkStub is another good use case.

@38elements I don't think using a css selector (in the case of RouterLink, it would be find("a")) is appropriate. You then need to know that the component uses <a href> internally - which means you are testing the internals of another component. It is no longer a contained unit test.

If <router-link> changes and uses <div> instead of <a> in a future version, then you test would fail. The objective of stubbing dependencies is so they can change their internals without impacting the public API.

@38elements
Copy link
Contributor

38elements commented Jun 24, 2018

@38elements I don't think using a css selector (in the case of RouterLink, it would be find("a")) is appropriate. You then need to know that the component uses <a href> internally - which means you are testing the internals of another component. It is no longer a contained unit test.

If <router-link> changes and uses <div> instead of <a> in a future version, then you test would fail. The objective of stubbing dependencies is so they can change their internals without impacting the public API.

Passing test is not purpose.
This is your subjective opinion and not reasonable.

@eddyerburgh
Copy link
Member

The RouterLink example is the same with shallowMount or mount

@38elements
Copy link
Contributor

38elements commented Jun 24, 2018

I see.
I misunderstood.
I thought about the inner component which is not exported in other component.
It seems that the case only exists in only using the component provided by only plugin.
I do not think the libraries is many.
I do not think the example is very useful in using shallowMount().
I do not think it's a sufficient reason for adding this feature.
I think using stub is sufficient.

@lmiller1990
Copy link
Member Author

The PR for this has quite a few conflicts. I can update it, but it might be good to make a decision, be it to include this feature (then I'll need to add more tests) or to not include it, and close the PR, instead of just letting the PR sit forever.

@eddyerburgh
Copy link
Member

I think we should implement this, and deprecate constructors as selectors.

There are a few benefits:

  • every rendered component will have a tag name
  • with a tag name you can select external components without a reference (RouterLink)
  • with a tag name you can select external components without knowing there internals
  • we can extend components like in fix: wrap extended child components #840 and still find them with find
  • it is easier to explain: you use the string of the tag name of the rendered instance, vs you use the constructor of the component that you rendered
  • it reduces complexity in code

@38elements
Copy link
Contributor

38elements commented Jul 27, 2018

I think existing the tag name is existing the component name.
It seems that only few people are interested about this feature.

@lmiller1990
Copy link
Member Author

lmiller1990 commented Jul 27, 2018

I think searching by tag name is definitely more intuitive.

One thing I think we need to establish is which find APIs remain and which don't. Using constructors as selectors is the find(Foo) API, so we are left with

  • find({ name: 'foo' })
  • find("my-component")

This should simplify find a lot. The current PR regarding this issue has some groundwork, but if we are removing the constructor option, some existing code can be removed, and finding by tagName should be as simple as:

  1. recursively get the tagName for all children (we have a function that does something similar already
  2. see if (or how many) times it appears in wrapper.html

@38elements
Copy link
Contributor

38elements commented Jul 27, 2018

There is no constructor option in wrapper page.
https://vue-test-utils.vuejs.org/api/wrapper/

@lmiller1990
Copy link
Member Author

lmiller1990 commented Jul 28, 2018

Passing a component matches against the constructor, for example wrapper.find(Foo).

export function vmCtorMatchesSelector (

https://vue-test-utils.vuejs.org/api/#selectors

@eddyerburgh
Copy link
Member

@38elements the big benefit of using tag is to fix the problem of extended child components that will be introduced by this PR—#840.

We shouldn't remove any code in the initial PR, but we could add warnings that the consturctor selector will be deprecated in 1.0.0.

@38elements
Copy link
Contributor

@eddyerburgh
There is the name selector.
I think using name selector is resolved.

@38elements
Copy link
Contributor

There is no constructor option in below selector page.
https://vue-test-utils.vuejs.org/api/#selectors
I think there is no term called "constructor option" in Vue Test Utils.

@eddyerburgh
Copy link
Member

When I say constructor selector, I mean component selector.

@38elements
Copy link
Contributor

I commented to this.
#695 (comment)

@38elements
Copy link
Contributor

@eddyerburgh
If many people need this feature, I think this feature is necessary.
Since you are a maintainer of Vue Test Utils, you can add feature to Vue Test Utils without my agreement.

@lmiller1990
Copy link
Member Author

I'd like to work on this, once we decide it is going to be included!

@eddyerburgh
Copy link
Member

Yes please start working on it. I'd like to release it at the same time as I merge #840

@38elements
Copy link
Contributor

I am very sorry.
I lost interest this issue.
Would you please ignore my opinion in this issue.

@eddyerburgh
Copy link
Member

I'm closing this because this feature would be too complex to implement. We'll continue with the current selector API instead

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

No branches or pull requests

3 participants