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

feat: Add getByTestId utility #10

Merged
merged 3 commits into from
Mar 21, 2018

Conversation

pbomb
Copy link
Collaborator

@pbomb pbomb commented Mar 21, 2018

What:

This adds the getByTestId utility function.

Why:

To work better with typical use case of expecting queried elements to exist instead of potentially being null.

How:

queryByTestId still exists and still can return null, but getByTestId is added that will throw an Error instead of returning null. For consumers using Flow or TypeScript, using this new function will not complain about directly using the returned element, since it cannot return null.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@codecov
Copy link

codecov bot commented Mar 21, 2018

Codecov Report

Merging #10 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #10   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           6     10    +4     
  Branches        0      1    +1     
=====================================
+ Hits            6     10    +4
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf3ab6...52bc0eb. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super! Thank you! Just a few docs related changes.

README.md Outdated
expect(hiddenItemElement).toBeFalsy() // we just care it doesn't exist
```

#### `getByTestId`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this above queryByTestId to indicate that it's recommended.

README.md Outdated
```

## More on `data-testid`s

The `queryByTestId` utility is referring to the practice of using `data-testid`
The `queryByTestId` and `getByTestId` utilities refer to the practice of using `data-testid`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swap the order here to give preferential treatment to getByTestId

README.md Outdated
@@ -286,7 +299,7 @@ Or you could include the index or an ID in your attribute:
<li data-testid={`item-${item.id}`}>{item.text}</li>
```

And then you could use the `queryByTestId`:
And then you could use the `queryByTestId` utility:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you switch this to getByTestId?

Also, could you add a FAQ for: What if I want to verify that an element does NOT exist? Where we show how to use queryByTestId as well as container.querySelector as possible solutions.

@pbomb
Copy link
Collaborator Author

pbomb commented Mar 21, 2018

Thanks, @kentcdodds I'll implement your suggestions this afternoon.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super! Thank you!

function getByTestId(div, id) {
const el = queryByTestId(div, id)
if (!el) {
throw new Error(`Unable to find element by ${select(id)}`)
Copy link
Collaborator

@antsmartian antsmartian Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kentcdodds Oops! If now getByTestId throws error, then not of toBeInTheDOM (which I'm working on it) wouldn't make sense right? Because people will use toThrowError for their assertion instead of not. So toBeInTheDOM is only for +ve case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd thought of that. It still works for queryByTestId though.

julienw pushed a commit to julienw/react-testing-library that referenced this pull request Dec 20, 2018
…ary#10)

* feat(waitForElements): add implementation, tests, docs

Add `mutationobserver-shim` to `devDependencies` to provide
`MutationObserver` in jest tests where `jsdom` has no built-in support
for it: jsdom/jsdom#639

* docs(contributors): update sompylasar

* CR changes

- rename `waitForElements` to `waitForElement`
- move `mutationobserver-shim` to dependencies, import from
`waitForElement` to provide the polyfill to the users of
`dom-testing-library`
- fix `kcd-scripts` version to match `master` branch
- add synchronous `callback` call to detect an element if it's already
present before any DOM mutation happens
- add/change tests about synchronous `callback` call
- tweak variable names in docs examples
- add docs about the default `container` option value
- add docs example about querying multiple elements
- add docs about the `mutationobserver-shim` polyfill
- add docs link and anchor to `mutationObserverOptions`
- add docs link to MDN from the second mention of `MutationObserver`

* fix(waitForElement): ensure it works with default callback

Should wait for the next DOM change, as advertised in the docs.

The default value is `undefined` so that the `options` object can be
used while still keeping the default callback:
```
waitForElement(undefined, {attributes: true})
```

* CR: tweak docs examples for wait and waitForElement

- use `container` in the examples as this is a more popular use case
than the default value of global `document`
- use full sentences with capital first letter and period in the
example comments

* CR: rename files to kebab-case

* CR: await promise -> return promise

@kentcdodds:
> Rather than `await promise`, I'd prefer `return promise`. Maybe I'm
being irrational here, but it feels better to me.

@sompylasar:
> I'm changing this, but if this line was the only one with `await`
expression, then `eslint` would say `async` function must have an
`await`. We are lucky that there are more `await`s in all the tests.
>
> P.S. I don't agree with this rule because `async` functions have
their use for the error handling; `async` function is just the one that
is wrapped in a `return new Promise(...)`.

* CR: shorter timeouts and wait times for quicker tests
lucbpz pushed a commit to lucbpz/react-testing-library that referenced this pull request Jul 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants