Improving api #13

antsmartian · 2018-03-22T14:17:04Z

What: Improving test API

Why: Discussed already here : #3

How: Again as discussed :)

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

Related to this thread : #3

kentcdodds

Looking very good! Thanks! Just a few comments. Let's iterate and make this really amazing!

kentcdodds · 2018-03-22T16:48:40Z

src/__tests__/fetch.js


  // Act
  Simulate.click(getByTestId('load-greeting'))

  await flushPromises()

  // Assert
+  expect(queryByTestId('foo')).not.toBeInTheDOM()


I think people will look to these tests as an example of how to test their components with good practices.

With that in mind, let's do one of the following:

Add a comment suggesting: "Here are a few assertions you could use, but you don't need all of them."

Move these to a separate testing file that's intended to show off the different kinds of assertions.

I don't really care which we do.

kentcdodds · 2018-03-22T16:49:18Z

src/__tests__/fetch.js

@@ -1,6 +1,7 @@
 import React from 'react'
 import axiosMock from 'axios'
 import {render, Simulate, flushPromises} from '../'
+import '../../extend-expect' //eslint-disable-line import/no-unassigned-import


Let's just disable this rule globally. It really annoys me and I should remove it from my base config eventually...

In the package.json, update the eslintConfig to have a rules property that disables this rule 👍

kentcdodds · 2018-03-22T16:55:35Z

src/jest-extensions.js

