added waitForExpect #25

lgandecki · 2018-03-28T13:37:27Z

What: Added waitForExpect with a test.

Why: Based on the #21 discussion

How: Simple import/export :-)

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

The documentation is not added - as I'm curious first to how @kentcdodds wants to argue for a usecase with this library - I can add documentation based on that. Otherwise - the testcase I added I think pretty clearly explains how and when it should be used.

Also - I need to add typings, so I will first do that and again update the PR.

codecov · 2018-03-28T13:41:07Z

Codecov Report

Merging #25 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #25   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines          72     72           
  Branches       17     17           
=====================================
  Hits           72     72

Impacted Files	Coverage Δ
src/index.js	`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 66e8d45...f4888bf. Read the comment docs.

kentcdodds

This is very awesome! I'd like to review the code for wait-for-expect before I merge, this but I'm a fan of this PR :)

Could you add docs for it in the README?

lgandecki · 2018-03-28T13:57:31Z

Definitely! Would you want the doc to be similar to the test I added for the functionality? Meaning - showing an example like that?
As for the wait-for-expect go ahead whenever you have a time :) It's tiny.

Thanks!

kentcdodds · 2018-03-28T16:31:22Z

I've been thinking about this more and I'm starting to wonder whether waitForExpect should actually simply be: waitFor(() => somethingThatReturnsATruthyValue())

This way you could do:

await waitFor(() => queryByLabelText('username'))

getByLabelText('username').value = 'my-username'
expect(getByTestId('username-display')).toHaveTextContent('my-username')

I like this better because it avoids nesting. I realize you can accomplish this already:

await waitForExpect(() => expect(queryByLabelText('username')).not.toBeNull())

getByLabelText('username').value = 'my-username'
expect(getByTestId('username-display')).toHaveTextContent('my-username')

But I think changing this to waitFor instead will result in fewer people nesting assertions. What do you think?

lgandecki · 2018-03-28T16:49:11Z

Hmm.. I definitely see your point of not nesting tests, but I'm not sure.. Keeping aside the fact that with reasonably long assertion it will become nested by prettier anyway.

I think I'd prefer to use expectations instead of truthy value, mostly to keep consistent syntax.

Sure, I can do

await waitFor(() => getByTestId('fetchedMessage').text === 'My fetchedMessage')

but then I:
a) lose the familiar expectations that I might want to use.. for example expect().toMatch() with regexp becomes tricky. We also lose all the expectations extends that were recently introduced for this tool. Also - we can't wait for a snapshot to match, if someone wants to do that.
b) lose the nice error message when the wait fails. So I wouldn't have "expected myFetchedMessage to equal differentMessage" ,I would just have a timeout.
c) let's say I do two graphql queries, or even normal rest calls, they can come back in different orders, if I can just put expectations for both in one waitForExpect block then jest will wait for both of them to pass (or point out nicely which one failed). Which I think is against your idea of not-nesting.. :)

I think this non-nesting idea, and doing things like:

await waitForExpect(() => expect(queryByLabelText('username')).not.toBeNull())

could be something you/we emphasis as best-practices.

kentcdodds · 2018-03-28T16:58:19Z

Those are all good points.

Ok, so my next concern is that the implementation is overly complex. Is there a reason the implementation isn't simply:

function waitForExpect(expectation, {timeout = 4000, interval = 40}) {
  const startTime = Date.now()
  return new Promise((resolve, reject) => {
    const intervalId = setInterval(() => {
      try {
        expectation()
        clearInterval(intervalId)
        resolve()
      } catch(error) {
        if (Date.now() - startTime >= timeout) {
          return reject(error)          
        }
      }
    }, interval)
  })
}

I'm pretty sure that should work :)

lgandecki · 2018-03-28T17:11:43Z

Yeah, I think you are right - I can definitely make it simpler along the lines of what you tried here.
I run my tests with it and they passed, besides the one that calculated the number of retries, which is caused by one functional difference I see between the two implementantions:

if we use setInterval, then the first expectation will happen after the timeout - which means, we might add the default timeout to every test unnecessary.
That's why I did

flushPromises().then(() => {
      setTimeout(doStep, 0);

to ensure that the initial try happens as soon as possible (as I stated in my readme - with 100 tests of extra 50 ms you add 5 seconds to your tests, which can be a difference between great and mediocre dev experience) :)

More about the design part, I was thinking about passing an object at a second position, I generally prefer that, but here:

having the second argument as a number is parallel to setTimeout setInterval etc, it feels more natural.
Basically, you change your existing timeOut expectation from:

setTimeout(() => {
   expect(true).toEqual(true)
}, 100)

to:

waitForExpect(() => {
   expect(true).toEqual(true)
}, 100)

changing the timeout will happen MUCH more often than changing the default interval - to the point that I am not sure whether it's a great idea of including that option at all :)

lgandecki · 2018-03-28T17:18:05Z

How about:

module.exports = function waitForExpect(
  expectation,
  timeout = 4000,
  interval = 40
) {
  const startTime = Date.now();
  return new Promise((resolve, reject) => {
    let intervalId;
    function runExpectation() {
      try {
        expectation();
        clearInterval(intervalId);
        resolve();
      } catch (error) {
        if (Date.now() - startTime >= timeout) {
          reject(error);
        }
      }
    }
    flushPromises().then(() => {
      setTimeout(runExpectation, 0);
      intervalId = setInterval(runExpectation, interval);
    });
  });
};

kentcdodds · 2018-03-28T17:27:24Z

I don't think that will work first of all because you're never clearing the interval. Also, I think including flushPromises is not necessary. Is there a reason my example wont work?

kentcdodds · 2018-03-28T17:29:39Z

Oh, sorry, I missed your comment. Here's my adjusted solution:

function waitForExpect(expectation, {timeout = 4000, interval = 40}) {
  const startTime = Date.now()
  return new Promise((resolve, reject) => {
    // attempt the expectation ASAP
    try {
      expectation()
      resolve()
    } catch(ignoreError) {
      // ignore the error here and try it in an interval
    }
    const intervalId = setInterval(() => {
      try {
        expectation()
        clearInterval(intervalId)
        resolve()
      } catch(error) {
        if (Date.now() - startTime >= timeout) {
          return reject(error)          
        }
      }
    }, interval)
  })
}

lgandecki · 2018-03-28T17:37:35Z

Thanks. I did clear the interval in the runExpectation function.
This new version will still wait for the timeout interval now for mocked things. So,

  await waitForExpect(() => {
    expect(getByTestId("title").textContent).toMatch("Title");
  });

in my workshop will now take 40 ms instead of ~3ms. Those numbers add app quickly:)

as for flushing promises - same reason as above - but maybe setTimeout(()=> {}, 0) will be enough? I'm not sure

lgandecki · 2018-03-28T17:43:07Z

Maybe something like this:

function waitForExpect(
  expectation,
  timeout = 4000,
  interval = 40
) {
  const startTime = Date.now();
  return new Promise((resolve, reject) => {
    function runExpectation() {
      try {
        expectation();
        return resolve();
      } catch (error) {
        if (Date.now() - startTime >= timeout) {
          return reject(error);
        }
        setTimeout(runExpectation, interval);
      }
    }
    setTimeout(runExpectation, 0);
  });
};

kentcdodds · 2018-03-28T17:44:29Z

I had a big thing written out, then saw you updated your example and that works just fine for me 👍

I'd prefer to include that utility directly in the react-testing-library rather than depend on it in another package I think though. What do you think?

lgandecki · 2018-03-28T17:51:34Z

that is fine with me, it will add some complexity and extra tests to this library, a bit of extra noise that I didn't think you would want. If you are fine with that I can add them to this PR, along with the agreed on implementation.

https://github.com/TheBrainFamily/wait-for-expect/blob/master/src/waitForExpect.spec.js - those are the tests, they are passing with the new implementation (I'd have to adjust the last one that expects the number of retries when you change the interval time, since there will be less retries new)

I think I will still keep the waitForExpect npm package out there though - looks like it's getting a bit of a traction, even though I didn't even tweet about it. It's useful for other test tools as well :)

kentcdodds · 2018-03-28T18:33:38Z

I suppose if you update the implementation of the package we can use your package instead (as this PR shows). In the docs you can have a simple example then point to the package for your docs. Looks like there's merge conflict in the README. You should be able to fix it by running: npx kcd-scripts contributors generate.

Let me know when everything's updated and ready to go. Thank you very much @lgandecki!

lgandecki · 2018-03-29T09:52:33Z

Thanks a lot @kentcdodds , it was great "pairing" with you :-)

I've sent you an invitation to become an admin of that package - I figured that if you are going to directly depend on it from this one you might feel more comfortable having control as well.

I've done the required changes in the package. I want to put a bit of a time rewriting it to typescript so we can import the types directly here, to export them from your typing file. If I can't get it to work properly in ~hr or two, I will just use custom typing for now.

Once I'm done with that I will update the docs and resolve conflicts.

…d exports its typings

…library into waitForExpect

lgandecki · 2018-03-29T14:15:30Z

I've added the typescript and updated the docs, but I'm sure you have a different idea about how to document this functionality, feel free to give me some direction, or it might be easier if you just do it your style - since the whole documentation is in a way written as one your "articles", it's hard to imitate it :-)

I also pinpointed the dependency version to a current one - I don't expect many changes in that tool since it's so tiny and well tested, but also want to make sure we never accidentally break this library when updating wait-for-expect.

kentcdodds

I'm looking forward to this feature! Thanks!

kentcdodds · 2018-03-29T14:25:36Z

README.md

@@ -289,6 +292,14 @@ expect(getByTestId('count-value')).not.toHaveTextContent('21')
 // ...
 ```

+## Other
+
+#### `waitForExpect(expectation: () => void, timeout?: number, interval?: number) => Promise<{}>;`


Let's actually put this right after flushPromises and call out a limitation of flushPromises. Specifically that it only flushes the promises that have been queued up already and not those that will be queued up subsequently (maybe we can provide an example?)

kentcdodds · 2018-03-29T14:28:19Z

README.md

+
+When in need to wait for non-deterministic periods of time you can use waitForExpect,
+to wait for your expectations to pass. Take a look at [`Is there a different way to wait for things to happen?`](#waitForExcept) part of the FAQ,
+or the function documentation here: [`wait-for-expect`](https://github.com/TheBrainFamily/wait-for-expect)


Could we add a very simple (incomplete) example of how to use the function? We can keep the FAQ too, but a simple example will help people know what this does:

// ... await waitForExpect(() => expect(queryByLabelText('username')).not.toBeNull()) getByLabelText('username').value = 'chucknorris' // ...

kentcdodds · 2018-03-29T14:31:20Z

src/__tests__/end-to-end.js

+
+  await waitForExpect(() => {
+    expect(queryByText('Loading...')).toBeNull()
+    expect(queryByTestId('message').textContent).toMatch(/Hello World/)


Because we're going to discourage nesting assertions, could we move this assertion to after the waitForExpect?

This has the added benefit of not being waited for if it actually is broken. Maybe we should call that out actually.

kentcdodds · 2018-03-29T14:32:18Z

README.md

+  await waitForExpect(() => {
+    expect(queryByText('Loading...')).toBeNull()
+    expect(queryByTestId('message').textContent).toMatch(/Hello World/)
+  })


Similar to my comment in the test, let's update this so only the necessary assertions are inside the callback 👍

kentcdodds · 2018-03-29T15:29:27Z

Awesome. Thanks for the updates! I'm going to make a few updates of my own. I'm going to deprecate flushPromises (we can remove it in a future breaking change). I'd rather not have two ways to do the same thing and this one's superior I think. So I'm going to just update the README slightly. Thanks!

lgandecki · 2018-03-29T15:30:00Z

Cool :-) Yeah, I figured it might be easier for you to just smooth things out on your own from here. Thanks a lot!

kentcdodds · 2018-03-29T15:33:36Z

On second thought, I'll just merge this as-is and I'll go ahead and do the deprecation in another PR because I'll want to clean up the tests as well. This is so great! Thank you very much for your help @lgandecki!

kentcdodds · 2018-03-29T15:39:28Z

Hey @lgandecki, are you around? Could you join me in chat really quick?

kentcdodds · 2018-03-29T15:39:37Z

https://gitter.im/kentcdodds

kentcdodds · 2018-03-29T15:41:42Z

Actually I'm pinging you in twitter DMs. If you could check in with me there I have a quick question.

kentcdodds · 2018-03-29T16:15:37Z

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

lgandecki · 2018-04-01T07:56:33Z

Thank you, Kent! I appreciate the invitation. I will love to spend some time working on this library. Let's see if there is enough work here for me to actively contribute. :-)

Do you have any TODOs/plans about it? In other words - if you had unlimited time what would be the things you would like to work on here next?
Or do you want to keep this minimal and are rather happy with the current state of it and most of the other things you would like to have as pluggable packages?

One thing I'm not sure about is that since we are getting so close to end-to-end/use-it-as-user-would kind of tests where is the boundary? When do you see yourself using cypress instead of this?
I've been struggling with this concept for the last few months and started working on this idea of Universal Page Objects.. where you define the structure of your pages and are able to run the same tests in cypress and in enzyme.

To show you an example:

https://github.com/TheBrainFamily/TheBrain2.0/blob/develop/testing/web/features/questions.test.js#L75

This test works exactly the same in cypress (with it's super nice debugging capabilities - TDD/feature development) and enzyme (with it's super speed and immediate feedback - refactoring).. I've actually aded puppeter for parallelization in CI and testcafe for running on any browser (but both should be deprecated by cypress ability to parallelize and run different browsers so I won't focus on them anymore)

the pageobjects like the question page:
https://github.com/TheBrainFamily/TheBrain2.0/blob/develop/testing/web/features/pageObjects/QuestionsPage.js

use common API that I create like this:

https://github.com/TheBrainFamily/TheBrain2.0/blob/develop/testing/testHelpers/EnzymeElement.js
https://github.com/TheBrainFamily/TheBrain2.0/blob/develop/testing/testHelpers/EnzymeDriver.js (btw you can see here where the inspiration for waitFor came from... ;-) )

https://github.com/TheBrainFamily/TheBrain2.0/blob/develop/testing/testHelpers/CypressElement.js
https://github.com/TheBrainFamily/TheBrain2.0/blob/develop/testing/testHelpers/CypressDriver.js

I understand that this is out of scope for this package but I'd love to hear your thoughts here. Where is the boundary. How do we decide what type of tests to write. Or do you think this is a good abstraction and it might be a good idea to write tests in a way that they can be run using different runners/in different contexts?

kentcdodds · 2018-04-01T14:53:28Z

Hey @lgandecki,
Thanks for asking! I don't have a whole lot of other TODOs for this project. Maybe some things will come up to improve/add to our query selectors, but I expect that'll be the biggest part of everything. I'm also considering perhaps extracting most of this out and having ports for react, vue, ember, angular, etc. But I don't plan to do that work personally. I use React so that's the only one I personally care about.

As for page objects, Cypress has docs on the page objects pattern: https://docs.cypress.io/faq/questions/using-cypress-faq.html#Can-I-use-the-Page-Object-pattern I was surprised by this fact because I actually share the perspective @brian-mann (Cypress creator) has on the pattern: cypress-io/cypress#198 (comment)

So I'm not personally interested in enhancing the page objects pattern experience with this library. However, I am interested in making a new project that uses the queries found in this project to enhance the testing experience. I've already played around with this in my testing workshop:

https://github.com/kentcdodds/testing-workshop/blob/d605a2ab8f1ba0986271d062b7770db19a5844f6/cypress/support/commands.js#L2-L43

I'm actually pretty pleased with that getCommandWaiter function. Seems like a pretty good solution to the problem (though I expect there's a more cypress way to do this).

Anyway, to answer your question I think that most of the work in this area will be around other projects and maybe extracting pieces of this project out for reuse in others :) Which now I think about it I may do to facilitate a cypress utility that uses the queries :)

* Expose pretty-dom utility * Allow users of prettyDOM to set max length * Remove logDOM * Add tests for prettyDOM * Add yarn-error.log to .gitignore * Add documencation for prettyDOM * Update pretty-dom.js

added waitForExpect with test

cef06a8

kentcdodds reviewed Mar 28, 2018

View reviewed changes

lgandecki and others added 5 commits March 29, 2018 17:00

added typescript and simplified version of the waitForExpect, used an…

e53d137

…d exports its typings

Merge branch 'master' of https://github.com/kentcdodds/react-testing-…

0a6bfaa

…library into waitForExpect

added initial notes about waitForExpect

dc7e692

fixed styling

1f15dbc

minor stylistic change

91b241a

kentcdodds reviewed Mar 29, 2018

View reviewed changes

lgandecki added 4 commits March 29, 2018 19:23

updated tests to remove the nesting

0c046ab

updated readme

94faf4b

fixed d .md syntax

32bbae3

improved style

f4888bf

kentcdodds merged commit 4adbc1d into testing-library:master Mar 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added waitForExpect #25

added waitForExpect #25

lgandecki commented Mar 28, 2018

codecov bot commented Mar 28, 2018 •

edited

Loading

kentcdodds left a comment

lgandecki commented Mar 28, 2018

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 28, 2018

kentcdodds commented Mar 28, 2018 •

edited

Loading

lgandecki commented Mar 28, 2018

lgandecki commented Mar 28, 2018 •

edited

Loading

kentcdodds commented Mar 28, 2018

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 28, 2018

lgandecki commented Mar 28, 2018 •

edited

Loading

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 28, 2018 •

edited

Loading

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 29, 2018

lgandecki commented Mar 29, 2018

kentcdodds left a comment

kentcdodds Mar 29, 2018

kentcdodds Mar 29, 2018

kentcdodds Mar 29, 2018

kentcdodds Mar 29, 2018

kentcdodds commented Mar 29, 2018

lgandecki commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

lgandecki commented Apr 1, 2018

kentcdodds commented Apr 1, 2018

added waitForExpect #25

added waitForExpect #25

Conversation

lgandecki 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

lgandecki commented Mar 28, 2018

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 28, 2018

kentcdodds commented Mar 28, 2018 • edited Loading

lgandecki commented Mar 28, 2018

lgandecki commented Mar 28, 2018 • edited Loading

kentcdodds commented Mar 28, 2018

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 28, 2018

lgandecki commented Mar 28, 2018 • edited Loading

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 28, 2018 • edited Loading

kentcdodds commented Mar 28, 2018

lgandecki commented Mar 29, 2018

lgandecki commented Mar 29, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Mar 29, 2018

Choose a reason for hiding this comment

kentcdodds Mar 29, 2018

Choose a reason for hiding this comment

kentcdodds Mar 29, 2018

Choose a reason for hiding this comment

kentcdodds Mar 29, 2018

Choose a reason for hiding this comment

kentcdodds commented Mar 29, 2018

lgandecki commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

kentcdodds commented Mar 29, 2018

lgandecki commented Apr 1, 2018

kentcdodds commented Apr 1, 2018

codecov bot commented Mar 28, 2018 •

edited

Loading

kentcdodds commented Mar 28, 2018 •

edited

Loading

lgandecki commented Mar 28, 2018 •

edited

Loading

lgandecki commented Mar 28, 2018 •

edited

Loading

lgandecki commented Mar 28, 2018 •

edited

Loading