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

Support find({ ref }) syntax #716

Closed
kanryu opened this issue Jun 30, 2021 · 16 comments
Closed

Support find({ ref }) syntax #716

kanryu opened this issue Jun 30, 2021 · 16 comments
Labels
bug Something isn't working

Comments

@kanryu
Copy link

kanryu commented Jun 30, 2021

Subject of the issue

Calling findComponent () with the ref method causes a run-time error.

Steps to reproduce

<template>
  <div class="hello">
    <h1>{{ msg }}</h1>
    <button ref="some_ref" v-on:click="$emit('accepted')">OK</button>
  </div>
</template>
import { mount } from '@vue/test-utils';
import Component from '@/components/HelloWorld.vue'

describe('button-exists', () => {
    const wrapper = mount(Component)

    it('ref check', () => {
        const some_ref = wrapper.findComponent({ref: 'some_ref'})
        expect(some_ref.exists()).toBe(true)
    })
})
  • write a template like the one above
  • call wrapper.findComponent({ref: 'some_ref'})

Expected behaviour

findComponent() should return a valid node.

Actual behaviour

runtime error.

    TypeError: Cannot read property 'emits' of undefined

      42 |     it('ref check', () => {
      43 |         console.log(wrapper)
    > 44 |         const some_ref = wrapper.findComponent({ref: 'some_ref'})
         |                                    ^
      45 |         expect(some_ref.exists()).toBe(true)
      46 |     })
      47 | })

      at VueWrapper.Object.<anonymous>.VueWrapper.attachNativeEventListener (node_modules/@vue/test-utils/dist/vue-test-utils.cjs.js:7225:33)
      at new VueWrapper (node_modules/@vue/test-utils/dist/vue-test-utils.cjs.js:7202:15)
      at createWrapper (node_modules/@vue/test-utils/dist/vue-test-utils.cjs.js:7384:12)
      at VueWrapper.findComponent (node_modules/@vue/test-utils/dist/vue-test-utils.cjs.js:7305:24)
      at Object.<anonymous> (tests/App.spec.js:44:36)

Possible Solution

I can't comment because I don't know the detailed implementation of this framework, but when I checked it with the debugger, VueWrapper.prototype.attachNativeEventListener() is executed twice.

The first time, vm points to a Proxy and it runs success.
The second time it points to a HTMLButtonElement and it doesn't have a $options property, which causes a run-time error.

versions

  • "vue": "^3.0.0"
  • "core-js": "^3.6.5",
  • "@vue/compiler-sfc": "^3.0.0",
  • "@vue/component-compiler-utils": "^3.2.2",
  • "@vue/test-utils": "^2.0.0-rc.9",
  • "babel-jest": "<27",
  • "jest": "<27",
  • "vue-jest": "^5.0.0-alpha.10"
@afontcu afontcu transferred this issue from vuejs/vue-test-utils Jun 30, 2021
@xanf
Copy link
Collaborator

xanf commented Jun 30, 2021

The issue is that VTU is expecting findComponent result to always be a Vue instance, while your ref is assigned to HTML node

