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

Proposal: Official way of targeting elements #1234

Closed
TheJaredWilcurt opened this issue May 15, 2019 · 22 comments
Closed

Proposal: Official way of targeting elements #1234

TheJaredWilcurt opened this issue May 15, 2019 · 22 comments

Comments

@TheJaredWilcurt
Copy link

TheJaredWilcurt commented May 15, 2019

What problem does this feature solve?

Currently there are several ways of targeting elements demonstrated in the docs (good). But people are confused as what the "best practice" would be and how to remove test tokens from their code in snapshots and production builds.

What does the proposed API look like?

  • Template/HTML:
    • Pick an official data- attribute for use. Something like <h1 data-test="pageTitle">.
  • Targeting:
    • Instead of:
    // prefixing ref with test to denote it is only used for tests
    wrapper.findAll({ ref: 'testPageTitle' })
    // This would still work, but we can do better
    wrapper.findAll('[data-test="pageTitle"]')
    • It could be:
    // Will target all elements with a data-test="pageTitle"
    wrapper.findAll({ test: 'pageTitle' })
  • Snapshots:
    • jest-serializer-vue now automatically removes all data-test="whatever" values from snapshots.
    • My fork of jest-serializer-vue has this enabled by default, with vue config options to disable it. (among many other improvements)
  • Production:
@TheJaredWilcurt
Copy link
Author

TheJaredWilcurt commented May 15, 2019

Notes on data-test naming:

  • I originally wanted to go with data-test-id but that didn't make sense with .findAll since ID would imply uniqueness.
  • From what I could find online most people are already using data-test.

I also think v-test="pageTitle" could work as well, but would cause confusion:

  • All other directives use JS expressions, so it would need to be v-test="'pageTitle'" (passing in a string).
  • If we wanted it to accept a string by default (like a normal attribute), it would deviate from the pattern of all other directives. There are no examples of using :v-if or :v-for etc. So making it pass a string by default and then allowing v-bind on it would be weird.
  • It would need to be documented with the other directives, and would feel out-of-place
  • I like that there aren't a ton of directives in Vue (see: Angular 😬).

What about data-qa?

I feel like this should be left untouched. The data-test is used by developers during development. So you would want to be able to automatically remove it from builds. But data-qa would be used by QA Engineers after the code has been deployed to automate their own manual testing. It could also be used in E2E testing. Taking this approach could allow for an easier distinction for developers to know "does this token need to be there in prod?" And if so, give it data-qa. Though we could also support wrapper.find({ qa: 'token' }), just to reduce redundancy.

@TheJaredWilcurt TheJaredWilcurt changed the title Have an official way of targeting elements Proposal: Official way of targeting elements May 19, 2019
@andreixk
Copy link

andreixk commented Dec 3, 2019

I'm currently using (my own) v-test directive for that exact purpose:

import { VNodeDirective } from 'vue';
export default {
  bind: (el: HTMLElement, binding: VNodeDirective) => {
    if (process.env.NODE_ENV === 'test') {
      el.setAttribute('data-test', binding.value || binding.arg);
    }
  },
};

can be used like this: <div v-test="'MyId'"></div> or <div v-test:MyId></div> both are a bit awkward, but I don't know how to make it work with v-test="MyId" without vue complaining on every component that MyId doesn't exist

@TheJaredWilcurt
Copy link
Author

TheJaredWilcurt commented Jan 12, 2020

I made my own fork of jest-serializer-vue with this and other features

@JessicaSachs
Copy link
Collaborator

Hmm @TheJaredWilcurt is this issue mostly concerning snapshot testing and Edd’s jest serializer? Can we close it? I don’t think it’s part of VTU’s core functionality.

@lmiller1990
Copy link
Member

Hi @TheJaredWilcurt , sorry no-one followed up on this. There are a bunch of new maintainers on board now and we are trying to catch up on all the outstanding issues.

Regarding this issue, I personally do not think there needs to be one "official" way to get elements. find("#selector") makes sense to me, because that's how you find things in the DOM, any web developer will be familiar with this. find(Component) is useful when your component doesn't have a selector (and you do not want to add an additional one like data-test. find({ name: 'blah' }) I personally like using that so I don't need an extra import MyComponent from 'whatever-library'.

I don't think there is any "best" practice for something like this, since everyone's applications and tests are different. These are just my thoughts, I know some (a lot?) of people like to have a guideline on "how to do ____", so I'm interested to see what other people think.

@TheJaredWilcurt
Copy link
Author

@JessicaSachs No, sorry for the poor wording. I've edited my comment, and the original post. The snapshots are just one part of this proposal. VTU's repo would still be responsible for adding in the more convenient shorthand of

// Will target all elements with a data-test="pageTitle"
wrapper.find({ test: 'pageTitle' })

@lmiller1990 Yes you can use any DOM selector to target an element for your test. And that is useful in many scenarios. However, when adding a token to your DOM solely to make it easier to target for your tests, it is a best practice to use an identifier token unique to testing (such as data-test="token"). This is because an ID can be used by many things, and may change as your code changes. Similarly, if your tests are targeting a nested DOM structure like td div .thing, it makes your tests more brittle and may require you to update them when modifying your code structure later when you otherwise may not be effected.

Pros to using a unique identifier:

  • Separation of concerns (Clearly related to testing and not used by anything else)
  • Tokens only used for testing can be removed automatically during build for lighter builds
  • Tests are less likely to break when refactoring. You know you are safe to delete the token. If the token was an ID or class, then it may be in use by your styles or other logic. This leads to leaving in vestigial code "just in case".
  • Does not add additional memory/cpu/overhead that is related to using a ref in Vue.
  • Easily accessible in child components, where as ref is not

Cons to using a unique identifier:

  • Shows up in snapshots (unless you automate removing them)
  • Increases build size. Shows up in the DOM and in your dist build (unless you automate removing them during builds)
  • Bike-shedding around what unique identifier to use. (data-test seems most popular, but the name is arbitrary)
  • Using ref="token" will scope to the current component being tested, data-test="token" does not so the tokens need to be properly name-spaced, in case they are used in a child component duing a mount.
  • wrapper.find('[data-test="pageTitle"]') is kinda ugly, which is why supporting a shorthand like wrapper.find({ test: 'pageTitle' }) is important.

Part of this issue is to identify and address the cons so that there can be a clear "best practice" to recommend people use for the default. The other approaches should still be offered and documented, so people can use best judgement to deviate from data-test when it makes sense.


Using a unique identifier, specific to testing, to target DOM elements is a widely considered best practice.

Resources:

@lmiller1990
Copy link
Member

lmiller1990 commented Jan 15, 2020

I agree using data-test is a good practice. I'd be in support of adding the shorthand you described: find({ test: 'el' }) which matches data-test='el' or some variation of this.

I also would be in support of documenting some best practices around testing.

I don't think we should pick an "official" way of targeting elements, and make that the only way to find elements. Vue lets you use $el.querySelector to look for elements, and I think VTU should mirror this. There is a difference between encouraging and supporting best practices and telling people how to write their tests. I think VTU should not be too opinionated - everyone has their own use case and opinions on testing.

I'm happy to accept a PR adding better support for search for data-test elements (I often find myself doing find('[data-test-submit]'), and some documents on best practices around selecting elements. You'd be welcome to make it, @TheJaredWilcurt. We could consider using data-testid as the selector, like testing library, which is a library designed to encourage best testing practices. They also have Vue integration via VTU - removing functionality from find might negatively affect their integration, which is another reason I'm not keen on a breaking change to find. @afontcu has more knowledge around this than I do.

These are just my opinions - I'm happy to go with whatever works for the majority of users. What do you think about these comments?

@afontcu
Copy link
Member

afontcu commented Jan 15, 2020

My two cents:

For the time being, I agree with Lachlan's view on not providing "the" right way of doing things – we're currently in "damage control" mode with VTU, and the main goal now is to reach 1.0 in a timely manner. After that, I'm sure we can discuss what's the best (if any) way of doing certain things, and I'd like very much to reduce some duplication in VTU – we have several tools that serve the same purpose, and I believe this might be confusing.

removing functionality from find might negatively affect their integration, which is another reason I'm not keen on a breaking change to find. @afontcu has more knowledge around this than I do.

I don't think this would happen, mostly because in Vue Testing Library we target the DOM, so as long as the attribute is still there, we're good.

And last but not least, thanks @TheJaredWilcurt for the well thought-out proposal – it is yelding interesting debates and let us focus on the big picture. Keep'em coming! :)

@lmiller1990
Copy link
Member

I thought about this a bit more. I do think there is value in simplifying find. I don't like name and ref finding options very much. I do think we need to keep the querySelector syntax. find(Component) would be good, except it doesn't work with functional components.

For the Vue 3 compat version of this library. I think we should drop ref and name. We should keep querySelector, every web developer knows about this and it's basically free since it's built into JSDOM. Only downside it is it not environment agnostic. Considering things like Weex/NativeScript or whatever someone thinks to build in the future, the most agnostic way to do this would be find(Component). Maybe we can work with Evan and friends to have something in Vue 3 to let find work with functional and non functional components.

For the beta and Vue 2 compat, I don't see much benefit in remove ref/name now. We could consider adding a depreciation, if we decide to remove them for Vue 3 compat.

@dobromir-hristov
Copy link
Contributor

dobromir-hristov commented Jan 25, 2020

Just my two cents:

Targeting by component is not ideal if we have more than one in the DOM.
Targeting by a DOM selector is also not OK if your component's root is another component, because they both share the same DOM node. I have explained this in #1385.
Targeting by ref is also not perfect, because the way we are used to traverse the DOM tree down with DOM selectors, is not how refs work. A set of refs, applied to the children of a component, are only available to it and it only.

<template>
 <component-a ref="a">
  <component-b ref="b" />
  <component-b ref="b" />
  <component-b ref="b" />
 </component-a>
<template>

If you are to do wrapper.find({ ref: 'a' }).findAll({ ref: 'b' }) you cant, because b is a ref of the "mounted" component, not a.

I do think we need to think of a more versatile, less error prone way of finding elements, but as of now I cant suggest it :/

@lmiller1990
Copy link
Member

It's tough. Each solution was imperfect, so we added another alternative. Now we have 4 problems instead of 1 😆

@TheJaredWilcurt
Copy link
Author

TheJaredWilcurt commented Jan 25, 2020

Since taking on the jest-serializer-vue-tjw project. I've learned more about common practices for selecting elements in tests. Here is the general consensus I've found:

  • data-test="token" - This should be the default choice. You should only deviate from this if you have a good reason to. Would be nice to have a shorthand like wrapper.findAll({ test: 'token' }).at(0).
  • data-testid="token" - This is used less often from what I can see online, and is sometimes used as an indicator that there should only be one instance of the token (similar to id attributes). Could also have a shorthand like wrapper.find({ testid: 'token' }).
  • data-test-id="token" - Same as above. Seems to be personal preference if you like it with the hyphen or not. In jest-serializer-vue-tjw, I'm supporting both as testid and testId. Same could be done here with wrapper.find({ testId: 'token' }).
  • data-qa="token" - These are usually added in for QA/QE's that are writing their own automated tests in tools like Selenium. They act as easy targets that are less brittle for QA/QE's to target when writing their own tests. However some devs will use them as a replacement for data-test. In jest-serializer-vue-tjw I have an option to remove them from the snapshots, but it is disabled by default. Not sure if we want to encourage this route, but Vue is typically very unopinionated. So for those that prefer using data-qa="token" instead of data-test="token", we could offer a shorthand of wrapper.findAll({ qa: 'token' }).at(0).
  • ref="testToken" - These should be avoided. They conflate concerns, requiring prefixing your token with test to indicate it is only used by your test, and not by your components logic. Using them requires overhead in Vue to set up this.$refs.testToken. Since ref is used for logic in Vue at times, it becomes difficult to automate removing the test tokens from production builds. However! They do have one very nice benefit. The token name is automatically name spaced to the current component. This can also be a downside (as mentioned above), since it becomes confusing/difficult as to how to target child/grandchild components that are outside of the ref's scope.
  • class="test-token" - CSS classes as selectors for tokens should be avoided in almost all instances. They conflate concerns, requiring a prefix of test- to indicate the class is only used for targeting in a test. They are more difficult to automate removing from production builds and snapshots. CSS Selectors can be useful when paired with a better approach, such as wrapper.findAll('[data-test="list-items"].active').
  • id="testToken" - ID's as selectors for tokens should be avoided in all cases. They conflate the concerns of CSS styling, JS logic, and testing tokens when used in this manner. They offer no benefit over class selection and just lead to greater confusion and liklihood of vestigial code after a refactor. They are harder to remove from snapshots and production builds.

Shorthand proposal:

Markup Long form Shorthand
data-test="token" .find('[data-test="token"]') .find({ test: 'token' })
data-testid="token" .find('[data-testid="token"]') .find({ testid: 'token' })
data-test-id="token" .find('[data-test-id="token"]') .find({ testId: 'token' })
data-qa="token" .find('[data-qa="token"]') .find({ qa: 'token' })
ref="testToken" Currently not possible .find({ ref: 'testToken' })
class="test-token" .find('.test-token') None
id="testToken" .find('#testToken') None

Something like this could be converted into documentation.

@angelogulina
Copy link

Hi @lmiller1990 @TheJaredWilcurt 👋

Is this proposal moving forward? I'm also in a situation that would benefit from a findByTestId-like approach. As the proposal here seems to go in the same direction, I don't see a need to propose something different.

I'd gladly move on with a PR if it's OK for you 🚀

@lmiller1990
Copy link
Member

lmiller1990 commented Sep 24, 2020

What is your suggestion @angelogulina? Which proposal are you referring to (there are quite a few here)?

Changing how find currently works is not an option, this would be huge breaking change. Other libraries depend on find, like Vue Testing Library (which has an opinionated getByTestId) by @afontcu and Cypress Vue https://github.com/bahmutov/cypress-vue-unit-test by @JessicaSachs.

It sounds like this proposal is more about giving people guidance on how to write their tests (eg, docs) than actually changing the library? Is my understanding correct? There is something like this in v2 docs: https://vue-test-utils.vuejs.org/v2/guide/conditional-rendering.html#finding-elements. I would like to share guides between the two, if possible. V2 docs are more about "how to test". V1 is really just "here is the API, good luck" which is really not great.

(Editing to be more concise).

@angelogulina
Copy link

@lmiller1990, I didn't read the Issue as a documentation one, thanks for making it clearer.

To reduce the scope of my proposal then (which would probably need a separate Issue), it would be to have a findByTestId() (not the get one, as you already now I'm not fan of function with side-effects).

I think it covers many use-cases. What do you think about it?

@angelogulina
Copy link

Also, if you need any help on the documentation side, I'd be happy to take something!

@lmiller1990
Copy link
Member

lmiller1990 commented Sep 24, 2020

I think VTU should regain it's focus as a simple utility library; not an all encompassing library. It is Vue Test Utils, after all, not Vue Testing Library (which already exists).

While I personally am opinionated in how I write my apps and tests, I don't think we should be too opinionated about how other people write their apps. Adding this method on top of find feels like a step in the wrong direction; putting something VTU is basically the same as saying this is how you should do it. Some people don't think data-test is a good practice; rather suggesting we use what the user would (eg findByLabel, findByPlaceholder, findByRole).

Finally, if you do want findByTestId to make your own suites more expressive, you can actually make you own findDataTestId in V2 using the new "plugins" feature (which I'll present about at Vue Toronto). You can see a findByTestId plugin in the docs. For these reasons we will not implement a new way to find components in core.

What I do this is valuable is talking about why data-testid is good, instead of something like id or class. We could talk about that here in the docs or somewhere else might make sense.


As far as guides go, I'd suggest focus on v2 docs. This is the future; @afontcu has a roadmap here. The v2 docs are much more well designed; we talk about how to use VTU, not just "here is the API... good luck". This includes things like why using a data-test selector is valuable.

Specifically @angelogulina, (or anyone reading this is motivated to contribute), here are 4 things that need to be done.

  1. this page needs to be written. I don't know what the vision is for it, will need @afontcu to guide you here, or you can just own it.
  2. I would even say it would be nice to have an entire page in VTU with some "general tips". Is this something you would like to do? If so make an issue in VTU docs next, list what you think is useful to include, and after a quick discussion and we can have it merged within days. I think this would be great for @angelogulina since you seem to have a lot of exp. and learning to share reslated to testing Vue apps.
  3. Port isVisible to this V2. This is easy but VERY high impact, everyone wants this feature. see Testing for visibility with transition (v-show) test-utils#210
  4. Write testing visibility docs see here

@TheJaredWilcurt
Copy link
Author

I was really hoping to have the shorthand options added into VTU:

Markup Long form Shorthand Custom find methods
data-test="token" .find('[data-test="token"]') .find({ test: 'token' }) .findDataTest('token')
data-testid="token" .find('[data-testid="token"]') .find({ testid: 'token' }) .findDataTestid('token')
data-test-id="token" .find('[data-test-id="token"]') .find({ testId: 'token' }) .findDataTestId('token')
data-qa="token" .find('[data-qa="token"]') .find({ qa: 'token' }) .findQa('token')
ref="testToken" Currently not possible .find({ ref: 'testToken' }) .findRef('testToken')
class="test-token" .find('.test-token') None .findClass('.test-token')
id="testToken" .find('#testToken') None .findId('test-token')

The proposed custom "Find" methods increases the API you have to learn. I don't really like that approach/recommendation.

Targeting elements with .find is such a common thing. It feels like this added object based approach would be a welcome bit of ergonomics to improve the Developer Experience.

@JessicaSachs
Copy link
Collaborator

@TheJaredWilcurt I agree with Lachlan's reasoning, which is why I added plugins to vue-test-utils-next a few months ago.

Here's how to register a plugin (from my vite starter)

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

const DataTestIdPlugin = (wrapper) => {
  function findByTestId(selector) {
    const dataSelector = `[data-testid='${selector}']`
    const element = wrapper.element.querySelector(dataSelector)
    if (element) {
      return element
    }

    return null
  }

  return {
    findByTestId
  }
}

config.plugins.VueWrapper.install(DataTestIdPlugin)

and here's a usage

const wrapper = mount(Counter)
const buttonCounterEl = wrapper.findByTestId('button-counter') // [data-testid="button-counter"]

So you should have no problem building on top of VTU for this! We're hoping people take advantage of the plugin API to build convenience methods that suit their style.

@lmiller1990
Copy link
Member

You could also add it to V1 by just doing

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

Wrapper.prototype.findByTestId = function() {
  // ... implement ...
}

@TheJaredWilcurt
Copy link
Author

I'm still sad this convenience was never adopted. VTU would be so much nicer to work with if this was part of the default API. I have a million .find('[data-test="token"]') all over my codebases and it's so ugly to look at every time and a pain to type. And switching between .find and .findComponent seems completely arbitrary and annoying to have to distinguish.

I still think this is a massive DX failure. Unit testing should be simpler than this.

@JessicaSachs
Copy link
Collaborator

JessicaSachs commented Aug 31, 2022 via email

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

8 participants