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

npeterkamps · 2018-05-26T15:16:59Z

What:

Make timeout configurable for all query functions through the options object. (Issue #6)

Why:

So I don't need to use cy.wait() to prevent timeouts from happening.

How:

use timeout from the options object
make sure that Cypress' timeout is larger, so our error message is used
make sure Cypress' Command Log doesn't get cluttered when options isn't specified

Other changes

show example in the docs
updated outdated snapshot
ignore .idea folder (JetBrains IDE's)
added myself to contributers at the request of the contribution guide

Question:

Should I add something in the tests to test the timeout? Like some JS code in the html file to dynamically add an element after a certain time?

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

- make sure that Cypress' timeout is larger, so our error message is used - make sure Cypress' Command Log doesn't get cluttered when options isn't specified - show example in the docs - updated outdated snapshot - ignore .idea folder (JetBrains IDE's) - added myself to contributers at the request of the contribution guide

kentcdodds

Looks good to me!

sompylasar · 2018-05-30T23:19:32Z

src/index.js

      const queryImpl = queries[queryName]
      const baseCommandImpl = doc =>
-        waitForElement(() => queryImpl(doc, ...args), {
+        waitForElement(() => queryImpl(doc, text, options), {


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.

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?

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

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?

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 :)

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

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?

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

kentcdodds · 2018-06-02T13:37:12Z

Rats. The build is failing and I don't know why. I don't have time to look into it. Could you figure out what's going on?

kentcdodds · 2018-06-02T13:40:16Z

I'm going out of town for all week so I probably wont be able to merge this myself. @sompylasar, I think you have merge rights so if you want to merge this once the build is fixed that would be splendid :)

…itForElement`.

npeterkamps · 2018-06-02T15:35:27Z

I don't know what's going on either. It works locally. Problem persisted after I forced Travis to try and build it again...

sompylasar · 2018-06-02T18:56:05Z

Travis log says Cypress cannot connect to the app server that it has to run the tests against.

[lint] npm run lint --silent exited with code 0
�7[test] It looks like this is your first time using Cypress: 2.1.0
[test] 
[test] [15:27:30]  Verifying Cypress can run /home/travis/build/kentcdodds/cypress-testing-library/node_modules/cypress/dist/Cypress [started]
[test]  PASS  src/__tests__/add-commands.js
[test]  PASS  src/__tests__/commands.js
[test] 
[test] Test Suites: 2 passed, 2 total
[test] Tests:       2 passed, 2 total
[test] Snapshots:   1 passed, 1 total
[test] Time:        5.148s
[test] Ran all test suites.
[test] [15:27:32]  Verifying Cypress can run /home/travis/build/kentcdodds/cypress-testing-library/node_modules/cypress/dist/Cypress [completed]
[test] 
[test] Opening Cypress...
[test] Cypress could not verify that the server set as your 'baseUrl' is running:
[test] 
[test]   > http://localhost:13370
[test] 
[test] Your tests likely make requests to this 'baseUrl' and these tests will fail if you don't boot your server.
[test] 
[test] Please start this server and then run Cypress again.
[test] npm run test --silent exited with code 1
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] validate: `kcd-scripts validate build,lint,test`
npm ERR! Exit status 1

sompylasar · 2018-06-02T19:06:27Z

I believe the Travis failure is unrelated to this PR's changes, but it's understandable:

  "test:cypress": "npm-run-all --silent --parallel --race test:cypress:serve test:cypress:run",

— this line does not guarantee that test:cypress:run starts after test:cypress:serve has started listening, so there are race conditions, depends on how quick things happen in parallel in both processes.

sompylasar · 2018-06-02T19:16:35Z

We can use https://www.npmjs.com/package/wait-port (don't forget to npm i --save-dev wait-port) in test:cypress:run prior to starting Cypress.

"test:cypress:run": "wait-port -t 10000 localhost:13370 && cypress run",

kentcdodds · 2018-06-02T19:43:37Z

I'll merge this, but it won't release if the build isn't working so we will want to get that done asap.

kentcdodds · 2018-06-02T19:49:19Z

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

**What**: This change adds TypeScript support for the `timeout` matcher option introduced in #7.  **Why**: Currently, `cypress-testing-library` uses the `MatcherOptions` interface directly from `dom-testing-library`, which does not have a `timeout` property (`timeout` is unique to `cypress-testing-library`), so using the `timeout` option throws a type error. ![image](https://user-images.githubusercontent.com/1288694/50553888-bf2ee180-0c6d-11e9-9250-a8c00c0db833.png)  **How**: I introduced a new `CTLMatcherOptions` interface for `cypress-testing-library` specific matcher options, aliased the `MatcherOptions` import from `dom-testing-library` to `DTLMatcherOptions`, and created a new type `MatcherOptions`, a union of the two. I also added `timeout` to the existing typings test (which fails without this change).  **Checklist**:   * [ ] Documentation * [x] Tests * [x] Ready to be merged  * [x] Added myself to contributors table   **Additional Information**: The build was failing because the commands snapshot was outdated (new commands were added to `dom-testing-library` more recently than `cypress-testing-library` has been built on CI). I fixed the snapshot in this PR so I could progress, but let me know if you'd prefer that be separated out in its own PR and I'll split them up.

kentcdodds approved these changes May 26, 2018

View reviewed changes

kentcdodds requested a review from sompylasar May 26, 2018 21:02

sompylasar reviewed May 30, 2018

View reviewed changes

npeterkamps mentioned this pull request Jun 2, 2018

chore(deps): Allow Cypress 3 within peerDep. Updated Cypress and serve deps #9

Merged

4 tasks

kentcdodds approved these changes Jun 2, 2018

View reviewed changes

Pass all args to queryImpl, pass all args with some defaults to `wa…

8c9c766

…itForElement`.

npeterkamps force-pushed the pr/timeout branch from b76248a to 8c9c766 Compare June 2, 2018 15:25

kentcdodds merged commit ec3b712 into testing-library:master Jun 2, 2018

npeterkamps deleted the pr/timeout branch June 2, 2018 20:33

wKovacs64 mentioned this pull request Dec 31, 2018

fix(TS): add timeout to MatcherOptions type #28

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

npeterkamps commented May 26, 2018 •

edited

Loading

kentcdodds left a comment

sompylasar May 30, 2018

npeterkamps May 31, 2018

kentcdodds May 31, 2018

npeterkamps May 31, 2018

kentcdodds May 31, 2018

sompylasar May 31, 2018

kentcdodds May 31, 2018

sompylasar May 31, 2018

kentcdodds commented Jun 2, 2018

kentcdodds commented Jun 2, 2018

npeterkamps commented Jun 2, 2018

sompylasar commented Jun 2, 2018

sompylasar commented Jun 2, 2018

sompylasar commented Jun 2, 2018

kentcdodds commented Jun 2, 2018

kentcdodds commented Jun 2, 2018

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

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

Conversation

npeterkamps commented May 26, 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

kentcdodds commented Jun 2, 2018

kentcdodds commented Jun 2, 2018

npeterkamps commented Jun 2, 2018

sompylasar commented Jun 2, 2018

sompylasar commented Jun 2, 2018

sompylasar commented Jun 2, 2018

kentcdodds commented Jun 2, 2018

kentcdodds commented Jun 2, 2018

npeterkamps commented May 26, 2018 •

edited

Loading