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

Add a noMatchSnapshots option. #917

Closed

Conversation

tmeasday
Copy link
Member

See (storybook-eol/storyshots#93).

Issue:

The idea is that early on in a project, when things are changing a lot, snapshot tests are not particularly useful, but it's still helpful to "smoke test" all stories to ensure they run without error.

What I did

I added a noMatchSnapshots option to just setup a test that runs each story, without checking snapshots.

How to test

  1. Create a project, install storyshots but pass initStoryshots({ noMatchSnapshots: true }).
  2. Add a story, run jest. Should pass.
  3. Change the component's output, run jest again. Should still pass
  4. Make the component error in the render function, run jest again. Should fail.

Notes:

Perhaps it would make sense to just allow passing a custom test function, to cover off cases like this and storybook-eol/storyshots#76? Then storyshots is more of a generic tool to integrate storybook with Jest.

initStoryshots({
  test(renderedStory) {
     // In my case, just render without looking at the result
     renderer.create(renderedStory).toJSON()

     // For #76, shallow render + snapshot
     const tree = shallowToJson(shallow(renderedStory))
     expect(tree).toMatchSnapshot()
  }
});

@tmeasday tmeasday requested review from ndelangen and shilman and removed request for ndelangen April 19, 2017 12:40
@@ -97,3 +97,7 @@ initStoryshots({
```

Here is an example of [a regex](https://regex101.com/r/vkBaAt/2) which does not pass if `"Relay"` is in the name: `/^((?!(r|R)elay).)*$/`.

### `noMatchSnapshots`
Copy link
Member

Choose a reason for hiding this comment

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

@tmeasday What do you think about creating an option compareSnapshots that defaults to true? I don't like the double negative of noMatchSnapshots. Or just call the option you added skipComparison?

@shilman
Copy link
Member

shilman commented Apr 24, 2017

@tmeasday FWIW, I prefer your genericized solution to the current proposal.

@shilman
Copy link
Member

shilman commented Apr 24, 2017

I'm having problems testing out the change. It's probably unrelated to your change. What I did:

cd storybook
npm install
cd packages/storyshots
npm link .
cd ../../examples/cra-storybook
npm link storyshots
emacs src/storyshots.tests.js
npm run test

With the following contents for storyshots.tests.js:

import initStoryshots from 'storyshots'
initStoryshots({ noMatchSnapshots: true })

I get the error:

  ● Test suite failed to run

    Cannot find module 'react-test-renderer' from 'index.js'
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:151:17)
      at Object.<anonymous> (../../../../../.nvm/versions/node/v6.2.1/lib/node_modules/storyshots/dist/index.js:13:26)
      at Object.<anonymous> (src/storyshots.test.js:1:173)

I tried npm i -D react-test-renderer to add the peer dependency, but still get the same error.

I suspect this is related to Lerna. @tmeasday @ndelangen @aaronmcadam. Any help would be much appreciated.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

I'd name this autoUpdate and have it default value be false.

But I'm not really feeling much for this feature myself, since updating snapshots is real easy already anyway.

@tmeasday
Copy link
Member Author

I'm having problems testing out the change

Apologies, I didn't test the changes on the monorepo. My bad. Let me have a go at it and I'll ping you when I figure it out.

@tmeasday
Copy link
Member Author

I'd name this autoUpdate and have it default value be false.

But I'm not really feeling much for this feature myself, since updating snapshots is real easy already anyway.

@ndelangen -- the idea isn't to update the snapshots, it's to "smoke test" your stories but not to snapshot them.

When you are early in development, pretty much every commit changes your snapshots, and so (in my experience) you tend to disable snapshot tests as they are too noisy. However it's pretty useful to know if a commit has broken a component (in the sense that it fails to render for some story). That's the idea of this change.

@shilman -- I tend to agree that we should do the more generic thing.

Storyshots wants to plugin to the version used by the app, not have its own version. So its a clear peer dep.

Not sure it should depend on *storybook* at all given it is optional whether the user uses it or `react-native-storybook`
@tmeasday tmeasday force-pushed the storyshots-no-match-snapshots branch from 7cccef7 to c1c0489 Compare April 28, 2017 10:42
@tmeasday
Copy link
Member Author

I rebased this onto #971

Should be easy to test now @shilman.

Haven't changed the API yet though, LMK if it works for you and I'll look at that.

@tmeasday
Copy link
Member Author

Closing this in favour of #1034, simple PR to follow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants