Skip to content

feat(timeout): allow to configure the timeout for queries #7

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.

Sign up for GitHub

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 2 commits into from
Jun 2, 2018
Merged

feat(timeout): allow to configure the timeout for queries #7

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3925c9
feat(timeout): allow to configure the timeout for queries
npeterkamps May 26, 2018
8c9c766
Pass all args to `queryImpl`, pass all args with some defaults to `wa…
npeterkamps Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .all-contributorsrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@
"code",
"test"
]
},
{
"login": "npeterkamps",
"name": "Peter Kamps",
"avatar_url": "https://avatars1.githubusercontent.com/u/25429764?v=4",
"profile": "https://github.com/npeterkamps",
"contributions": [
"code",
"doc",
"ideas"
]
}
],
"repoType": "github"
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ dist
.opt-out
.DS_Store
.eslintcache
.idea/

# these cause more harm than good
# when working with contributors
Expand Down
9 changes: 4 additions & 5 deletions README.md
View file
Open in desktop
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-3-orange.svg?style=flat-square)](#contributors)
[![All Contributors](https://img.shields.io/badge/all_contributors-4-orange.svg?style=flat-square)](#contributors)
[![PRs Welcome][prs-badge]][prs]
[![Code of Conduct][coc-badge]][coc]

Expand Down Expand Up @@ -73,6 +73,7 @@ To show some simple examples (from [cypress/integration/commands.spec.js](cypres
cy.getAllByText('Jackie Chan').click()
cy.queryByText('Button Text').should('exist')
cy.queryByText('Non-existing Button Text').should('not.exist')
cy.queryByLabelText('Label text', { timeout: 7000 }).should('exist')
```

## Other Solutions
Expand All @@ -85,11 +86,9 @@ here!
Thanks goes to these people ([emoji key][emojis]):

<!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->

