Separate snapshots into the folders close to each story

[shilman]

Issue by xareelee
Wednesday Mar 15, 2017 at 08:06 GMT
Originally opened as storybook-eol/storyshots#84

---

Currently, the .snap files generated by storyshots will be put in the __snapshots__ folder where the Storyshots.test.js is.

snapshots/
  ┣━ Storyshots.test.js   // run for storyshots
  ┗━ __snapshots__       // current we put snapshots for storyshots in this folder
          ┗━ Storyshots.test.js.snap    // we should separate the snapshots to be close to the .stories file

This results in a huge .snap file with thousands lines of code. It's hard for code review.

I think that the snapshots should be put in the __snapshots__ folder being close to the related .stories.

src/components/
  ┗━ SomeComponent   // for import
          ┣━ index.js                                // to export the component
          ┣━ SomeComponent.js              
          ┣━ SomeComponent.styles.js   
          ┣━ SomeComponent.stories.js  // Storybooks
          ┗━ __snapshots__                       // we should put .snap files here for storyshots
                 ┗━ Storyshots.test.js.snap // generated by Storyshots.test.js  (should put it here)

This will make code review easier. It's easy to know a change from which .stories.js file.

It would be better when we need to move the whole component folder into a separated module or project without losing its snapshots, doesn't it?

[tmeasday]

I think it would be best if we could make Jest consider the story tests as coming from the story file as opposed to the Storyshots.test.js file (I think this would have the side-effect of solving this problem).

Does anyone know of a way to alter the location that Jest considers a test as coming from?

[orta]

Well I just looked through the source code and there is a private, but accessible API you could abuse:

e.g.

const thisTestPath = expect.getState("testPath")
expect.setState("testPath", "path/to/story")
runStoryshotForFile("path/to/story")
expect.setState("testPath", thisTestPath)

/cc @cpojer

[tmeasday]

@orta have you tried messing with that? I gave it a go but it didn't seem to affect things. Reading the source code a bit, it seems like reading the snapshots is done per-test-file at startup time: https://github.com/facebook/jest/blob/9b157c3a7c325c3971b2aabbe4c8ab4ce0b0c56d/packages/jest-jasmine2/src/setup-jest-globals.js#L145

