New matcher `toBeHeading` for SEO-related tests

[SergiCL]

Describe the feature you'd like:

The idea is to create a toBeHeading matcher to check if an element is an HTML Heading, no more, no less.

This can be useful for SEO-related tests, for example to check if desired text is an h1, h2, etc.

I've been working on this as a hobbie to familiarize myself with the library, but I think it could be useful for someone else.

Suggested implementation:

The implementation is simple, the function has only one parameter wich is used to define desired HTML heading.

My first aproach was something like this:

toBeHeading({heading: 'h1'})

But I think is so redundant. Maybe something like this could be simpler:

toBeHeading('h1')

On the other hand, I have my doubts about whether or not to add a default behavior. I can think of two possible implementations if no parameter is passed:

• Use 'h1' as the default header.

  expect(elementH1).toBeHeading()
  expect(elementH1).toBeHeading('h1')
  expect(elementH1).not.toBeHeading('h2')

• Return true if it is any heading.

  // Yeah, they're headings but I don't think people want to try it this way.
  expect(elementH1).toBeHeading()
  expect(elementH2).toBeHeading()
  expect(elementH3).toBeHeading()

  I think both, especially the second one, can lead to false positives and confusion. What do you think?

Describe alternatives you've considered:

Maybe Google Chrome Audits or another SEO tools could be posible alternatives but I prefer to have control over what I am testing.

Teachability, Documentation, Adoption, Migration Strategy:

It is not a complex project although there are some things that would be good to discuss and modify.

If someone else thinks it might be useful and have a place in the library, I can take care of the implementation myself if you want.

[gnapse]

Maybe this is a naive question, but what's wrong with something along the lines of

expect(element.tagName).toEqual('H1')

[SergiCL]

You're right. It probably doesn't bring much beyond readability, and maybe on put the focus on SEO-related tests.

As I said is a really simple matcher I was working on to familiarize myself with the library, probably is to simple and in the end not needed.

[gnapse]

I mean, even if it does not add much, I appreciate it a bit. Our toBeEmpty does not do much internally, but it makes things readable.

I'd understand having a matcher toBeTag('h1') or toHaveTagName('h1'), but that'd be to check up on any tag name, not just headings. Would that fill your needs too?

BTW headings can also be expressed as <div role="heading" aria-level="1">Title</div> and I also thought this hypothetical toBeHeading custom matcher could check up on those too. But then I see this different way of doing headers is discouraged.

[SergiCL]

I agree. It solves the problem perfectly. The implementation is simpler than for toBeHeading('h1') and is more useful.

In this case there is not much doubt, maybe one. Does it make sense to check if the tag is a valid HTML tag? Or can we assume that?

toHaveTagName('h1') seems more descriptive in my opinion.

Are you OK if I start to work in this approach?

[gnapse]

+1 on toHaveTagName. I think it makes sense overall, but would like the input of others. I'll invoke @eps1lon who's insight is always useful, and he thumbed up your original proposal, so I'd love to know what he thinks.

Does it make sense to check if the tag is a valid HTML tag

Maybe in the long run, but I wouldn't worry about that in an MVP. Unless we can have easy access to something that would validate that for us without too much hassle. The only thing that comes to mind is that maybe we can check the validity of a tag name by trying to create a detached element with the given tag name. Assuming any DOM implementation would fail in doing so for invalid tag names, that'd be an easy way to check that. I'd rather avoid having to maintain a list of all valid tag names ourselves.

[IagoLast]

👋 Hey there, I just wanted to give my two cents here.

I totally agree with @SergiCL on having methods for specific SEO related tests, but as far as I understand one of the core principles of the testing-library ecosystem is removing implementation details from tests in order to make them less fragile.

Since toHaveTagName is very low level, I'm afraid adding this matcher could lead to potential misuses.

My proposal here would be adding some kind of namespace (or even a new set jest-matchers on a different package) for this kind of tests where we could potentially add more checks (ie the lighthouse ones)

const myWebPageWrapper = render(<App/>);