<!-- 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/cypress-testing-library/commits?author=kentcdodds "Code") [📖](https://github.com/kentcdodds/cypress-testing-library/commits?author=kentcdodds "Documentation") [🚇](#infra-kentcdodds "Infrastructure (Hosting, Build-Tools, etc)") [⚠️](https://github.com/kentcdodds/cypress-testing-library/commits?author=kentcdodds "Tests") | [<img src="https://avatars2.githubusercontent.com/u/498274?v=4" width="100px;"/><br /><sub><b>Ivan Babak</b></sub>](https://sompylasar.github.io)<br />[💻](https://github.com/kentcdodds/cypress-testing-library/commits?author=sompylasar "Code") [🤔](#ideas-sompylasar "Ideas, Planning, & Feedback") | [<img src="https://avatars1.githubusercontent.com/u/4002543?v=4" width="100px;"/><br /><sub><b>Łukasz Gandecki</b></sub>](http://team.thebrain.pro)<br />[💻](https://github.com/kentcdodds/cypress-testing-library/commits?author=lgandecki "Code") [⚠️](https://github.com/kentcdodds/cypress-testing-library/commits?author=lgandecki "Tests") |
| :---: | :---: | :---: |

| [<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/cypress-testing-library/commits?author=kentcdodds "Code") [📖](https://github.com/kentcdodds/cypress-testing-library/commits?author=kentcdodds "Documentation") [🚇](#infra-kentcdodds "Infrastructure (Hosting, Build-Tools, etc)") [⚠️](https://github.com/kentcdodds/cypress-testing-library/commits?author=kentcdodds "Tests") | [<img src="https://avatars2.githubusercontent.com/u/498274?v=4" width="100px;"/><br /><sub><b>Ivan Babak</b></sub>](https://sompylasar.github.io)<br />[💻](https://github.com/kentcdodds/cypress-testing-library/commits?author=sompylasar "Code") [🤔](#ideas-sompylasar "Ideas, Planning, & Feedback") | [<img src="https://avatars1.githubusercontent.com/u/4002543?v=4" width="100px;"/><br /><sub><b>Łukasz Gandecki</b></sub>](http://team.thebrain.pro)<br />[💻](https://github.com/kentcdodds/cypress-testing-library/commits?author=lgandecki "Code") [⚠️](https://github.com/kentcdodds/cypress-testing-library/commits?author=lgandecki "Tests") | [<img src="https://avatars1.githubusercontent.com/u/25429764?v=4" width="100px;"/><br /><sub><b>Peter Kamps</b></sub>](https://github.com/npeterkamps)<br />[💻](https://github.com/kentcdodds/cypress-testing-library/commits?author=npeterkamps "Code") [📖](https://github.com/kentcdodds/cypress-testing-library/commits?author=npeterkamps "Documentation") [🤔](#ideas-npeterkamps "Ideas, Planning, & Feedback") |
| :---: | :---: | :---: | :---: |
<!-- ALL-CONTRIBUTORS-LIST:END -->

This project follows the [all-contributors][all-contributors] specification.
Expand Down
4 changes: 4 additions & 0 deletions src/__tests__/__snapshots__/commands.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,9 @@ Array [
"queryAllByTitle",
"getByTitle",
"getAllByTitle",
"queryByValue",
"queryAllByValue",
"getByValue",
"getAllByValue",
]
`;
19 changes: 14 additions & 5 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ import {queries, waitForElement} from 'dom-testing-library'
const commands = Object.keys(queries).map(queryName => {
return {
name: queryName,
command: (cy, ...args) => {
command: (cy, text, options = {}) => {
const { timeout = 3000 } = options;
const queryImpl = queries[queryName]
const baseCommandImpl = doc =>
waitForElement(() => queryImpl(doc, ...args), {
waitForElement(() => queryImpl(doc, text, options), {
Copy link
Collaborator

@sompylasar sompylasar May 30, 2018

Choose a reason for hiding this comment

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

This call isn't quite safe. What if queryImpl takes other set of arguments, not just text followed by options? The original idea was to propagate all the query options verbatim to queryImpl. I would consider queryImpl.length (the function arity) to see how many arguments it actually takes, and passed the respective slice of args to it; if there's one more argument remaining, that's our Cypress command options with timeout and potentially other stuff such as log: false.

Relying on function arity may be unreliable if the queryImpl uses variable number of ...args (i.e. arguments) instead of defining all arguments explicitly.

Copy link
Collaborator Author

@npeterkamps npeterkamps May 31, 2018

Choose a reason for hiding this comment

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

Good point. However, I prefer to have a single options object, so there's no need to keep in mind which options go into which object. Do you agree?

Copy link
Member

@kentcdodds kentcdodds May 31, 2018

Choose a reason for hiding this comment

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

I prefer the ...args for stuff like this if we can get away with it.

Copy link
Collaborator Author

@npeterkamps npeterkamps May 31, 2018

Choose a reason for hiding this comment

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

I would consider queryImpl.length (the function arity) to see how many arguments it actually takes, and passed the respective slice of args to it; if there's one more argument remaining, that's our Cypress command options with timeout and potentially other stuff such as log: false.

So if I understand this correctly, the function arity of our command function is the function arity of the query function + 1, right?
If the query function takes two arguments; text and options and options is optional, you'd have to pass in ("text", undefined, { timeout: 10000 }) to set a timeout?

I prefer the API to have a single options object. For that to work and still use ...args, we'd have to make the assumption that the options parameter is the last argument. In that case, I can retrieve it using args[args.length -1] and destructure it like already in the PR. So I agree that ...args is better, but I think the command function shouldn't have an extra options object just for the Cypress related options. What are your thoughts on that?

Copy link
Member

@kentcdodds kentcdodds May 31, 2018

Choose a reason for hiding this comment

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

Honestly I don't care all that much. Let's just go with what you have here and we can change it in the future if it needs changing. I think I'm good with this PR as-is :)

Copy link
Collaborator

@sompylasar sompylasar May 31, 2018

Choose a reason for hiding this comment

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

Okay, whatevs. I'm just saying that queryImpl is not guaranteed to be that generic to always take (doc, text, options).

Copy link
Member

@kentcdodds kentcdodds May 31, 2018

Choose a reason for hiding this comment

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

Hmmm... I think that's right. In that case, below where we're using text and options in message, do we need to make that more generic somehow to support different APIs?

Copy link
Collaborator

@sompylasar sompylasar May 31, 2018

Choose a reason for hiding this comment

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

Hmmm... I think that's right. In that case, below where we're using text and options in message, do we need to make that more generic somehow to support different APIs?

Yes, that, too. There was a generic solution before: just pass all the args for message (Cypress can handle arrays in the message as far as I remember).

container: doc,
timeout: 3000,
timeout,
})
let commandImpl
if (
Expand All @@ -32,12 +33,20 @@ const commands = Object.keys(queries).map(queryName => {
)(commandImpl)
return cy
.window({log: false})
.then(thenHandler)
.then({ timeout: timeout + 100 }, thenHandler)
.then(subject => {
Cypress.log({
$el: subject,
name: queryName,
message: args,
message: [text, options].filter((value) => {
if (Array.isArray(value) && value.length === 0) {
return false;
}
if (typeof value === 'object' && Object.keys(value).length === 0) {
return false;
}
return Boolean(value);
}),
})
return subject
})
Expand Down