Update find and findAll to be more robust

[lmiller1990]

See #651 for details of the goal of this PR.

WIP. Some failing tests to fix, then add some for the new feature.

The general idea I'm going with at the moment:

Change validators/isDomSelector to only work on "real" dom elements. This includes the following cases:

• div, span
• jquery-like selectors "#id"1, ".class"`
• tricky pseudo selectors p:second
• svg selectors text

With that change, isNameSelector will catch any other strings, like MyElement

And add some tests. I am part way through updating isDomSelector, 4 failing tests remain. The approach isn't that nice - anyone has a better way, please let me know. The main complexity is differentiating:

find("div") //=> dom element

find("headerbar") //=> name of component

If we had a hard rule on custom component names to be either camelcase or kebabcase, we could use that logic, however I'm sure some people will have single word components, like <poster>.

We should also render contents of default slot by in mountShallow, this might be okay in a separate PR to keep things easy to review. One of my coworkers is keen on his first contribution, maybe can leave that one for him. First contribution to OSS is always challenging.

[lmiller1990]

Quite ugly but needed to cover all existing test cases:

• find("#id")
• find(".class")
• find("div") html element
• find("text") svg element
• `find("div > p") pseudo selector, although this is prob bad practise in tests anyway
• find("p:first-of-type")

[lmiller1990]

both libs are just json lists. an alternative would be just copy paste the json lists into the project

[38elements]

IMHO,
Since their size is small,
I think it is not necessary to use library.
Vue.js does not use the library as below.
https://github.com/vuejs/vue/blob/f76d16ed9507d4c2a90243ea3d77ccf00df29346/src/platforms/web/util/element.js#L11-L32

[lmiller1990]

yeah, I agree. Just going to grab the list from vuejs/vue repo.

[lmiller1990]

All green! Might have missed some edges cases, but looking good.

[lmiller1990]

@eddyerburgh @38elements Should be okay to review now, CI finished, all green. Thanks.

[eddyerburgh]

Sorry I don't think I made my intentions with this clear.

I don't think this should be an alias for name. I think this should work as a tag selector for components:

const SomeComp = {
   name: 'a-name'
}

const TestComponent = {
  template: '<div><some-comp /></div>',
  components: {SomeComp}
}

const wrapper = mount(TestComponent)
wrapper.find('a-name').exists() // false
wrapper.find('some-comp').exists() // true
wrapper.find('SomeComp').exists() // false
wrapper.find({name: 'a-name'}).exists() // true

As @38elements suggested, we already have a name selector that's explicit.

[lmiller1990]

Maybe I misunderstood. Let me check my new understanding:

Instead of writing the logic where I did (in isNameSelector and vmMatchCtorName) I should have a isTagSelector and vmMatchTag?

If that is the case it’s a matter of moving things around. I’ll wait for your confirmation first. Thanks.

[eddyerburgh]

Yes. Or have an VM_TAG_SELECTOR const, and then use that to get a vm and match its tag.

For instances you need to check the vnode.componentOptions.tag

I'm not sure about functional components, I think vnode.fnOptions

[38elements]

I can not understand this pull request comment and state.

#651 is an issue which is feature request for createStub().
There is another feature in #651.
This is inappropriate.

IMHO,
If createStub() is not plan to be add, #651 should be closed.
If this feature is necessary, New issue for this feature should be made.
I think splitting the implementation of a feature and implementing a part of the feature by another person are inappropriate.

[lmiller1990]

@eddyerburgh Thanks for the clarification, I can work on it tomorrow. It makes sense to not add this logic where I did, but in a isNameSelector function.

@38elements isStub is closed. The "feature" I am suggesting in #651 is to make find and findAll more convenient. I proposed two ideas (see #651 ). Simply:

Proposal 1) add createStub helper (we decided against this)
Proposal 2) allow find to take a string, which is then matched against component tags. For example

<template>
  <hello>
</template>

I want to assert hello is rendered. Currently I have to do

expect(find({ name: "hello" }).exists()).toBe(true)

I think it's a little unintuitive. I need to specify name when really I am asserting a tag, <hello />, is rendered. As you know, in a web browser we can write something like:

document.querySelector("div")

I want to be able to write this style of assertion in vue-test-utils. I also think it is more intuitive for people new to vue and TDD in general - most web developers understand querySelector("#id"), querySelector(".class") and querySelector("div"). We support all of these, so I think makes sense to support find("component-tag") as well.

Once implemented, the API will be like this:

import { shallowMount } from "@vue/test-utils" 

const wrapper = shallowMount(Component)

expect(Component.find("hello").exists()).toBe(true) 

Does that make sense?

There was also another issue discussed, which is rendering the contents of a default slot. That is another issue, #658 , and as you said, should be addressed in a separate PR.

[38elements]

@eddyerburgh @lmiller1990
There are Name Selector and Vue Components Selector.
Would you tell me the feature of this pull request is an essential API or not?

[38elements]

Does that make sense?

No, it does not.
Using mount(), the component tag is expanded.
find("component-tag") returns a Wrapper object, despite there is no component-tag element.
This behavior is weird.
If this feature is not essential, I object to add this feature.

[eddyerburgh]

Ok, we should split this PR out into two issues and continue discussion there:

• render default slot content in stubs
• add tag selector for instances

[38elements]

@eddyerburgh
render default slot content in stubs should be discussed at #658.
add tag selector for instances should be discussed at #681(here).

[lmiller1990]

New PR for this feature is here: #692

Please review and continue discussion there.