+  toBeInTheDOM(received) {
+    if (received) {
+      return {
+        message: () => 'expected the dom to be present',


Rather than calling it "dom" call it "element." The received is an element, not "dom" 👍

Same below.

kentcdodds · 2018-03-22T17:39:43Z

src/jest-extensions.js

+    const pass = textContent === checkWith
+    if (pass) {
+      return {
+        message: () => `expected ${textContent} not equals to ${checkWith}`,


Let's use jest-matcher-utils here so it's colored properly.

kentcdodds · 2018-03-22T17:44:47Z

src/jest-extensions.js

+  },
+
+  toHaveTextContent(htmlElement, checkWith) {
+    const textContent = htmlElement.textContent


Could we add an extra check here that htmlElement is actually instanceof HTMLElement? If not we can:

throw new Error(`The given subject is a ${getDisplayName(htmlElement)}, not an HTMLElement`)

Or return an object, but this should probably fail regardless of whether people use .not.

The getDisplayName function could be something like this:

function getDisplayName(subject) { if (subject && subject.constructor) { return subject.constructor.name } else { return typeof subject } }

Or, maybe there's a better thing for this available on npm...

I guess the code that you had mentioned getDisplayName is enough. npm packages are there, but they offer many functionalities which we won't need at this time.

kentcdodds · 2018-03-22T17:44:57Z

src/jest-extensions.js

+      }
+    } else {
+      return {
+        message: () => `expected ${textContent} equals to ${checkWith}`,


Let's use jest-matcher-utils here so it's colored properly.

kentcdodds · 2018-03-22T17:47:14Z

src/jest-extensions.js

+  },
+
+  toSatisfyDOM(element, fn) {
+    const pass = fn(element)


I think this should be more like this: https://github.com/jest-community/jest-extended/blob/master/src/matchers/toSatisfy/index.js

In fact, it looks like we should probably be changing everything in this file to use jest-matcher-utils so it's colored properly etc. Unfortunately it looks like there are no docs, but here's the source.

…eries

antsmartian · 2018-03-24T14:43:09Z

@kentcdodds Hey, have addressed your comments also I have moved our assertions to element-queries, I guess now that's the apt place. Also all the assertions are moved to jest-color, so we get nice color when things get failed :) Let me know if everything is good to go..

kentcdodds

Looking awesome. Please address my few comments and let's add a section for these to the README please :) Thanks a ton!

kentcdodds · 2018-03-27T04:24:22Z

src/__tests__/element-queries.js

@@ -1,5 +1,6 @@
 import React from 'react'
 import {render} from '../'
+import '../../extend-expect'


The problem with this is it means you have to run the build before you can run the tests (because on a fresh clone the dist directory doesn't work). It also means that any changes to jest-extensions.js wont be picked up by tests until the next build.

Instead, you'll basically need to duplicate the code.

Alternatively... You could put the code that's currently in extend-expect.js into src/extend-expect.js and then change the contents of: extend-expect.js to be simply: require('./dist/extend-expect'). Then you could update this import to simply: '../extend-expect' 👍

kentcdodds · 2018-03-27T04:24:46Z

src/__tests__/element-queries.js

+    <span data-testid="count-value">2</span>,
+  )
+
+  //other ways to assert your test cases, but you don't need all of them.


Not sure why prettier didn't add a space here, but please add a space before the o :)

kentcdodds · 2018-03-27T04:26:15Z

src/__tests__/element-queries.js

+  //other ways to assert your test cases, but you don't need all of them.
+  expect(queryByTestId('count-value')).toBeInTheDOM()
+  expect(queryByTestId('count-value')).toHaveTextContent('2')
+  expect(getByTestId('count-value')).toSatisfyDOM(el => el.textContent === '2')


I'm starting to question the value of this matcher. Maybe instead we should just recommend people use jest-extend instead. This is basically the same as toSatisfy from there.

The other matchers are still very valuable though!

@kentcdodds toSatisfyDOM seems to be a fit for me in this library. Why we need to ask user to use jest-extend for a such a simple call? Yes, its almost the same code, but I feel it should be added. Anyways, let me know if you think otherwise.

I agree that this matcher can be useful, but I'd much rather be very selective about what we include in this library. It's much easier to exclude something and add it later than add something and remove it later. So I lean on the side of excluding things. I'm thinking most people wont need this and those who do will be happy to include jest-extend I think.

kentcdodds · 2018-03-27T15:16:46Z

src/jest-extensions.js

+      )
+
+    const textContent = htmlElement.textContent
+    const pass = textContent === checkWith


Could we extend this to use the matcher function that's in the queries.js file? Perhaps we should move that matcher function to a utils file.

Using the matcher function would allow the toHaveTextContent to use a regex/case insensitive substring/function which I think would make this more powerful.

Yes, thats a good idea. Can take care of that, will move matcher to utils folder. Import matcher in queries.js and jest-extension.js files to use them. So fundamentally the call would be:

const pass = matches(textContent,htmlElement,checkWith)

antsmartian · 2018-03-27T15:25:43Z

@kentcdodds Sure, will look into the changes and update the PR.

kentcdodds · 2018-03-27T15:39:35Z

Also, let's figure out what what extra coverage we need for our tests. Open coverage/lcov-report/index.html in your browser to see the coverage report.

antsmartian · 2018-03-27T15:53:00Z

@kentcdodds Yes, jest-extensions.js need to be tested. I can write few test cases for that. Seems to be a very interesting discussion on this PR, long thread!

antsmartian · 2018-03-27T16:05:27Z

@kentcdodds Update the PR. Few things that are pending:

Add test cases for jest-extensions
Write few more assertions for toHaveTextContent with regex etc, to show what user can actually do.
Update the README on the following:

toHaveTextContent
toBeInTheDOM

their use cases etc.

Anything else is missing?

kentcdodds

Looks like we still need docs in the README and we're not at 100% code coverage.

Also, there's a merge conflict in the README and .all-contributorsrc. If you just deal with the conflict in .all-contributorsrc then run npx kcd-scripts contributors generate then that should fix the README 👍

Thank you so much for iterating on this!

kentcdodds · 2018-03-27T22:40:39Z

src/utils.js

@@ -0,0 +1,9 @@
+export default function matches(textToMatch, node, matcher) {


Rather than exporting this as the default (because this file is called utils), could we make it a named export (and feel free to disable the eslint warning, that's another one I've been meaning to disable).

function matches() {/* etc... */} export {matches}

antsmartian · 2018-03-28T01:25:16Z

@kentcdodds This should be good to go. Let me know if anything else is needed.

codecov · 2018-03-28T01:27:30Z

Codecov Report

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

@@          Coverage Diff          @@
##           master    #13   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      5    +3     
  Lines          53     72   +19     
  Branches       13     17    +4     
=====================================
+ Hits           53     72   +19

Impacted Files	Coverage Δ
src/queries.js	`100% <ø> (ø)`	⬆️
src/utils.js	`100% <100%> (ø)`
src/extend-expect.js	`100% <100%> (ø)`
src/jest-extensions.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 8403e84...e222206. Read the comment docs.

kentcdodds

This is great! Thank you so much for these changes! Just a few docs changes please.

kentcdodds · 2018-03-28T02:29:40Z

README.md

+The first one `toBeInTheDOM` which allows you to assert whether an element present in the DOM or not
+
+```javascript
+import 'extend-expect' //adds few API to jest's extend


This will be import 'react-testing-library/extend-expect'

kentcdodds · 2018-03-28T02:31:14Z

README.md

@@ -219,6 +219,31 @@ const usernameInputElement = getByTestId('username-input')
 > Learn more about `data-testid`s from the blog post
 > ["Making your UI tests resilient to change"][data-testid-blog-post]

+#### `toBeInTheDOM`


Could we put these in a different section of the README (maybe right where it is).

So just change this to:

## Custom Jest Matchers There are two simple custom Jest matchers which you can use to extend the `expect` API of Jest for making assertions easier. ### `toBeInTheDOM` This allows you to assert whether or not an element present in the DOM ... etc.

kentcdodds · 2018-03-28T02:31:45Z

README.md

+This API allows you to check whether the given element has a text content or not.
+
+```javascript
+import 'extend-expect' //adds few API to jest's extend


Here as well. Also, please add a space after the //

kentcdodds · 2018-03-28T02:32:21Z

README.md

+
+render(<span data-testid="count-value">2</span>)
+expect(queryByTestId('count-value')).toHaveTextContent('2')
+expect(queryByTestId('count-value')).not.toHaveTextContent('21')


For these two examples, could we use getByTestId rather than queryByTestId?

kentcdodds · 2018-03-28T02:33:26Z

README.md

+
+render(<span data-testid="count-value">2</span>)
+expect(queryByTestId('count-value')).toBeInTheDOM()
+expect(queryByTestId('count-value1')).not.toBeInTheDOM()


Could you add a comment below that says: "Note: for this assertion, make sure you use a query function (like queryByTestId) rather than a get function (like getByTestId). Otherwise the get function could throw an error.

kentcdodds · 2018-03-28T02:33:45Z

extend-expect.js

@@ -0,0 +1 @@
+require('./dist/extend-expect')


antsmartian · 2018-03-28T02:42:44Z

Sure Kent, will make these changes in sometime and update the PR.

antsmartian · 2018-03-28T11:02:29Z

@kentcdodds Addressed your comments. Let me know if anything else is needed?

kentcdodds

This is great! Thanks!

kentcdodds · 2018-03-29T16:16:10Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

* added fireEvent util * remove react logic * 💯 coverage * Update README.md

chore: 🤖 add dom-testing-library as npm keyword

abelginrayen added 4 commits March 22, 2018 19:40

Improving API's for testing.

5a2c05f

Improving Apis

dd727d0

Adding all contributions

485e8fb

Fixing wrong url

96a1a9e

kentcdodds reviewed Mar 22, 2018

View reviewed changes

Anto Aravinth added 2 commits March 24, 2018 14:53

Fixing review comments & making colorful assertions :)

cc2a1c3

Rebasing with latest code and moving the new assertions to element-qu…

3931a69

…eries

Removing unwanted changes

3ae81f6

kentcdodds reviewed Mar 27, 2018

View reviewed changes

Anto Aravinth added 2 commits March 27, 2018 21:30

Fixing review comments

987c4f3

removing unwanted comments

57fd424

kentcdodds reviewed Mar 27, 2018

View reviewed changes

Anto Aravinth added 2 commits March 28, 2018 06:39

Adding test cases for the coverage

d1157a2

Adding in documentation for API's

0cdb02b

removing commented code and making few changes to the contribution file

74d8205

kentcdodds reviewed Mar 28, 2018

View reviewed changes

abelginrayen added 2 commits March 28, 2018 16:27

Updating the readme

8348852

Making line break changes

492645b

Update README.md

e222206

kentcdodds approved these changes Mar 28, 2018

View reviewed changes

kentcdodds merged commit 18104fe into testing-library:master Mar 28, 2018

julienw pushed a commit to julienw/react-testing-library that referenced this pull request Dec 20, 2018

feat(fireEvent): add fireEvent util (testing-library#13)

d45b449

* added fireEvent util * remove react logic * 💯 coverage * Update README.md

lucbpz pushed a commit to lucbpz/react-testing-library that referenced this pull request Jul 26, 2020

Merge pull request testing-library#13 from Gpx/keywords

57f860a

chore: 🤖 add dom-testing-library as npm keyword

		@@ -0,0 +1,9 @@
		export default function matches(textToMatch, node, matcher) {

		@@ -0,0 +1 @@
		require('./dist/extend-expect')

Improving api #13

Improving api #13

Conversation

antsmartian commented Mar 22, 2018 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Mar 24, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

antsmartian commented Mar 27, 2018

kentcdodds commented Mar 27, 2018

antsmartian commented Mar 27, 2018 • edited Loading

antsmartian commented Mar 27, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Mar 28, 2018

codecov bot commented Mar 28, 2018 • edited Loading

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Mar 28, 2018

antsmartian commented Mar 28, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Mar 29, 2018

antsmartian commented Mar 22, 2018 •

edited

Loading

antsmartian Mar 27, 2018 •

edited

Loading

antsmartian commented Mar 27, 2018 •

edited

Loading

codecov bot commented Mar 28, 2018 •

edited

Loading