Bug: findAll returns parent node in result array

[twoBoots]

Describe the bug
findAll returns the parent node in the result set if it matches the selector when chained from get

To Reproduce

ParentComponent.vue

<template>
  <div class="parent">
    <div>child</div>
    <div>child</div>
    <div>child</div>
  </div>
</template>

ParentComponent.spec.js

import { mount } from '@vue/test-utils';

import ParentComponent from '../ParentComponent';

describe('parent', () => {
  test('finds 3 children', () => {
    const wrapper = mount(ParentComponent);

    const parent = wrapper.get('.parent');

    expect(parent.findAll('div').length).toBe(3);
  });
});

result

parent
    ✕ finds 3 children (21 ms)

  ● parent › finds 3 children

    expect(received).toBe(expected) // Object.is equality

    Expected: 3
    Received: 4

Expected behavior
findAll returns only the 3 child nodes in the result array. This is the behaviour of vue-test-utils@1

Related information:

• @vue/test-utils version: 2.0.0-rc.18
• Vue version: 3.2.26
• node version: 16.13.1
• npm (or yarn) version: [email protected]

Additional context
I'm aware of the open PR reworking find and findAll, as well as a few mentions in other issues relating to findAll of a contributor currently working on changes to findAll. Maybe the open PR or the work in progress fixes this, but I thought it was worth documenting.

[lmiller1990]

Hmmm weird, I remember having some PRs around find and the parent, I wonder if this is a regression.

It looks like this line is checking that the parent is not returned, maybe is doesn't work as expected.

Are you interested in digging a bit and solving this?

[freakzlike]

I think this comes from this line

test-utils/src/baseWrapper.ts

Lines 48 to 50 in d414157

	const result: Element[] = [
	...elementRootNodes.filter((node) => node.matches(selector))
	]

The origin might come from #897 and after some refactoring it has been implemented into findAll.

But when removing this behavior then this will not work anymore:

const parent = wrapper.get('.parent')

Because wrapper.element is already the parent element. Also some issues when using multiple root nodes.

I think this is more a question on how the intention of find/findAll is. Should it be possible to use find/get to get the exact same wrapper again?

IMO it should not and it would make it more complicated. Only the edge case in #897 makes sense to me, where you have a DOMWrapper and want to get the VueWrapper. Maybe this can be done with a method for example domWrapper.asVueWrapper() and then all find methods will always return child elements

[aethr]

I'm not a react developer but I looked at enzyme to see what they do and it appears that enzyme's find does return the root node: https://github.com/enzymejs/enzyme/blob/57baba5ceaccec7a3ada40d7559f5bf71289cbe7/packages/enzyme-test-suite/test/shared/methods/find.jsx#L32-L36

I suppose in their case, they are making a distinction between the Wrapper itself, and the DOM tree within it, and their find is "find within the DOM tree of this Wrapper". VTU's find and findAll have felt more like "find within this element", which has made sense to me in all the tests I've written so far.

However, enzyme's approach might make sense for VTU in the case of a multi-root node, ie:

<template>
  <select class="a"><option>Foo</option></select>
  <select class="b"><option>Bar</option></select>
</template>

You might want to:

// findAll needs to operate on the root for select.a to match anything
expect(wrapper.findAll('select.a option')).toHaveLength(1);

Then if you modified the component later to split your selects and you ended up with:

<template>
  <select class="a"><option>Foo</option></select>
</template>

Would the same test still work? If you wanted the behaviour to be the same between single and multi-root nodes, I think the parent would need to be considered part of the search space for the target selector for findAll.

Unfortunately I think this is a more confusing API. I prefer find and findAll to search within the parent element, because if you wanted to test something about the parent element, well you already have the wrapper. Is the nicer, easier to understand API worth making the behaviour different between single and multi root components?

[freakzlike]

@lmiller1990 I can work on this if you want, but I need your decision. Here are my proposals:

1. find/findAll will not return the parent element

• find and findAll will always return the same results
• The wrapper will behave the same, no matter if single root or multi root element
• The root wrapper is always the "template"-tag of the component

In my opinion the cleanest way to avoid conflicts between find<>findAll and single<>multi root elements. But this will be some work to do and will also lead to a (maybe big) regression

2. find will return the parent element. findAll will not

• find will work as before
• findAll will not return the parent element except for multi root components

I was able to implement this successfully with the current unit tests, but I am not 100% sure if this will cause some regressions for other users.
https://github.com/freakzlike/vue-test-utils-next/commit/43d2d96422927b993a283a335de6e0a992ce345a

[lmiller1990]

