Skip to content
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

Storyshots: Fix async issue with obtaining custom Puppeteer instance … #5130

Merged

Conversation

gidztech
Copy link

@gidztech gidztech commented Jan 2, 2019

…(#5069#)

Issue: The customBrowser config option for storyshots-puppeteer doesn't work because Jest doesn't wait for async functions to complete in test files that aren't part of the actual test function. You will get a Your test suite must contain at least one test. error because it doesn't find the tests in the current stack.

What I did

Instead of trying to do the async operation outside of the test function, we can pass the function to initStoryshots to later be called inside the beforeAll hook, where Jest will happily wait.

I changed customBrowser to getCustomBrowser, which is an async function that returns the browser instance. If that function isn't provided in config, the normal launch behaviour remains.

@storybook-safe-bot
Copy link
Contributor

storybook-safe-bot commented Jan 2, 2019

Fails
🚫

PR is not labeled with one of: ["cleanup","doc-dependencies:update","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS against 52414f7

@gidztech gidztech force-pushed the fix_custom_puppeteer_async_issue branch from e8521a4 to 52414f7 Compare January 2, 2019 12:40
@codecov
Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #5130 into next will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #5130      +/-   ##
==========================================
+ Coverage   35.07%   35.07%   +<.01%     
==========================================
  Files         594      594              
  Lines        7362     7361       -1     
  Branches     1007     1001       -6     
==========================================
  Hits         2582     2582              
  Misses       4271     4271              
+ Partials      509      508       -1
Impacted Files Coverage Δ
...ddons/storyshots/storyshots-puppeteer/src/index.js 0% <0%> (ø) ⬆️

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 ac75fa5...52414f7. Read the comment docs.

@ndelangen
Copy link
Member

@gidztech I think this is a breaking change, right?

@gidztech
Copy link
Author

gidztech commented Jan 3, 2019

@ndelangen Technically a breaking change because of the API change, but it wasn't actually working before.

@ColCh
Copy link
Contributor

ColCh commented Jan 15, 2019

@ndelangen this is a bug fix.

For now, it's impossible to use custom puppeteer instance

@ndelangen ndelangen merged commit 5f0ff3e into storybookjs:next Jan 15, 2019
@ndelangen ndelangen added this to the v5.0.0 milestone Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants