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

getByText scoping and preference order when the text is not unique across the page #16

Closed
sompylasar opened this issue Apr 10, 2018 · 5 comments
Labels
needs discussion We need to discuss this to come up with a good solution question Further information is requested

Comments

@sompylasar
Copy link
Collaborator

  • dom-testing-library version: 1.4.0
  • node version: N/A
  • npm (or yarn) version: N/A

Relevant code or config

cy.getByText('Create')

What you did:

Used getByText with a text that is not unique across the page (which is often the case).

What happened:

Wrong element was found.

Reproduction repository:

N/A

Problem description:

When attempting to find an element by text, label, or other visual cues,

  1. some scoping is required (e.g. find an element with this text that is around the top right corner of the viewport; or, find an element below some other element)
  2. some ordering is required if there is still more than one element found after scoping

Suggested solution:

This issue is here to start a larger discussion, I've got no complete coded solution yet. It's closely related to Cypress but visual element finding is DOM-specific, not specific to Cypress, as proven by my proof-of-concept powered by the same Guiding Principles that uses Puppeteer.

In the mentioned proof-of-concept I experimented with visual element finding and came to an API that allows finding more than one element around some viewport point and filter them after that (potentially, with a text matcher, but the filter is generic). https://github.com/sompylasar/puppeye/blob/d01aa117d3bacf8a458b03dffebef081380b54d9/test/smoke.test.ts#L145-L183

        const checkboxLabelCenterLeftPagePoint = {
          x: checkboxLabelElement.pageRect.left,
          y: checkboxLabelElement.centerPagePoint.y,
        } as puppeye.TPagePoint;
        const [checkboxElement] = await puppeye.findElements(
          pageModel,
          puppeye.makeElementsAroundFinder({
            viewportPoint: puppeye.convertPagePointToViewportPoint(
              pageModel.getPageState(),
              checkboxLabelCenterLeftPagePoint,
            ),
            filter: (el) =>
              el.viewportRect.area > 10 * 10 &&
              el.viewportRect.area < 40 * 40 &&
              el.viewportRect.ratio > 0.9,
            maxDistancePx: 100,
          }),
        );

        await puppeye.moveMouseIntoElementRect(pageModel, checkboxElement);

        await puppeye.clickElement(pageModel, checkboxElement);

        const [continueButton] = await puppeye.findElements(
          pageModel,
          puppeye.makeElementsBelowFinder({
            viewportPoint: puppeye.convertPagePointToViewportPoint(
              pageModel.getPageState(),
              checkboxLabelElement.centerPagePoint,
            ),
            filter: (el) =>
              el.isInteractive && el.textNormalized === puppeye.normalizeText('Continue'),
          }),
        );
        expect(continueButton).toMatchObject({
          text: 'Continue',
        });

        await puppeye.clickElement(pageModel, continueButton);

Cypress has some heuristics for ordering for the similar functionality they have (contains) described here: https://docs.cypress.io/api/commands/contains.html#Preferences – but the only scoping they have is based on DOM selectors. Cypress has something similar, coordinate-based, for the click action (click different parts of rectangles) but doesn't have that for scoping and ordering of elements.

@kentcdodds
Copy link
Member

How about: getByText(getByTestId('some-container'), 'Create')? Nothing about dom-testing-library says that the container has to be the document. Perhaps cypress-testing-library could expose an additional container parameter so you could do:

cy.getByText('Create', {container: cy.getByTestId('some-container')})

Would that work?

@kentcdodds kentcdodds added needs discussion We need to discuss this to come up with a good solution question Further information is requested labels Apr 10, 2018
@sompylasar
Copy link
Collaborator Author

How about: getByText(getByTestId('some-container'), 'Create')? Nothing about dom-testing-library says that the container has to be the document.

No because I'd like to avoid data-testid altogether.

Perhaps cypress-testing-library could expose an additional container parameter

Yes, this is on my TODO to expose waitForElementOptions.

@kentcdodds
Copy link
Member

No because I'd like to avoid data-testid altogether.

Well, the user has some way of knowing which "Create" button to click. If that method is visual-only (or similar for screen-reader users), then you're going to have a really hard time simulating that with a visual analysis tool of some kind. That's definitely outside the scope of this tool.

@sompylasar
Copy link
Collaborator Author

That's definitely outside the scope of this tool.

I had a glimmer of hope when you started "the Guiding Principles", but now it's gone. Only low-hanging fruit cases are in the scope. Thanks.

@kentcdodds
Copy link
Member

Hey now @sompylasar. I'm really sorry that the scope of this project disappoints you. I'm afraid that testing is a means to an end for me and not an end in itself. There are companies that have been trying to solve this problem for years now. It's not an easy problem. We have to make trade-offs unless we want to spend our lives trying to solve the problem. This library (and every abstraction) needs to make trade-offs and draw lines somewhere.

The guiding principle is just that, a guide and a principle. It's not a hard fast rule. It simply states that the more your tests resemble the way your software is used, the more confidence they can give you. But investing the time and effort into a solution that mimics user behavior perfectly is not only challenging but literally impossible to automate in a way that's acceptable from a usability standpoint as well as a time constraints perspective.

Because of this, we have to decide where we'll make certain decisions that diverge from the guiding principle in the name of shipping software and making the world a better place. And honestly, I think that the difference in confidence from what we have in this library and what you could get from a tool that makes fewer diversions from that principle like you're talking about is pretty minimal. Like, a sliver more confidence, but a HUGE amount of work.

Calling what we're doing "low-hanging fruit" is disingenuous though (and honestly frustrates me). What we've accomplished is fairly simple in implementation, but has already consumed dozens of hours of my own time and many more hours of other people's time, in addition to leveraging all that I've learned about testing over the years. Any "fruit" higher hanging than what we're already doing is so far up the proverbial tree that it makes it completely impractical to even try, at least given my time constraints.

I'm not stopping you from following your own dreams here. Feel free to pursue them. But I'm afraid that I'll only be a pleasant user of your software if you succeed, because I have stuff to ship and can't spend all my time working on how to get that last sliver of confidence I'm getting from my current testing solution.

Thank you for your contributions, and I hope you continue to use, enjoy, and contribute to this library. Good luck.

alexkrolick pushed a commit to alexkrolick/dom-testing-library that referenced this issue Sep 13, 2018
…library#17)

**What**: Add the following methods

- queryByText
- getByText
- queryByPlaceholderText
- getByPlaceholderText
- queryByLabelText
- getByLabelText

**Why**: Closes testing-library#16

These will really improve the usability of this module. These also align much better with the guiding principles 👍

**How**:

- Created a `queries.js` file where we have all the logic for the queries and their associated getter functions
- Migrate tests where it makes sense
- Update docs considerably.

**Checklist**:

* [x] Documentation
* [x] Tests
* [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
* [ ] Added myself to contributors table N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion We need to discuss this to come up with a good solution question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants