Pass Jest done callback to testMethod

[hisapy]

Issue: NO ISSUE

What I did

I added the Jest done callback to be passed to the testMethod in StoryShot addon. Also updated the addon's README.

How to test

Is this testable with Jest or Chromatic screenshots?
I don't think so

Does this need a new example in the kitchen sink apps?
I don't think so

Does this need an update to the documentation?
I updated the StoryShot addon README with usage example.

If your answer is yes to any of these, please make sure to include it in your PR.

[hisapy]

Hello again ... I finally made some time to update and finish this PR. Added tests, updated to use the new API of the getSnapshotFilename.

[hisapy]

So far the only check failing is the one at Better Code Hub ... I tried to access it but it says only organization members can access the code ...

[hisapy]

I'm going to rename the option to avoid conflicts with async keyword

[codecov]

Codecov Report

Merging #3853 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3853      +/-   ##
==========================================
+ Coverage   40.59%   40.74%   +0.15%     
==========================================
  Files         483      484       +1     
  Lines        5747     5760      +13     
  Branches      770      771       +1     
==========================================
+ Hits         2333     2347      +14     
+ Misses       3042     3041       -1     
  Partials      372      372

Impacted Files	Coverage Δ
addons/storyshots/storyshots-core/src/api/index.js	82.35% <ø> (ø)	⬆️
...s/storyshots-core/src/api/ensureOptionsDefaults.js	100% <ø> (+10%)	⬆️
.../storyshots-core/src/api/snapshotsTestsTemplate.js	88.88% <100%> (+2.22%)	⬆️
...ore/stories/required_with_context/Async.stories.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 c6de989...917d745. Read the comment docs.

[hisapy]

@igor-dv can you take a look at this again please ?

[ndelangen]

Don't worry about Better Code Hub

[ndelangen]

Let's get @igor-dv's approval too

[igor-dv]

Can we call it just async?

[igor-dv]

It can be flacky

[igor-dv]

this parameter should be listed in the Options sections also

[hisapy]

I've just updated this PR with changes you requested @igor-dv .

By the way, I've used just async first but then I've opted for asyncJest to avoid issues with syntax highlightings that were assuming it was the async ES keyword

[igor-dv]

please separate each example file to a its own block

[igor-dv]

Thanks. Can you please merge from master, to pass CI?

[hisapy]

@igor-dv the branch is now up-to-date ... the CircleCI is apparently failing because Slack notficaion failed. No active hooks

[hisapy]

Thx for accepting this PR ladies and gentlemen

[SimenB]

Note that done is mostly for legacy reason in Jest, we recommend to return a promise instead. Mostly because if an assertion fails, done is never called unless you're careful with try-catches etc. Using new Promise(resolveCb => {}) is less of a footgun.

Not saying that you shouldn't expose a done - there are definitely valid use cases, but the docs makes it seem as the callback is the only way if you need to do async work

[hisapy]

Thx and wow! I haven't heard of done being legacy until now ... I'll try to investigate about the promise approach and create a new PR to address async components with it ASAP.

[SimenB]

"legacy" might be too strong. It's probably never going away because of old code bases, and it's a nice way to programatically fail a test. But most of the time doing return Promise.reject(new Error('fail test')) is better, and async-await makes working with promises quite nice. So we definitely wanna encourage promises over a done callback.

If you work with a callback API I would still use promises, maybe like this:

test('some api with cb pattern', async () => {
  const result = await new Promise((resolve, reject) => {
    callApiWithCallback((err, res) => {
      if (err) {
        reject(err);
      } else {
        resolve(res);
      }
    });
  });

  expect(result).toBeSomething();
});

Or use something like https://nodejs.org/dist/latest-v8.x/docs/api/util.html#util_util_promisify_original.

So yeah, done is perfectly valid, but we should try to encourage people to use promises instead. At least mention it in the docs section added in this PR

[hisapy]

I'd like to submit a PR to mention about using promise instead of done() but I need to do my homework about how to use promise to test async rendered components... currently the ones I've been testing don't have a render or afterRender callback... I just wait 1 ms so the component is updated and then start the assertions based on enzyme's mount and jest. Perhaps it should be as easy as rendering the component inside the awaiting Promise but I'm not so sure ...

Anyway, for people that might be worry, I can tell you that you still get a full test run with failing assertions and done().

[SimenB]

You can wait a millisecond using await new Promise(resolve => setTimeout(resolve, 1));. As long as a promise is returned (which using an async function does implicitly), Jest will wait for it