// Matcher discussed here
expect(myWebPageWrapper.getByText('Welcome to my Site!').seo.toHaveTagName('h1');

// New specs under the seo "namespace"
// https://developers.google.com/web/tools/lighthouse/audits/description
expect(myWebPageWrapper).seo.toHaveDescription(...);
// https://developers.google.com/web/tools/lighthouse/audits/hreflang
expect(myWebPageWrapper).seo.toHaveHrefLang(...);

@gnapse Does it make sense to you? Do you think this fits in jest-dom or maybe would be better to have them under a new jest-seo package?

[SergiCL]

as far as I understand one of the core principles of the testing-library ecosystem is removing implementation details from tests in order to make them less fragile.

This is the main reason why I focused the selector on the headers and not on the tags in my first suggestion. But thinking about it, it's already possible to use expect(element.tagName).toEqual('H1') to check tagName.

I'm not a big fan of limitate a tool options just to avoid users to mess up. In the end if they want to do things wrong they will always find a way to do it. On that basis, I don't think toHaveTagName will be a problem.

My proposal here would be adding some kind of namespace (or even a new set jest-matchers on a different package) for this kind of tests where we could potentially add more checks (ie the lighthouse ones)

I don't dislike the namespace idea, but I think it can complicate readability. For example, in this case:

expect(myWebPageWrapper.getByText('This is a subtitle').not.seo.toHaveTagName('h1');

On the other hand, I don't think it's a good idea to create another package. This would complicate access to these selectors and in case you want to use both, it would add more unnecessary imports.

I don't think it'll be a problem to add any of those selectors directly to this library. As long as they are organized in the documentation under a SEO section no one should have a problem understanding them.

If it is true that when creating a SEO section probably toBeHeading is a more convenient matcher.

There are some interesting Seo matches to be implemented (To have title, meta description, social meta tags, etc) but I guess this is for another conversation. How about starting with toBeHeading('H1')?

I propose to always call the function with desired heading tag in capital letters, to be closer to 'expect(element.tagName).toEqual('H1')`. For the option to call it without parameters I would discard it because it takes away readability and is confusing. In case of making the call without a tag it would throw an error. The same case if the tag is not valid heading or is in lower case.

So that is how it could be:

expect(elementH1).toBeHeading('H1')

expect(elementH1).not.toBeHeading('H2')

expect(() => {
    expect(elementH1).toBeHeading()
}).toThrowError()

expect(() => {
    expect(elementH1).toBeHeading('H7')
}).toThrowError()

expect(() => {
    expect(elementH1).toBeHeading('h1')
}).toThrowError()

[IagoLast]

¿Qué os parece si empezamos por toBeHeading('H1')?

😂 Me parece perfecto!

For the option to call it without parameters I would discard it because it takes away readability and is confusing

👍

I don't think it'll be a problem to add any of those selectors directly to this library. As long as they are organized in the documentation under a SEO section no one should have a problem understanding them.

In my experience people doesn't read docs, they just copy paste that's why I think it worth highlighting this is not a regular selector.

The namespace option is my favourite but we could also put something in the name:

expect(wrapper).toHaveHeadingForSEO('H1');

expect(wrapper).toHaveSEOHeading('H1');

expect(wrapper).checkSEOHeading('H1');

[SergiCL]

¿Qué os parece si empezamos por toBeHeading('H1')?

😅 I will correct this.

we could also put something in the name

IMHO toBeHeading has enough SEO connotations. Don't get me wrong. I don't dislike the namespace option, I only see a readability problem when 'not' is used:

expect(wrapper).not.seo.toBeHeader('H1');

Maybe something like this could be more readable:

// It is clearer that toBeHeader belongs to the seoTools
expect(wrapper).not.seoTools.toBeHeader('H1');

// It seems more readable as a set 'not valid seo, to be header'
expect(wrapper).not.validSeo.toBeHeader('H1');

[gnapse]

I don't like that namespacing thing at all. Haven't even seen it as standard practice in jest matchers, not even sure if it's possible. Would really not want to go down that road.

One thing I'm not sure I fully understand in all this discussion, and that I should've flagged earlier is: what does asserting that a given element in a html fragment is a heading has to do with SEO?

[IagoLast]

Haven't even seen it as standard practice in jest matchers not even sure if it's possible

Good point.

what does asserting that a given element in a html fragment is a heading has to do with SEO?

I never found a proper source but apparently using headings correctly improves the seo of a site so The toBeHeader matcher could be used to ensure the right keywords have the right header and they are not lost during a refactor.

(General info about headings usage) https://yoast.com/how-to-use-headings-on-your-site/

(Last point mentions SEO) https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#Usage_notes

[eps1lon]

In the end if they want to do things wrong they will always find a way to do it.

Nobody wants to do things wrong. Constraining an API is not to hurt people that want to do things wrong but assist people that don't want to make mistakes. In the end of the day we all make mistakes. If we have the chance to design an API that helps avoiding mistakes then we should do it.

Personally I don't think that it is worth it to add heading tests to this library. There are plenty of tools that verify that you have a valid heading structure. If our goal is to aid SEO testing then we also need to verify that the hierarchy is valid. After all expect(element).toBeHeading() doesn't really tell us much about how the element does SEO-wise or a11y-wise. You could still have a screen with just <h3 /> or <h2><h1 /></h2>

[gnapse]

Thanks for the info.

Seems to me improved SEO is just one of the advantages of using headings properly. There are also a11y advantages. So focusing this potential new feature on making it a SEO matcher is not helpful.

Also, some of the advantages in using headings properly are around how the overall headings structure of a page is built, and you cannot say much about it by inspecting a single particular element.

So not sure how a potential toBeHeading matcher would help, with SEO or otherwise. Especially if all it can do is check if the element is of a given tag name. I'm leaning towards there having to be more to it before this can make it into a dedicated jest-dom feature.

[SergiCL]

Nobody wants to do things wrong. Constraining an API is not to hurt people that want to do things wrong but assist people that don't want to make mistakes. In the end of the day we all make mistakes. If we have the chance to design an API that helps avoiding mistakes then we should do it.

You're right, I didn't say it in the best way. What I meant was that not giving flexibility to a tool can leave out cases that may be getting away from us. As I see it warning or a linters could give flexibility and avoid some errors.

I'm leaning towards there having to be more to it before this can make it into a dedicated jest-dom feature.

I'd like to add a little context to this story. More than once I had to add heading tags in a page as a requirement, and when I was testing I missed a matcher to do it in a simple way. That's why I thought of creating a matcher and mainly to see how the library was built. I wanted to see if there was anyone else who could use it, but I think it's too specific.

In the end of the day this is not a bad solution for my problem:

expect(element.tagName).toEqual('H1')

As far as I'm concerned, the case is solved. If no one wants to contribute anything else we can close the issue.

Thank you for your attention and congratulations on the great work you are doing.

[gnapse]

Glad it is working out for you with the simpler alternative.

BTW, I just realized there may be another alternative as well, assuming you're using DOM Testing Library or some of its more specific sister packages for React, Cypress, etc. Assuming also that you are grabbing the header element by text or by some other query in the first place, that is, assuming you are doing something like this:

const header = await findByText(/my page title/i)
expect(header.tagName).toEqual("H1")

I think you might be able to do the same with just the testing lib query:

await findByText(/my page title/i, { selector: 'h1' })

Haven't tried it, but I think it could work.

[eps1lon]

The better alternative would be to have a snapshot of the accessibility tree. Though that is quite complex. I think as a good first iteration we could have something that matches the heading structure e.g.

// should also work with a list
expect(screen).toHaveHeadingStructure({
  name: 'Title',
  level: 1,
  children: [
    { name: 'first item', level: 2 },
    { name: 'second item', level: 2 }
  ]
})

We already have the ability to compute the accessible name and the rest would be solved by dom-traversal. I think we can skip aria-owns. I don't even know if this is supported by search engine crawlers.

Validation of the heading structure would be another step.

[eps1lon]

Oh an also: getByRole('heading', { name: 'My heading' }) is already possible. Adding a level filter would be a good addition as well i.e. getByRole('heading', { name: 'My title', level: 1 }).

[gnapse]

I was thinking also of something along the lines of that toHaveHeadingStructure. I'd say it would even skip the levels given that there's the recommendation that you should not skip them. Maybe only specify the expected level at the root and that's it.

[SergiCL]

The toHaveHeadingStructure option seems like a very good solution to me. Maybe the complexity can be reduced a bit, but it's on the right track.

I'd say it would even skip the levels given that there's the recommendation that you should not skip them.

I have thought about just that, I don't think it is necessary to specify the level in each element, doing it in the root would be enough.

Another question that comes to mind is whether the order of the children is important. In my case, as long as they don't show up before the father does, that's enough. One possible solution is to add an explicitOrder field (maybe in a next step)

Something like this:

// if 'First level 2' is after 'Second level 2' it must be true
expect(screen).toHaveHeadingStructure({
  name: 'Title',
  rootLevel: 1,
  explicitOrder: false,
  children: [
    { name: 'First level 2' },
    { name: 'Second level 2', children [
       { name: 'Level 3 under second level 2' },
     ] }
  ]
})

[gnapse]

One random thought to throw at there as well: this is becoming a lot similar to snapshot testing. Maybe it could go down the path of having something generate the heading structure of a piece of DOM, and the matcher could just be .toMatchSnapshot().

[gnapse]

There seems to be no concrete actionable stemming from this issue, right? There are some cool ideas, but nothing concrete. I'm leaning towards closing this issue, but I want to hear what other's think.

[SergiCL]

Oh, I'm sorry, I completely forgot about this. The problem for which I opened this issue has been solved. I finally used this:

expect(element.tagName).toEqual('H1')

For my part, we can close it. Thank you all for your help!