I am conflicted. I do not really want to introduce breaking changes, but the example in the OP just seems completely wrong.

Your (1) proposal seems to make the most sense. I like to think of find and findAll like querySelector and querySelectorAll.

find

With non-fragment:

<template>
  <div id="a">
    <div id="b" />
  </div>
</template>

find('div') returns the outer element.

With fragment:

<template>
  <div id="a" />
  <div id="b" />
</template>

find('div') returns the #a element.

findAll

With non-fragment:

<template>
  <div id="a">
    <div id="b" />
  </div>
</template>

findAll('div') returns #a and #b.

With fragment:

<template>
  <div id="a" />
  <div id="b" />
</template>

findAll('div') returns all elements.

As per the OP

<template>
  <div class="parent">
    <div>child</div>
    <div>child</div>
    <div>child</div>
  </div>
</template>

find('.parent').findAll('div') returns the three children.

To confirm my understanding, is find('.parent').find(.'parent').find('.parent') currently working?

I think we should 1:1 match querySelector(All). This seems like the obvious way to handle things. I could be missing context, but why does findAll return the parent node at all? This seems completely wrong.

@xanf maybe has some context? Since he is located in Ukraine, I am not sure can reply soon :(

If not, I say go ahead - we should match the V1 behavior where possible, and make sure the default is intuitive and makes sense.

[xanf]

@lmiller1990 I totally agree with your logic. I will implement it

@lmiller1990 @freakzlike Actually after some digging I'm not sure we should change current behavior.

It is in line with HTML behavior:

const root = document.createElement('div')
root.classList.add('parent')
root.innerHTML = `<div>child</div><div>child</div><div>child</div>`

const template = document.createDocumentFragment()
template.appendChild(root)

// This also matches v1 behavior when `find` can return root element
console.log(template.querySelector('div')) // .parent

// This one differs from v1 behavior, but makes sense for me in terms of "unified" logic
console.log(template.querySelectorAll('div').length) // 4

It is a question of mental model - what wrapper is wrapping? :)
For me thinking of it always as fragment makes more sense just because it is simpler
But I will be happy to hear your feedback

[aethr]

@xanf I think that's still in line with what is being suggested, extending your example to match the OP's example:

const parent = template.querySelector('div'); // .parent
console.log(parent.querySelectorAll('div').length); // 3

This should be the expected behaviour, and I think your examples are in line with what's being suggested to treat a component wrapper like a fragment made using the <template> element.

When wrapper is a component, if we consider the <template> the top-level element, then template.querySelectorAll('div').length should return 4.

When wrapper is a DOM element (ie, .parent) then find/findAll should only search within the element and not include the element itself in the search space.

Or am I mis-understanding something?

[lmiller1990]

I think we are all on the same page.

const template = `
<template>
  <div class="parent">
    <div>child</div>
    <div>child</div>
    <div>child</div>
  </div>
</template>
`

wrapper.get('.parent').findAll('div') // should be 3 elements 
// `.parent` should not return `.parent` when using `findAll`

Right?

[lmiller1990]

Does this happen in rc.17? Was this a regression?

I want to release rc.19 but I think we need to fix regressions from rc.18 first. Namely, this (if regression) and #1173.

[xanf]

@lmiller1990 let's double check if we are on the same page:

• wrapper.findAll('div') should return 4 - since we are operating on Vue wrapper, we expect to include root nodes (especially important for multi-root)
• wrapper.get('div').findAll('div') should return 3 (obviously).

Am I correct? If so I could fix that and also update docs to reflect this
/cc @cexbrayat

[freakzlike]

@lmiller1990 let's double check if we are on the same page:

• wrapper.findAll('div') should return 4 - since we are operating on Vue wrapper, we expect to include root nodes (especially important for multi-root)
• wrapper.get('div').findAll('div') should return 3 (obviously).

Am I correct? If so I could fix that and also update docs to reflect this /cc @cexbrayat

I tested this with rc.16 and both cases are working as you mentioned

[lmiller1990]

Yes, that seems correct. My fix has a problem as pointed out #1377 (review). We don't have this test which now fails:

  // https://github.com/vuejs/test-utils/issues/1233
  it('finds all divs with findAll', () => {
    const wrapper = mount({
      template: `
        <div class="parent">
          <div />
          <div />
          <div />
        </div>
      `
    })

    expect(wrapper.findAll('div').length).toBe(4)
  })

I will add a test for this case and see if I can get it passing.

[lmiller1990]

@xanf added a patch for this missing edge case: a9fd8b5

I'm just hacking my way to success, happy to add more edge cases if they exist.