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

findAllComponents() returning the same component twice? #689

Closed
Giwayume opened this issue Jun 23, 2021 · 14 comments · Fixed by #896
Closed

findAllComponents() returning the same component twice? #689

Giwayume opened this issue Jun 23, 2021 · 14 comments · Fixed by #896

Comments

@Giwayume
Copy link

Giwayume commented Jun 23, 2021

Is there a reproduction template for https://codesandbox.io? I'm having trouble finding one.

My component looks something like this:

<template>
    <component1 v-for="item in array" :key="item+ '_item'">
        <component2 data-some-attr="attrvalue" />
    </component1>
</template>
<script>
return {
    data() {
        return {
            array: ['foo']
        };
    }
};
</script>

Test is trying to retrieve all component instances by the custom attribute:

it('test', () => {
    const wrapper = mount(MyComponent);
    const components = wrapper.findAllComponents('[data-some-attr="attrvalue"]');
    expect(components.length).toBe(1);
});

It fails on the length assertion, appears to return the same component twice, even though array controlling the loop has 1 item.

If this problem is not familiar to you, I'd like to know if there's an easy reproduction template so I can sort this out.

@lmiller1990
Copy link
Member

I could not reproduce this: https://github.com/vuejs/vue-test-utils-next/compare/issue-689?expand=1

I don't think we have CSB yet. You could fork this repo, and reproduce it there - that'd be ideal, since then if there is a bug, we have the failing test case. I usually put components for reproductions inside of https://github.com/vuejs/vue-test-utils-next/tree/master/tests/components. You can use .vue files, or inline render functions like I did in my example - both are fine.

@xanf
Copy link
Collaborator

xanf commented Aug 7, 2021

@lmiller1990 here we go with reproduction

  it('finds components - demo', () => {
    const CompA = {
      template: '<div>A</div>' 
    }
    const CompB = {
      components: { CompA },
      template: '<comp-a />'
    }
    const RootComponent = {
      components: { CompB },
      template: '<div><comp-b class="foo" /></div>'
    }

    const wrapper = mount(RootComponent);
    // will return 2
    expect(wrapper.findAllComponents('.foo')).toHaveLength(1)
})

Frankly speaking, I'm not sure what is correct answer here
VTU v1 previously returned 1 for this scenario, returning top matching node - CompB. Why previously? Because usage of findAllComponents with DOM selector was removed in v1

While it might look reasonable to have "2" returned, the problem is, that this "2" might become "3" for example - consider CompA being refactored to have one more level of nesting. This is bad, because (for example) CompB might be some library component and we definitely do not want to rely our tests on implementation details of library and I definitely do not want @vue/test-utils to allow creating such fragile tests.

IMO it might be great to remove ability to find components by DOM selector in the same way it was done in VTU v1
In my opinion selecting Vue component by DOM selector

@lmiller1990
Copy link
Member

Right - I agree, we should not support finding components via CSS selector. This was deprecated, and should not have been implemented here in the first place, right?

@Giwayume
Copy link
Author

I don't agree, cause all my unit tests are written using CSS selectors. What if I'm testing a 3rd party library component where I do not have the ability to modify the source code?

How does this behavior compare to v1 where it wasn't broken?

@xanf
Copy link
Collaborator

xanf commented Aug 10, 2021

@Giwayume I can't say previous behavior (in VTU v1) was not broken - since multiple components can have same $el - it is a tough philosophical discussion what is correct behavior here and from what I know this was one of the reasons why finding components via CSS selector was removed on VTU v1 release

What if I'm testing a 3rd party library component where I do not have the ability to modify the source code?

I believe (please note - I'm not a member of VTU team) that if you're relying on retrieving components from inside of 3rd party component HTML - you're testing implementation details of 3rd party component, which is definitely not the way VTU should encourage people to do.

@Giwayume
Copy link
Author

It's not so much that I'm testing the functionality of the 3rd party components, but using CSS selectors to interact with them is required in order to test our UI that is built with them.

@lmiller1990
Copy link
Member

I think findComponent should match the current V1 behavior, which is not to support CSS selectors iirc. It currently does (and is documented, as well), so removing this will be a breaking change. We are pre 2.0, so we can make a breaking change, but we should make sure there is a migration path.

I don't fully understand how to use CSS selectors with a third party library. Once you select the component using something like findComponent(".some-selector") what do you do? Are you then accessing .vm?

@afontcu
Copy link
Member

afontcu commented Aug 20, 2021

I also agree we should not support using CSS selectors and encourage terser ones instead where possible. Moreover, findComponent on v1 only search for Component|ref|name, not CSS selectors

@Giwayume
Copy link
Author

Giwayume commented Aug 20, 2021

I think maybe I'm digging into the weeds too much, I don't care so much what findComponent or findAllComponents specifically does so long as I still have a solution in V2 to use a query selector to get to a Wrapper<Vue>.

const mainWrapper = mount(MyComponent);
const vueComponentEl: Element | null = mainWrapper.element.querySelector('.some-selector')`;
const vueComponentWrapper: Wrapper<Vue> = `WHAT GOES HERE?`;

@lmiller1990
Copy link
Member

^ This feature was removed from V1 intentionally. Can't you just search by passing the component instance?

but using CSS selectors to interact with them is required in order to test our UI that is built with them.

This doesn't make sense - you should be able to interact with them in the same way a user does, eg by finding the <input> or whatever HTML element they render. Is there some use case that I'm missing?

@Giwayume
Copy link
Author

Giwayume commented Aug 27, 2021

by finding the or whatever HTML element they render

Define "finding", because using a CSS selector is how I define "finding". CSS selector or XPath is what integration testing frameworks like Selenium use to simulate what a "user" does.

I would say passing in the component instance is "knowing", not "finding". Unit tests should be as unknowing to the internal workings of a component as possible and rely on the external interface (e.g. HTML).

@Giwayume
Copy link
Author

I don't understand this design that assumes the context of the structure of HTML elements surrounding Vue components is unnecessary. If I use a Vue library to place buttons on the page, findComponent('library-button') is pretty useless cause I could have several different buttons. I'm not going to ref every component just to satisfy a testing framework, tests shouldn't require code modification.

@lmiller1990
Copy link
Member

You could just do find('input#unique-id') - since you are searching by a CSS selector already, you'll end up finding the same element.

I think I mentioned in my post above - but once you find the component, what are you doing with it? Are you trying to access vm and modify with internal values or something like that? If you are just trying to click a button (like in your example) is there any reason find('button#some-id').trigger('click') does not suffice?

@Giwayume
Copy link
Author

Really depends, sometimes that's the case, other times it's checking props or emits.

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.

4 participants