Dom-testing-library: Improve the 'byText' queries capability.

[Dajust]

Describe the feature you'd like:

'byText' queires, getByText(), for example, works greate in writing tests that closely resembles users actions, but it has a drawback, expecially on integration tests, which usually requires many components to be rendered.

Sometimes a user might be interested in a particular text, but only on a prticular section of the page. Take the bellow screenshot for example.

So the user sends a 'lol' and 'today' message and wants to make sure the message is sent. She'll have to look for the message "byText" 'lol' to see when/if the message has been markded as sent. It is only the "byText" queries that can help us do this job, but here's the problem: There are many 'lol' and 'today' texts on the page, but our user is only looking at or interested in the 'lol' text INSIDE-THE-MESSAGES-SECTION.

getByText in it's current form will not help us in such case, because it will grab the text 'lol' anywhere it founds it on the page, which in turn require our tests to rely on DOM structure to get the text where're interested in.

Suggested API:

Provide an option to tell the 'byText' queries where-in-the-document they should get the text. Something like this:

getByText('some text', {in: node})

Suggested implementation:

I think this could be easily implemented by some minor editting the queryAllByText fucntion.

We could simply check to see if the in option is present, then use it as the baseElement of the querySelectorAll(). We only need to touch too lines in the function. Something like this:

function queryAllByText(
  container,
  text,
  {selector = '*', exact = true, collapseWhitespace = true, trim = true, ...options} = {}, 
) {
  const matcher = exact ? matches : fuzzyMatches
  const matchOpts = {collapseWhitespace, trim}
  // determin what the baseElement should be
  const baseElement = options.in ? options.in : container;
  return Array.from(baseElement.querySelectorAll(selector)).filter(node =>
    matcher(getNodeText(node), node, text, matchOpts),
  )
}

Describe alternatives you've considered:

N/A

Teachability, Documentation, Adoption, Migration Strategy:

To use this feature, user will need to first grab the in node, then use it as the in option. Like this:

const messageArea = getByTestId('message-area');
getByText('some text', {in: messageArea})

[sompylasar]

See https://github.com/kentcdodds/cypress-testing-library/pull/11

[sompylasar]

Also see getVisuallyBelow in the comment here: https://github.com/kentcdodds/cypress-testing-library/issues/6#issuecomment-392115036
I cannot find time to make it a part of cypress-testing-library or dom-testing-library, but the idea I hold is that to not rely on the DOM structure, the DOM search by text or other visual criteria has to happen on the whole set of DOM nodes, ranking them by visual criteria such as distance from an anchor point, z-index, size and area to determine the relevance.

[Dajust]

Hey, thanks so much for the quick response @sompylasar. I'm not sure, but I think the solutions you're pointing to are all for cypress-testing-library. I'm suggesting this feature for react-testing-library. I've not used others, but I think if we could add just that one line here, the rest dependants shouldn't bother changing a thing.

[sompylasar]

@Dajust Yes, as I understood, you wanted a field in the "options" argument to override the "container" argument to serve the same purpose that "container" already does. First of all, I suggest "within" for a name, "in" is too short to be searchable, it's also a JS keyword in some contexts. Second, could you please elaborate why do you want it to be in the "options" and why "container" does not work for you?

[sompylasar]

P.S. The solutions I show are implemented in cypress-testing-library because I now use Cypress at work for UI end-to-end tests and that's where I was given enough time to apply my ideas. Previously I played with Puppeteer and made a POC implementing the same ideas with injected scripts (for context, this is how Cypress works, by injecting scripts next to your running app).

[sompylasar]

getByText in it's current form will not help us in such case, because it will grab the text 'lol' anywhere it founds it on the page, which in turn require our tests to rely on DOM structure to get the text where're interested in.

Specifying within option in the test does the same, makes your test rely on DOM structure, right?

[Dajust]

Sure, I don't have a problem with whatever name that works :smile: I'm also not doing end-to-end testing at this time, just integration tests.
In react-testing-library, I'm not sure of any way to override what the container would be because it is hard-coded during the render phase. Besides, I think it's necessary that the container remains how it is, for that will help house everything we render. The only problem is during queries. We want to say, "don't start searching from the top-level container, only find the text 'within' this node."

[Dajust]

Okay, you could override the container during the render phase, but that doesn't change anything to what we want, because whatever container that is provided needs to 'contain' what was rendered before we can even start querying stuffs.

[sompylasar]

@Dajust Okay, if you want to query within a node that you got (how?), you could use dom-testing-library queries directly to provide that node as a container, not use the pre-baked query functions provided by render.

[Dajust]

Yeaa, makes sense! :smile:

But on a second thought, then that means we have to import both react-testing-library and dom-testing-library in our tests. That kinda kills the beauty of having react-test-library as a wrapper around dom-testing-library. (Same applies to other dependants)

Also, how about teachability? I was actually writing a thing on react-testing-library that inspired this suggestion. But now having to start explaining the limitations on react-testing-library and why you need to import the core dom-testing-library to text X kinda sucks. To be clear, it works just fine for me, but teaching it will suck, especially when I know we could effortlessly simplify that process. My thoughts though.

[sompylasar]

I don't have a lot of real teaching / course writing experience, but when I explain things to my colleagues, my goal is to make them understand how the thing works, not to abstract things away from them pretending there's nothing down there. I'd start with explaining that react-testing-library is a thin wrapper around dom-testing-library, which in turn is a thin wrapper around the DOM; their purpose is to avoid boilerplate by these nice abstractions for querying based on how a user uses the app, not on DOM structure.

[gnapse]

What about:

const sidebar = container.querySelector('.sidebar')
getByText(sidebar, 'some text')

You don't have to use the container obtained from render, you can query for some other part inside it and use it as the container to pass to getByText.

[kentcdodds]

Sorry, I don't have time to catch up on the thread, but I think it'd be totally cool to add some kind of helper function to make this more straightforward and easy for people. Even if it's very simple to workaround.

[kentcdodds]

I think this should work (on my phone):

import {render, queries} from 'react-testing-library'

// ....

const {getByTestId} = render(/* stuff */)
const lolMessage = queries.getByText(getByTestId('messages'), 'lol')

[kentcdodds]

And by that, I'm pretty sure that'll work right now.

[kentcdodds]

I think it'd maybe make more sense though to support an in or within option in the queries for this use case. May be a little more straightforward. Thoughts?

[gnapse]

I initially thought this would be unwarranted, but since the version of the query* and get* functions exported by react-testing-library's render do not receive the container and work implicitly with the render container element as the root, I think it makes sense.

Both in or within seem ok to me.

[kentcdodds]

I vote for within

Who wants to make the PR?

[kentcdodds]

What if we did:

const {getByTestId, within} = render(/* stuff */)

within(getByTestId('messages')).getByText('lol')

Reads more like a sentence I think... What's returned by within is all the queries with the given element as the container.

[kentcdodds]

I think I like that better

[gnapse]

I can do it, but only if @Dajust is not interested in doing it himself.

Regarding the other option you just suggested, what happens if you call within with a node that is node contained in the render container? Not that it'd be the common use case, but the way you pose that api, it could happen. It's an edge case though, we could ignore it or catch it with an error or something.

[kentcdodds]

lol, now that I think of it, within as written above is basically getQueriesForElement 😂

import {getQueriesForElement as within, render} from 'react-testing-library'

// ...

const {getByTestId} = render(/* stuff */)

within(getByTestId('messages')).getByText('lol')

Sooo.... Hmmm.... considering this, I'm thinking that no change is best and rather we should add examples/docs to help people avoid this question.

[gnapse]

It's always best when no api changes are needed. Documentation then, a FAQ entry maybe? Or something more explicit? In any case, I really think @Dajust should be the one doing it.

[kentcdodds]

I agree. @Dajust, what would have made this approach more obvious? How can we document this better?

[Dajust]

This works just fine for me @kentcdodds :smile: And now that I gave a broader thought, same is applicable to all queries, not just the "byTexts".

Docs: We could add a new section, maybe somewhere after the queryAll and getAll APIs and before the Exaples section, that documents within as an API. Then explain what it is, and when and how to use it. Something like:

within

Sometimes, there is no garauntee that the text, placeholder, or label you want to query is unique on the page. So you might want to explicity tell react-render-dom to get an element only within a particular section of the page. within is a helper function for this case.

Example: To get the text 'hello' only within a secton called 'messages', you could do:

import {getQueriesForElement as within, render} from 'react-testing-library'

// ...

const {getByTestId} = render(/* stuff */)
const messagesSection = getByTestId('messages')
const hello = within(messagesSection).getByText('hello')

Note that the actuall helper function here is getQueriesForElement. But we thought the name within better conveys the intention in this use case, so we preffer to import it as within.

[Dajust]

Umm... :thinking:
I just thought that within is not actually an API, it is just an alias. Instead of documenting it, we should be documenting getQueriesForElement. But the problem is getQueriesForElement doesn't quite convey the intention in this use case.

How about we export getQueriesForElement as within instead? The docs will make more sense. And we'll no longer need to add the additional note down there, after the example. Just my thoughts though.

[kentcdodds]

We could export it with both names as it makes sense as it's named for the current use case and makes more sense as within for the new use case. Then we can document it as you described.

[Dajust]

Can I help do this please?

[kentcdodds]

Please do!

[gnapse]

Wasn't this solved in #54? If so the issue should be closed manually.