This implies to me that "fooling" jest into thinking a single test file (like so: https://github.com/storybooks/storybook/blob/master/examples/test-cra/src/storyshots.test.js) is providing snapshots for multiple test files isn't really going to work. I could be wrong though.

Maybe a better way forward is rather than storyshots reading all the stories in, and then looping over the stories, setting up a test for each story, it could instead hook into the .add() behaviour, so that if you .add a story and Jest globals are defined, it creates a test at "add time".

I've no idea if the above would work, I'm just noting it down as something to investigate to help with this issue.

[ndelangen]

I think you're onto something there @tmeasday.

You could use jest mocking to facade storybook's storyriesOf() & .add into describe() & .it()!

[tmeasday]

Yeah, that's my thinking. Pretty much the exact opposite to what https://www.npmjs.com/package/jest-storybook-facade does.

[ndelangen]

Would be a real big improvement for storyshots if you ask me!

[igor-dv]

Hey, I've created an experimental jest matcher that will probably help here.. Will try to have a working demo with storyshots soon.

[ndelangen]

Wow cool! @igor-dv! I was talking with @kentcdodds, this came up as well!

Although there was a misconception Storyshots was outputting as 1 big snapshot (which it is NOT). It outputs all snapshots individually into 1 file.

We're hoping to find a way to store the snapshots near the sourcefiles.

Looking forward to seeing your proposal @igor-dv, You're on a roll!

[tmeasday]

Oh, that's really interesting @igor-dv! Let's give it a try!!

[igor-dv]

So far

[xareelee]

By the way, we should avoid to update a gigantic file frequently using git. It's good to split the Storyshots.test.js.snap into multiple individual files for each story.

My current Storyshots.test.js.snap file is a size of 1.1 MB, and my git repo is going big quickly as the project goes on.

[igor-dv]

Should it be marked as merged if it's merged to the release branch ?

[tmeasday]

Not sure. I guess so, so people know it is solved?

[xareelee]

Has it been released? When or how can I start to use this nice feature? 😄

[tmeasday]

Hi @xareelee -- we'll do a 3.3 alpha soon, you can try installing that.

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

[Hypnosphi]

Works in 3.3.0-alpha.4

[sashless]

@igor-dv Using "multiSnapshots" the package https://github.com/styled-components/jest-styled-components stopped working. I guess the problem is that the method to change the serializer changed as the result snapshot is missing all the styles.

They add the serializer in addition to the styleRule matcher like this

expect.addSnapshotSerializer(styleSheetSerializer)
expect.extend({ toHaveStyleRule })

Would be nice to be compatible to that package using multiSnapshots

[igor-dv]

@sashless , can you please try adding this serializer to 'jest-specifics-snapshot' directly ?

  import { addSerializer } from 'jest-specifics-snapshot';

  addSerializer(styleSheetSerializer);

(jest-specifics-snapshot is used in Storyshots)

[greggb]

Yes! This fixed my serializer issue @igor-dv

The package is jest-specific-snapshot without the s on specific, btw

[igor-dv]

@greggb , cool !
BTW, @brentmclark, @zvictor we talked earlier here about exposing the addSerializer from Storyshots. Do you think we still need to do it? Or this solution is good enough?

[sashless]

@igor-dv I replaced the line expect.addSnapshotSerializer(styleSheetSerializer) with your proposal inside the plugin i mentioned but it did not fix the issue

[igor-dv]

@sashless , do you have a public repo to reproduce the problem?

[sashless]

I created a repo to show the problem https://github.com/sashless/storyshot-styled-components-with-multisnapshots-broken @igor-dv

[igor-dv]

You didn't really add custom serializer in your example. Here is how it should look like.

import initStoryshots, { multiSnapshotWithOptions } from '@storybook/addon-storyshots';
import styleSheetSerializer from 'jest-styled-components/src/styleSheetSerializer';
import { addSerializer } from 'jest-specific-snapshot'

addSerializer(styleSheetSerializer);

initStoryshots({
    suite: 'FileProperties',
    test: multiSnapshotWithOptions({}),
});

jest-specific-snapshot is used in multiSnapshotWithOptions. If you want to add a custom serializer you need to add it directly there.

[sashless]

awesome, thanks @igor-dv

[brentmclark]

I'm a little late to the party, @igor-dv; sorry for the slow response.

My preference is for more control, but I'm willing to try without addSerializer if you're on the fence about exposing it.

[SteffenGorenflo]

We really like that feature. But is it possible to use it for visual snapshots (storyshots-puppeteer) as well? So far I couldn't find anything in the docs

[igor-dv]

Visual snapshots in storyshots are already saving separated images.
I am not sure I understand the question 🤷‍♂️

[SteffenGorenflo]

Right now, all the screenshots are saved in only one folder __image_snapshots__ at the same level where Storyshots.test.js is located.

Currently:

src/components/
  ┗━ __image_snapshots__  // Global image snapshot folder, which should be split and saved in each component folder separately
  ┗━ Storyshots.test.js  
  ┗━ SomeComponent  
          ┣━ SomeComponent.jsx              
          ┣━ SomeComponent.css
          ┣━ SomeComponent.stories.js 
          ┗━ __snapshots__                       
                 ┗━ SomeComponent.test.js.snap

But we like to have a __image_snapshots__ folder in each component folder - just like the __snapshots__ folder.

src/components/
  ┗━ Storyshots.test.js  
  ┗━ SomeComponent  
          ┣━ SomeComponent.jsx              
          ┣━ SomeComponent.css
          ┣━ SomeComponent.stories.js 
          ┗━ __image_snapshots__   // move folder here with screenshots only of this component
          ┗━ __snapshots__                       
                 ┗━ SomeComponent.test.js.snap

[igor-dv]

You can possibly use this to customize the options passed to toMatchImageSnapshot

[SteffenGorenflo]

Ah yes, thank you! Works perfectly 👍

[Stupidism]

You didn't really add custom serializer in your example. Here is how it should look like.

import initStoryshots, { multiSnapshotWithOptions } from '@storybook/addon-storyshots';
import styleSheetSerializer from 'jest-styled-components/src/styleSheetSerializer';
import { addSerializer } from 'jest-specific-snapshot'

addSerializer(styleSheetSerializer);

initStoryshots({
    suite: 'FileProperties',
    test: multiSnapshotWithOptions({}),
});

jest-specific-snapshot is used in multiSnapshotWithOptions. If you want to add a custom serializer you need to add it directly there.

Thanks!!! Your solution helped me a lot.

Here comes another problem. Storyshots does not re-run after stories update anymore. It seems to be solved before in #2936 (comment)