Skip to content

Improving api #13

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

Merged
merged 15 commits into from
Mar 28, 2018
Merged
10 changes: 10 additions & 0 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@
"contributions": [
"platform"
]
},
{
"login": "antoaravinth",
"name": "Anto Aravinth",
"avatar_url": "https://avatars1.githubusercontent.com/u/1241511?s=460&v=4",
"profile": "https://github.com/antoaravinth",
"contributions": [
"code",
"test"
]
}
]
}
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
[![downloads][downloads-badge]][npmtrends]
[![MIT License][license-badge]][license]

[![All Contributors](https://img.shields.io/badge/all_contributors-7-orange.svg?style=flat-square)](#contributors)
[![All Contributors](https://img.shields.io/badge/all_contributors-8-orange.svg?style=flat-square)](#contributors)
[![PRs Welcome][prs-badge]][prs]
[![Code of Conduct][coc-badge]][coc]

Expand Down Expand Up @@ -564,6 +564,7 @@ Thanks goes to these people ([emoji key][emojis]):
<!-- prettier-ignore -->
| [<img src="https://avatars.githubusercontent.com/u/1500684?v=3" width="100px;"/><br /><sub><b>Kent C. Dodds</b></sub>](https://kentcdodds.com)<br />[💻](https://github.com/kentcdodds/react-testing-library/commits?author=kentcdodds "Code") [📖](https://github.com/kentcdodds/react-testing-library/commits?author=kentcdodds "Documentation") [🚇](#infra-kentcdodds "Infrastructure (Hosting, Build-Tools, etc)") [⚠️](https://github.com/kentcdodds/react-testing-library/commits?author=kentcdodds "Tests") | [<img src="https://avatars1.githubusercontent.com/u/2430381?v=4" width="100px;"/><br /><sub><b>Ryan Castner</b></sub>](http://audiolion.github.io)<br />[📖](https://github.com/kentcdodds/react-testing-library/commits?author=audiolion "Documentation") | [<img src="https://avatars0.githubusercontent.com/u/8008023?v=4" width="100px;"/><br /><sub><b>Daniel Sandiego</b></sub>](https://www.dnlsandiego.com)<br />[💻](https://github.com/kentcdodds/react-testing-library/commits?author=dnlsandiego "Code") | [<img src="https://avatars2.githubusercontent.com/u/12592677?v=4" width="100px;"/><br /><sub><b>Paweł Mikołajczyk</b></sub>](https://github.com/Miklet)<br />[💻](https://github.com/kentcdodds/react-testing-library/commits?author=Miklet "Code") | [<img src="https://avatars3.githubusercontent.com/u/464978?v=4" width="100px;"/><br /><sub><b>Alejandro Ñáñez Ortiz</b></sub>](http://co.linkedin.com/in/alejandronanez/)<br />[📖](https://github.com/kentcdodds/react-testing-library/commits?author=alejandronanez "Documentation") | [<img src="https://avatars0.githubusercontent.com/u/1402095?v=4" width="100px;"/><br /><sub><b>Matt Parrish</b></sub>](https://github.com/pbomb)<br />[🐛](https://github.com/kentcdodds/react-testing-library/issues?q=author%3Apbomb "Bug reports") [💻](https://github.com/kentcdodds/react-testing-library/commits?author=pbomb "Code") [📖](https://github.com/kentcdodds/react-testing-library/commits?author=pbomb "Documentation") [⚠️](https://github.com/kentcdodds/react-testing-library/commits?author=pbomb "Tests") | [<img src="https://avatars1.githubusercontent.com/u/1288694?v=4" width="100px;"/><br /><sub><b>Justin Hall</b></sub>](https://github.com/wKovacs64)<br />[📦](#platform-wKovacs64 "Packaging/porting to new platform") |
| :---: | :---: | :---: | :---: | :---: | :---: | :---: |
| [<img src="https://avatars1.githubusercontent.com/u/1241511?s=460&v=4" width="100px;"/><br /><sub><b>Anto Aravinth</b></sub>](https://github.com/antoaravinth)<br />[💻](https://github.com/kentcdodds/react-testing-library/commits?author=antoaravinth "Code") [⚠️](https://github.com/kentcdodds/react-testing-library/commits?author=antoaravinth "Tests") |

<!-- ALL-CONTRIBUTORS-LIST:END -->

Expand Down
7 changes: 7 additions & 0 deletions extend-expect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const expect = require('expect') //eslint-disable-line import/no-extraneous-dependencies
const extensions = require('./dist/jest-extensions')

const {toBeInTheDOM, toHaveTextContent, toSatisfyDOM} = extensions.default
expect.extend({toBeInTheDOM, toHaveTextContent, toSatisfyDOM})

module.exports = expect
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
"eslintConfig": {
"extends": "./node_modules/kcd-scripts/eslint.js",
"rules": {
"react/prop-types": "off"
"react/prop-types": "off",
"import/no-unassigned-import": "off"
}
},
"eslintIgnore": [
Expand All @@ -61,4 +62,4 @@
"url": "https://github.com/kentcdodds/react-testing-library/issues"
},
"homepage": "https://github.com/kentcdodds/react-testing-library#readme"
}
}
15 changes: 15 additions & 0 deletions src/__tests__/element-queries.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react'
import {render} from '../'
import '../../extend-expect'
Copy link
Member

Choose a reason for hiding this comment

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

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' 👍


test('query can return null', () => {
const {
Expand Down Expand Up @@ -66,4 +67,18 @@ test('totally empty label', () => {
expect(() => getByLabelText('')).toThrowErrorMatchingSnapshot()
})

test('using jest helpers to assert element states', () => {
const {queryByTestId, getByTestId} = render(
<span data-testid="count-value">2</span>,
)

//other ways to assert your test cases, but you don't need all of them.
Copy link
Member

Choose a reason for hiding this comment

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

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

expect(queryByTestId('count-value')).toBeInTheDOM()
expect(queryByTestId('count-value')).toHaveTextContent('2')
expect(getByTestId('count-value')).toSatisfyDOM(el => el.textContent === '2')
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

expect(queryByTestId('count-value')).not.toHaveTextContent(
'you are not there',
)
})

/* eslint jsx-a11y/label-has-for:0 */
2 changes: 1 addition & 1 deletion src/__tests__/fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ test('Fetch makes an API call and displays the greeting when load-greeting is cl
}),
)
const url = '/greeting'
const {getByText, container} = render(<Fetch url={url} />)
const {container, getByText} = render(<Fetch url={url} />)

// Act
Simulate.click(getByText('Fetch'))
Expand Down
102 changes: 102 additions & 0 deletions src/jest-extensions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import {matcherHint, printReceived, printExpected} from 'jest-matcher-utils' //eslint-disable-line import/no-extraneous-dependencies

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

const assertMessage = (assertionName, message, received, expected) => () =>
`${matcherHint(`${assertionName}`, 'received', '')} \n${message}: ` +
`${printExpected(expected)} \nReceived: ${printReceived(received)}`

const extensions = {
toBeInTheDOM(received) {
getDisplayName(received)
if (received) {
return {
message: () =>
`${matcherHint(
'.not.toBeInTheDOM',
'received',
'',
)} Expected the element not to be present` +
`\nReceived : ${printReceived(received)}`,
pass: true,
}
} else {
return {
message: () =>
`${matcherHint(
'.toBeInTheDOM',
'received',
'',
)} Expected the element to be present` +
`\nReceived : ${printReceived(received)}`,
pass: false,
}
}
},

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

const textContent = htmlElement.textContent
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

const pass = textContent === checkWith
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@antsmartian antsmartian Mar 27, 2018

Choose a reason for hiding this comment

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

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)

if (pass) {
return {
message: assertMessage(
'.not.toHaveTextContent',
'Expected value not equals to',
htmlElement,
checkWith,
),
pass: true,
}
} else {
return {
message: assertMessage(
'.toHaveTextContent',
'Expected value equals to',
htmlElement,
checkWith,
),
pass: false,
}
}
},

toSatisfyDOM(actual, predicate) {
const pass = predicate(actual)
if (pass) {
return {
message: assertMessage(
'.not.toSatisfyDOM()',
'Expected predicate not equals to true',
actual,
predicate,
),
pass: true,
}
} else {
return {
message: assertMessage(
'.not.toSatisfyDOM()',
'Expected predicate equals to true',
actual,
predicate,
),
pass: false,
}
}
},
}

export default extensions