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 support for test failures in async tests #11962

Merged
merged 2 commits into from Oct 3, 2020

Conversation

kahirokunn
Copy link
Contributor

@kahirokunn kahirokunn commented Aug 13, 2020

What I did

some times I wanna reject test like this.
jestjs/jest#6121 (comment)

@kahirokunn kahirokunn changed the title Just allowing ArgsStoryFn's Args type to be more specific. Just pass Promise.reject method to storyshots asyncJest test Oct 3, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Sorry for the slow turnaround on this. Shouldn't this be done with the done(error) API instead?

https://jestjs.io/docs/en/asynchronous#callbacks

@kahirokunn
Copy link
Contributor Author

Yeah, I wanna write code like it.
So, I tried this code but I got success all test in storyshots. 😢

import initStoryshots from '@storybook/addon-storyshots'
import path from 'path'

initStoryshots({
  asyncJest: true,
  test: async ({ done }) => {
    try {
      expect(1).toEqual(2)
      done()
    } catch (error) {
      done(error)
    }
  },
})

@kahirokunn
Copy link
Contributor Author

Is there any done(error) test code?

@shilman
Copy link
Member

shilman commented Oct 3, 2020

@kahirokunn i see. then WDYT about something more like:

        new Promise((done, reject) =>
          testMethod({
            done: (err) => (err ? reject(err) : done())
            ...

@kahirokunn
Copy link
Contributor Author

@shilman I tried it and that worked for me.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

💯

@shilman shilman added this to the 6.1 maintenance milestone Oct 3, 2020
@shilman shilman changed the title Just pass Promise.reject method to storyshots asyncJest test Storyshots: Fix support for test failures in async tests Oct 3, 2020
@kahirokunn
Copy link
Contributor Author

@shilman
Thanks for the quick reply!

One more thing.
I want to do this kind of thing with storyshots-puppeteer too, can I remove this assertion expect.assertions(1);? (I feel like these assertions should be in the test code and not in the runtime code.

https://github.com/kahirokunn/storybook/blob/e48ad84e1b1f9872a5e0c31e35583341aa37d684/addons/storyshots/storyshots-puppeteer/src/imageSnapshot.ts#L14

@shilman
Copy link
Member

shilman commented Oct 3, 2020

@kahirokunn i don't have enough context on that to respond. maybe a separate PR and we can figure that out?

@kahirokunn
Copy link
Contributor Author

Thanks a lot.
I tried like it.
#12657

@kahirokunn
Copy link
Contributor Author

Why CI is failed? 🤔
Is there anything I can do to?

@shilman shilman merged commit bac3800 into storybookjs:next Oct 3, 2020
@shilman
Copy link
Member

shilman commented Oct 3, 2020

@kahirokunn ignore it! 😁

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.

None yet

3 participants