jest-dom icon indicating copy to clipboard operation
jest-dom copied to clipboard

Group matchers in documentation

Open datenreisender opened this issue 5 years ago • 7 comments

Describe the feature you'd like:

Being new to testing-library, it all is a bit overwhelming. The long list of matchers jest-dom introduces, also makes it harder to start out.

Suggested implementation:

Group the matchers in the documentation over some headings, e.g. “Checking element existence”, “Checking forms”, “Checking element state”.

Describe alternatives you've considered:

A longer introduction into jest-dom with some examples on how to use it and which matchers to look into in which order might also be nice, but probably more work. 😊

datenreisender avatar Nov 29 '19 08:11 datenreisender

Nice suggestion. Though I wonder what these categories could be to be really useful. For example, based on your own examples “Checking element existence” is only one matcher (toBeInTheDocument). It's a category of one. While “Checking element state” is presumably a category of many other matchers (if I understand it correctly).

Would love to hear concrete proposals of what these categories could be. Here's the entire current list of matchers for reference.

  • toBeDisabled
  • toBeEnabled
  • toBeEmpty
  • toBeInTheDocument
  • toBeInvalid
  • toBeRequired
  • toBeValid
  • toBeVisible
  • toContainElement
  • toContainHTML
  • toHaveAttribute
  • toHaveClass
  • toHaveFocus
  • toHaveFormValues
  • toHaveStyle
  • toHaveTextContent
  • toHaveValue
  • toBeChecked

Note: I checked jest-enzyme and they don't group them, and still have three more matchers than jest-dom. Not implying it's not possible, but I thought we could get inspiration if they did something similar.

gnapse avatar Nov 29 '19 11:11 gnapse

The toContain… matchers also check for the existence of an element and I would even place the toBeVisible in that category, since from the point of view of an user, an element only exists if it is visible. So this would lead to these groupings:

Checking element existence

  • toBeInTheDocument
  • toContainElement
  • toContainHTML
  • toBeVisible

Checking element state

  • toBeEmpty
  • toHaveAttribute
  • toHaveClass
  • toHaveFocus
  • toHaveStyle
  • toHaveTextContent

Checking forms

  • toBeEnabled
  • toBeDisabled
  • toBeValid
  • toBeInvalid
  • toBeRequired
  • toHaveFormValues
  • toHaveValue
  • toBeChecked

Please also note, that I grouped toBe(Inv|V)alid together, just like toBe(En|Dis)abled, and I did put the positive variant first in both cases.

datenreisender avatar Dec 05 '19 12:12 datenreisender

Wow yes, now I see it. Thanks!

One nit-pick: I'd call the third category "Checking form controls" because some of them can be used outside forms (e.g. buttons are everywhere and that can be enabled or disabled). I'm actually a bit hesitant that the enabled/disabled checks are under "forms", but I'm ok with it. This is overall really better than our current flat list.

Are you up to it?

And now that we're at it, I've been meaning to get rid of this duality where for every element we present examples for both using native APIs and dom-testing-library APIs. I'm not sure which one to keep, but I find it too verbose in our README to have all this. (this is technically unrelated but a nice to have, though I may open a separate issue to discuss this).

gnapse avatar Dec 05 '19 12:12 gnapse

Yes, I had exactly similar thoughts, also being unsure what to call the third category and whether to place “enabled” in there because of buttons. But putting it in there seems the best choice to me right now.

Regarding examples with document.querySelector and DOM Testing Library: Yes, I also think the current variant makes everything too long. Since the main motivation for this package currently is the use with testing-library, I suggest explaining and showing only once that it can also be used with document.querySelector but then always using the functions from testing-library.

datenreisender avatar Dec 05 '19 13:12 datenreisender

Maybe also discuss what matchers we could remove?

toHaveTextContent ~is~ should just be toHaveProperty('textContent', 'foo')

I'm not sure which one to keep, but I find it too verbose in our README to have all this.

I would agree. The native DOM API should be enough.

eps1lon avatar Dec 05 '19 13:12 eps1lon

toHaveTextContent ~is~ should just be toHaveProperty('textContent', 'foo')

No, for two reasons: toHaveTextContent is so important, that it deserves a special matcher. Also, it does do additional normalising of the string, which toHaveAttribute does not do (I assume you meant toHaveAttribute).

datenreisender avatar Dec 05 '19 13:12 datenreisender

Exactly. toHaveTextContent does a bit more (see #21 and #56).

And in case someone's wondering about the simplicity of the implementation of toBeEmpty (it can easily be done with expect(element.innerHTML).toEqual('')), it is added for readability and more clearly expressing intent.

As per jest-dom's guiding principle (emphasis added here):

the overall criteria for what is considered a useful custom matcher to add to this library, is that doing the equivalent assertion on our own makes the test code more verbose, less clear in its intent, and/or harder to read.

gnapse avatar Dec 05 '19 15:12 gnapse