Since this is perfectly valid use case, I believe a perfect solution is to add assertion there and throw in case of such scenario as first step. After that allow find({ ref : ... }) on DOMWrapper and VueWrapper (this will require some effort, but is closely related to PR I'm working on, which allows chaining find() + findComponent() calls)

@afontcu
Copy link
Member

afontcu commented Jun 30, 2021

Adding a user-friendly exception there sounds a good first step. @kanryu fancy to open up a PR? 🚀

@lmiller1990
Copy link
Member

lmiller1990 commented Jul 8, 2021

I don't understand how this can be considered a valid use case. findComponent finds components, and in this case you the OP is trying to find a HTML node.

find({ ref: 'blah' }) for DOM nodes is not valid syntax and not supported. See the docs for how find works. It uses querySelector syntax - you can see the type definitions here.

This seems like the correct behavior. find for DOM nodes, findComponent for components (eg, things created with defineComponent). I am sure this has been discussed before.

If this was to be supported, it would be a new feature, and it would be find({ ref: '' }) if you are trying to find a DOM node. We can always discuss new features and ideas, but as of right now this is not supported.

I agree the error message is indeed very confusing. We should definitely fix that. I'd expect "Could not find a component with ref="some_ref", since that is exactly what's happening.

@xanf
Copy link
Collaborator

xanf commented Jul 8, 2021

@lmiller1990 Sorry, I needed to explain my "perfectly valid use case" - I was meaning that "it is perfectly valid use case to assign ref to DOM node in Vue".

I totally support your position about error message and that find({ ref: '' } could be considered as "new" feature (it was available in VTU1)

@lmiller1990
Copy link
Member

I understand now - thanks.

I see two actions here:

  1. fix this error message (this issue)
  2. consider finding DOM nodes via ref (separate issue, not a fan of features for the sake of new features, but happy to consider if there's some use cases)

@mgdodge
Copy link

mgdodge commented Aug 11, 2021

@lmiller1990 , I see the PR for the first action item you mentioned has been merged and will probably be released soon. Any idea when the second option (finding DOM nodes via ref, like VTU1 did) will see traction? I have numerous unit tests written for Vue 2 that utilize refs on DOM nodes, and converting them to Vue 3 would be a lot smoother with just that one addition. The example used for this issue, rewritten with find instead of findComponent is exactly the use case I would imagine.

@lmiller1990
Copy link
Member

lmiller1990 commented Aug 20, 2021

I think we could have this to help migration, but I don't think this should be a main feature or recommended way to query DOM nodes moving forward. I'd generally recommend things that work with querySelector (like role, id, data-testid, etc).

Unless you number of tests is truly staggering, it's probably just faster to change them all from ref to data-testid or something similar and move away from querying ref, than to wait for this to be supported.

In the meantime, if you want this now, you could try creating a plugin: https://next.vue-test-utils.vuejs.org/guide/extending-vtu/plugins.html#plugins. All the DOM traversing logic is here, we could expose some of the those functions if you need them. This feature could definitely be built in user land.

As for gaining traction, if enough people request the feature, we can definitely reconsider, but it seems like querying a DOM node by ref is tests not that common.

@robokozo
Copy link

robokozo commented Sep 13, 2021

Personally, I think it would be really great to be able to find HTML elements by ref. I prefer this because I don't end up adding extra data to the dom that are purely used for my unit testing purposes.

@sbmf21
Copy link

sbmf21 commented Sep 15, 2021

I am currently working on migrating our project to vue 3 and have about 400 tests that fail because I can't find dom elements by ref. I would love for this to be re-added

@lmiller1990
Copy link
Member

While we figure out how to implement this in a way that covers all edge cases, I will try to provide a method you can use in the mean-time to help with migration.

@mgdodge
Copy link

mgdodge commented Oct 28, 2021

@lmiller1990 Any progress on a method we ca use to help with migration?

@lmiller1990
Copy link
Member

Yes, https://github.com/vuejs/vue-test-utils-next/releases/tag/v2.0.0-rc.16 was released, findComponent now works with a DOM selector (again). Here is the PR: #994.

I don't think we have support for ref yet, but does this help?

@mgdodge
Copy link

mgdodge commented Nov 6, 2021

Sorry for the somewhat-wordy response, but I want to make a few points.

Unfortunately, your reference to findComponent does not help much in what I am asking. findComponent is useful for finding components, but the original example listed here and referenced in my initial comment demonstrate finding standard HTML elements using ref. If we were to replace findComponent with find, the problem of being unable to use a ref selector still exists. The example here is entirely valid, and matches my most common use case - refs on form inputs and buttons. The official Vue docs also include numerous examples - in fact, every example in their template refs section has them on standard HTML elements. This omission from the current VTU seems a real step backward, since it worked fine in the previous Vue 2 version.

I have always considered CSS selectors as being used for presentation, and if I really need a unit test to verify an element exists, assigning it a ref was a way of indicating that it had functionality important to the component being tested. I know that I could always add data-testid, but as @robokozo points out, why add a second attribute which ends up in the browser markup when we already have one assigned that is dedicated to Vue and doesn't pollute the markup at all?

Imagine scenarios like "Testing cancel and submit buttons in a modal" or "Testing input B when input A has a certain value"

  • In both of scenarios, a standard button or input element is being looked for.
  • Once I find the "submit button" or "input B", I might want to check if it is disabled or not.
    • Neither scenario has anything to do with what the element looks like.
  • Now, consider another developer who comes along and removes CSS classes as part of a rebranding effort.
    • Changing the look of something should not break the unit test, if functionally in the browser they work just fine.
    • A developer working on styles alone wouldn't likely touch the ref attribute, so there's a measure of confidence/safety there.

One additional limitation I have encountered using CSS selectors comes up when using LESS as your CSS preprocessor. In the Vue 2 version using <style lang="less" module> results in a warning that LESS isn't processed in VTU, so none of the classes even make it into the tests. In this scenario, you couldn't even start down the path of using CSS selectors to find things. I point this out because the suggestion to use CSS selectors to find HTML elements is not always going to be possible. Upgrading a component written with LESS modules and ref selectors will require changes that seem pointless.

I feel this should be re-added to core, but if that's not possible, a helper migration method would be amazing. A plugin would work as well, and I won't exclude that as a possibility. I do think, however, that the points I've made here illustrate that more thought needs to be given to a proper solution.

@lmiller1990 lmiller1990 changed the title findComponent() cause error "TypeError: Cannot read property 'emits' of undefined" Support find({ ref }) syntax Nov 18, 2021
@lmiller1990
Copy link
Member

Right, I understand now - the original issue title and discussion were completely different. I changed the issue title.

I forgot the context of why support for this was dropped. I can look into it again... if anyone else wants to speed things up, happy to help out with code review. Since we already have find by ref for findComponent, you can probably reuse a lot of that logic.

@lmiller1990
Copy link
Member

Coming in #1094

@kanryu
Copy link
Author

kanryu commented Jan 17, 2022

Thank you to everyone involved! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants