testing-library / react-testing-library

🐐 Simple and complete React DOM testing utilities that encourage good testing practices.
https://testing-library.com/react
MIT License
18.96k stars 1.11k forks source link

Add getById method to render exports #183

Closed kristianmandrup closed 6 years ago

kristianmandrup commented 6 years ago

Describe the feature you'd like:

For many apps the labels of elements are subject to the most potential change and volatility. In many cases makes much more sense to reference by id f.ex when developing i18n apps.

Suggested implementation:

getByTestId a shortcut to container.querySelector([data-testid="${yourId}"])

Simply add getById a shortcut to container.querySelector([id="${yourId}"])

Teachability, Documentation, Adoption, Migration Strategy:

Provide whatever docs to recommend it in some cases while not in others. I generally disagree with using text labels to reference HTML items as they are the most volatile in any app so it defeats the purpose IMO (+20 years web dev experience). But whatever rocks your boat.

Cheers.

Gpx commented 6 years ago

Hi @kristianmandrup, I'm curious why you say the text is volatile. Depending on which library you use to handle i18n you can have default text in your tests so that it doesn't change. An alternative could be to import your translations in the test file and use them to access your elements.

kristianmandrup commented 6 years ago

Yes, of course I thought about that, but then that would require the i18n setup before starting to test. You quickly get "locked in" using the text based approach IMO.

Adding my own API wrappers to better suit my needs. No worries ;)

const apiFor = (container) => {
  const elementBy = ({
    parent,
    tag,
    id,
    testId,
    name,
    type
  }) => {
    const sel = (name, value) => {
      return value && `[${name}="${value}"]`
    }

    let elem = sel('id', id) || sel('name', name) || sel('data-testid', testId)
    const typeSel = type
      ? `type=${type}`
      : undefined
    let attrSel = `[${elem}]`;
    attrSel = typeSel
      ? `${attrSel}[${typeSel}]`
      : attrSel
    const selector = tag
      ? `${tag}${attrSel}`
      : attrSel;
    const fullSelector = parent
      ? `${parent} ${selector}`
      : selector;
    return container.querySelector(fullSelector)
  }

  const api = {
    elementBy
  }

  api.submit = ({parent, id, name, testId}) => {
    const submitButton = api.elementBy({
      parent,
      id,
      name,
      testId,
      tag: 'button',
      type: 'submit'
    })
    fireEvent.click(submitButton)
  }
  api.change = (id, value) => {
    const field = api.byId(id)
    change(field, {
      name: id,
      value
    })
  }
  return api
}

Cheers!

kristianmandrup commented 6 years ago

Started creating an extension library here. Designed for now, specifically for testing forms and fields...

Gpx commented 6 years ago

To be clear, I'm not the owner of the project. I was trying to understand what your problem is. You should keep the issue open if you think it would be a beneficial feature to add

kristianmandrup commented 6 years ago

All of matter of dev style and philosophy I guess. I think the way most devs develop apps these days is still pretty much in the "iron age".

A core philosophy of this extension library is to better allow accessing elements by id or name as they are way less volatile reference markers.

Personally I like to use generators to generate most of my application artifacts from various schemas. Hence I don't know or care what labels will go in. This means that the core philosophy of "testing by usage" doesn't really suit my application development style, as I don't care about what the user sees at the end... until the end!

I tend to focus first on infrastructure and leave the UI concerns including labels/text to the final stage of development (ie. "skinning"). Hence methods such as getByText or getByLabel are pretty useless to me, except perhaps for E2E testing at the end. Sure, you could somehow reference the same labels being injected, but only if the injection mechanism is "fixed" and not subject to much change either.

I like to inject texts, labels, UI framework, theming etc. much later in the dev process and maintain flexibility with regards to i18n etc.

kentcdodds commented 6 years ago

This means that the core philosophy of "testing by usage" doesn't really suit my application development style

I think that means this library doesn't really suit your application development style either. Your style seems at odds with this library, it's philosophy, and it's utilities.

I'd recommend you create your own render method utility with the queries you like and go with that. You can actually pass custom queries as an argument to the render method so it could be built on top of react-testing-library if you want.

In any case, this is not something we'll be adding to the library. Good luck!

kristianmandrup commented 6 years ago

Agreed. I think the library is fine as is. I just have a different core dev philosophy, which means I need a few extra utility methods. I'm now working on my own extension library. "Gotta keep em separated..."

ivan-kleshnin commented 5 years ago

I think getById/getByClassName could be useful with debug(getById(...)) to suppress say header and footer tags from cluttering the render. What I have to do now is:

<div className="page-content" data-testid="page-content">

– where ids just duplicate class names for each page type 😞

kentcdodds commented 5 years ago

You can use container which is a DOM node. You can use regular DOM APIs on that.

pelotom commented 5 years ago

You can use container which is a DOM node. You can use regular DOM APIs on that.

Yes, but that doesn't give you the nice features of the getBy* methods:

Personally I would love to see getById, getByClassName and getByQuerySelector added to this library! I don't really see the downside.

kentcdodds commented 5 years ago

I don't really see the downside.

The downside is that people start using those features instead of the queries that they should be using instead. One of the things that makes react-testing-library so great is in what it makes more difficult. Think of it like dangerouslySetInnerHTML. Generally you should avoid it, but sometimes you need to. It's important that it's possible (as an escape hatch), but the fact that it's difficult/awkward is a feature.

This is something that I don't think you will be able to convince me otherwise. Sorry.

pelotom commented 5 years ago

Can I just ask, why is using getByTestId any more valid under your philosophy than getByClassName? Both are β€œinternal” attributes that the user has no perception of. The salient difference for me is that CSS classes often already exist, so you may not need to further instrument your component in order to test it.

kentcdodds commented 5 years ago

It's not much better. Using test ids is not recommended but it's an escape hatch to help you avoid worse selectors. Sometimes the given queries just aren't enough. Here's why a test id is better than class names it the id attribute: https://blog.kentcdodds.com/making-your-ui-tests-resilient-to-change-d37a6ee37269

kristianmandrup commented 5 years ago

The owner of any open source library can make his own rules and choices. If you don't like it, I guess you have to create your own extension to suit your needs and others who share that preference. Personally I also disagree with the specific choices and arguments to keep these restrictions, but I also respect it.

I've created an extension library with such extensions that you can help contribute to and improve or you can make your own.

We all have different opinions and styles etc. Not an "exact science". You can always create your own wrappers and DSL. Cheers.

kentcdodds commented 5 years ago

I've created an extension library with such extensions that you can help contribute to and improve

You gotta link to that project @kristianmandrup?

kristianmandrup commented 5 years ago

Here: https://github.com/kristianmandrup/dom-testing-lib-extensions

Has not been fully tested or used in real apps yet. Is a starting point with clear intensions, to make for a better DSL especially for testing forms.

pelotom commented 5 years ago

@kentcdodds with scoped CSS (css-in-js, css modules, etc.) I don't think the points of that blogpost hold water. I am having to add redundant data-testids which are identical to the classNames that I already had; it's pure noise. And in TypeScript my classnames are type safe; they could be imported to use in a test.

But of course @kristianmandrup is right, you can do whatever you want with this library. I hope I'm not out of line in expressing a pain point that I'm experiencing as a user.

kentcdodds commented 5 years ago

What are you selecting that the other selectors don't work for anyway? I rarely find myself actually using test-ids. Most of the time the other selectors work great for me.

kentcdodds commented 5 years ago

Also, no you're totally fine expressing pain points πŸ‘ I want to understand.

kristianmandrup commented 5 years ago

Exactly my point. Kent is able to work within his particular constraints on his projects with his select libraries, infrastructure etc and perhaps many others following in his footsteps. However the world is a very big place, and so there are many potential scenarios where these constraints become an issue... Why I find it better to "keep the options open" to scenarios that you don't contemplate yourself, I think. Then it's a "free for all menu" of options to build on. The CSS in JS is a perfect example. If Kent never uses that particular CSS flavor (or finds it an antibiotic pattern for whatever reason), he wouldn't recognize that pain point or find it trivial. Only natural.

kentcdodds commented 5 years ago

πŸ‘

For the record, I not only use CSS-in-JS but created one of the most popular CSS-in-JS libraries (glamorous) πŸ˜‰ In this case I actually find that trying to reference css-in-js generated class names is actually the worst thing you can do because your selectors will break any time you change the css which would be a nightmare (css modules may be ok if you're using identity-obj-proxy, but even then my blog posts explain why I think it's still not a good practice to use CSS selectors for tests).

kristianmandrup commented 5 years ago

I will just say that I strongly disagree with the whole notion of testing the app from the user perspective. Exactly to Kent's point about class names potentially changing.

Testing on elements having particular text and such, is like saying, give me the wall with that painting on it, when you want to test a wall. Well, you might decide to hang a different painting on the wall later or move it to another wall, or different text per locale etc.

What the user ultimately sees is the "icing on the cake", super volatile, which you should never rely on IMO.

Why I'd much prefer referencing by ID or class names, which is at the core of structure and resistant to being moved around (non-volatile structural attributes, mostly). Just my 2 cents.

kentcdodds commented 5 years ago

Huh, sounds like you should probably be using a different library then πŸ˜‚

I strongly disagree with that thought process πŸ˜…

kristianmandrup commented 5 years ago

Regarding class names, you often have some that are structural, for identification of element type and those that are decorative, ie. being targeted for styling. Only the decorative should be somewhat volatile.

alexkrolick commented 5 years ago

Using classnames for structural queries is prone to breaking with minor, non-visible refactors like adding an inner div, refactoring to use a 3rd party component with its own CSS, etc., etc.

Testids are great for these cases because they can be moved around when refactoring to point at a new element; however, there are often even better semantic alternatives.

For example, one common use case for testids is selecting a container element that doesn't seem to have a label:

within(getByTestId('card-x'))
  .getByLabelText('Name')
|-test-id="card-x"----------------------|
|                                       |
| Card Heading                          |
|                                       |
| [ input ] Name                        |
|_______________________________________|

But this can often be replaced by an ARIA label:

within(getByLabelText('Card Heading'))
   .getByLabelText('Name')
|-aria-labelledby="card-x-heading"------|
|                                       |
| Card Heading   "#card-x-heading"      |
|                                       |
| [ input ] Name                        |
|_______________________________________|
alexkrolick commented 5 years ago

Another comment was that text content changes a lot during development. Most internationalization frameworks allow using the default strings at runtime without any build step or HOCs. Even if you're not using an i18n framework you can export string constants and reference them in the test.

The only difficulty I've had with this is where multiple elements have the same text, like a form title that says: Sign In and a button inside that says the same thing. Testids help there but there may be a better way.

kentcdodds commented 5 years ago

This is what I'd do there:

getByText((text, node) => node.tagName === 'BUTTON' && /sign in/i.test(text))

Typically it's not good to depend on the tagName of elements, but in practical situations this escape hatch make it possible :)

alexkrolick commented 5 years ago

That works. And with i18n files, the translation id can also be used to disambiguate the elements - lots of solutions become possible when give the strings unique names.

kristianmandrup commented 5 years ago

Having thought more about this, I would soften up a bit regarding my previous comment. Obviously both have their place. At least ~90% of the DOM or a front end app is structural, while the icing on the cake is the remaining 5-10%. The house analogy is very accurate. Ideally we should have separate structural and presentational tests. Would be awesome if we could have React render the app without any Text elements or image sec attributes etc, to make a structural integrity check, like a house check. Then another set of tests from user's perspective to make sure they get what they need, tested in a way suitable for a much more volatile output (regex matchers mostly?). Could be a nice set of React libs on their own.

kristianmandrup commented 5 years ago

I'd actually propose ideally building the entire app without any presentation embedded in the DOM, then add all presentation elements, including text, images etc. using CSS entirely. Could be done elegantly, integrating with i18n solution using CSS content element. Much cleaner separation and ensures tests are operating in a clean environment as well. Then have tests for both layers, structural/unit (~90%) and presentational/acceptance (~10%). Would be interesting to experiment with such a clean approach.

kristianmandrup commented 5 years ago

@alexkrolick I've had several experiences where I've had to deliver an app while the client had no firm idea on the final presentation until very late in the game (f.ex having a design agency etc. doing this on an entirely separate, parallel track). In this scenario, you simply don't have any text except 'placeholders".

alexkrolick commented 5 years ago

In that scenario:

Victorcorcos commented 4 years ago

Why they don't have this by default?? I can't believe it. The most basic thing we want to query on the frontend is querying elements by Ids. It is the Hello World of frontend querying.

kentcdodds commented 4 years ago

Why so aggressive? There are good reasons and they're described in comments above.

Victorcorcos commented 4 years ago

@kentcdodds

It is not about being aggressive, it is just what I really think. Querying by id and classes is the Hello World of the frontend querying and the library doesn't allow that. What about creating the getById and getByClass on the tests?

Restricting the frontend id querying is not improving, it is quite the opposite, what is really does is decrease the capacity of the library.

Placing a lot of data-testids on the code pollutes it, and places unnecessary tags on html codes in terms of code functionality. The code becomes polluted with tags only related to test.

I am in favor of TDDs, but adding things in the code (or worse: on html tags) only for tests is exaggerated. A good TDD will improve the code with more organization! A bad TDD will add things on code only related for testing purposes.

Some people can argue and defend the usage of data-testids on the code for whatever reason, but what I find uncomfortable is to disable for everybody the most basic querying on DOM elements.

weyert commented 4 years ago

Personally, introducing getById or getByClass in my opinion would go against the philosophy of this library. The user of your application doesn't depend on id-attribute of an HTML element to access it. I have been using this library for a quite a while now and I haven't found the need to use a query like getById and barely need to use a test id.

I think the documentation and supportive tooling is getting better and better, specially with the latest addition like which-query.

kentcdodds commented 4 years ago

I don't think this issue can be any more productive do I'm going to lock it. There's no way we'll add more direct support for regular querySelector than we already have so no more needs to be said.