887 Generate snapshot per story file #1584

igor-dv · 2017-08-03T06:48:45Z

Issue: #887

What I did

This is an experimental feature, PR is for visibility and discussion (meanwhile).

I've created a custom jest matcher (review is welcomed as well) which patches a toMatchSnapshot matcher.

How to test

Go to
storybook\addons\storyshots
and run yarn example

TBD

~~#### (1) Handle react-native~~ ✅ Fixed as @tmeasday suggested in review
For react I've added this piece of code

const realStoriesOf = storybook.storiesOf;
storybook.storiesOf = (kind, module) => {
  const storyFileName = module ? module.filename : '';
  const newKindName = `${storyFileName}?${kind}`;
  return realStoriesOf(newKindName, module);
};

I don't know if it will work with react-native and currently can't test it.
Any other suggestions how to extract story file name (without stack trace thingy) ?

~~#### (2) Integrity test to check if every .storyshot file has its story file~~ ✅

(3) Add custom reporter for storyshots: ❌

Since with a jest-specific-snapshot we can't use the default .snap extension, these tests are not included to "Snapshots" summery

But I don't know if it's blocking..

…y file

ndelangen · 2017-08-03T08:03:56Z

Thanks for giving this a go @igor-dv !

This breaks existing common conventions in jest snapshot testing.
(location, name of snapshot file, matcher?)

How are edge cases handled?

How is the process of updating the snapshot?
Do files get removed when snapshots are redundant?

igor-dv · 2017-08-03T08:25:49Z

@ndelangen

It indeed breaks the convention, so in order to not conflict with jest I mark these snapshot files with a*.storyshot extension instead of *.snap =)

Snapshot update flag is supported.

Files are not removed... My assumption is: Files will be located in the same places where the stories are - so it's a responsibility of the developers to remove them when they remove component/storyfile like they need to remove other unneeded assets (e.g. css files, images etc). But it can be handled in a few ways:

Test that checks conventions by story - snapshot name
eslint rule

Are there any other edgecases that I am not familiar with ?

ndelangen · 2017-08-07T12:29:41Z

@tmeasday Can you take a look at this?

Generated by storyshots. Note that these are large files with snapshots for all the components that have stories. This is not ideal, but there is a pull request in progress for storyshots that will change this so that snapshots are stored in the component directory, which will make more sense. See: storybookjs/storybook#1584

tmeasday · 2017-08-09T05:24:07Z

addons/storyshots/src/test-bodies.js

+}
+
+function getSnapshotFileName(context) {
+  const fileName = context.storyFileName || __filename;


Surely __filename isn't right here?

Oh yeah, actually I need the test path.. I thought it will be a nice fallback..

Yeah this will just be the file test-bodies.js right? I'm not sure what a good default is but perhaps it should be figured out by the jest plugin?

tmeasday · 2017-08-09T05:41:17Z

addons/storyshots/src/index.js

@@ -21,6 +27,16 @@ const hasDependency = name =>
  (pkg.devDependencies && pkg.devDependencies[name]) ||
  (pkg.dependencies && pkg.dependencies[name]);

+function patchStoriesOfFunction(storybookImpl) {


Could we just save filename alongside the story in core rather than doing this hack?

Indeed it's a better way.. I just didn't want to change the core only for the addon needs.. But maybe in this case it's a good solution.

I think it's a pretty transparent change that could be useful for other purposes.

tmeasday · 2017-08-09T05:44:15Z

I'm still interested in the approach where each story file is treated as a separate Jest test file, but that's probably much more complex than this approach.

This seems great @igor-dv -- It would be ideal to solve your (2) above, but if it's not possible this seems an improvement on the status quo.

We might want to do a run over some other storyshots related issues as I guess this will be a sort-of-breaking change? @ndelangen @shilman does that mean this would have to wait for storybook@4?

igor-dv · 2017-08-09T05:54:55Z

Not necessarily a breaking change if this behavior is configurable =) But IMO this is a big change so it should be (If at all) a part of some release.

tmeasday · 2017-08-09T06:36:44Z

Yeah! I mean we want people to notice it right ;)

You're right, I guess if people have to use the new test body. We can make it the default for 4.0 then?

codecov · 2017-08-09T21:45:09Z

Codecov Report

Merging #1584 into release/3.3 will increase coverage by 1.91%.
The diff coverage is 52.38%.

@@               Coverage Diff               @@
##           release/3.3    #1584      +/-   ##
===============================================
+ Coverage        21.21%   23.12%   +1.91%     
===============================================
  Files              252      253       +1     
  Lines             5695     5756      +61     
  Branches           672      692      +20     
===============================================
+ Hits              1208     1331     +123     
+ Misses            3984     3930      -54     
+ Partials           503      495       -8

Impacted Files	Coverage Δ
app/react/src/server/babel_config.js	`83.33% <ø> (+83.33%)`	⬆️
app/react-native/src/preview/story_store.js	`0% <0%> (ø)`	⬆️
app/react-native/src/preview/story_kind.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/story_store.js	`0% <0%> (ø)`	⬆️
app/vue/src/client/preview/story_store.js	`0% <0%> (ø)`	⬆️
app/react-native/src/preview/index.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/client_api.js	`85.71% <100%> (+0.71%)`	⬆️
app/vue/src/client/preview/client_api.js	`85.71% <100%> (+0.6%)`	⬆️
addons/storyshots/src/utils.js	`100% <100%> (ø)`
addons/storyshots/src/index.js	`80.85% <100%> (+80.85%)`	⬆️
... and 39 more

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 b77376c...6496454. Read the comment docs.

tmeasday · 2017-08-10T04:51:40Z

Seems great @igor-dv!

Should we add the filename tracking to the vue layer as well (we really need to refactor this a bit I reckon)

igor-dv · 2017-08-10T05:18:27Z

Yeah, I'll add it. Storyshots addon seems react dependent though. Do you have plans to refactor this somehow or to introduce another addon for Vue ? BTW, after adding the filename to react and react-native, seems like we have a lot of copy-pasting there.. And even codebeat complains about it =(. Don't we want to have some common stories_store or something ?

digitalmaster · 2017-09-14T03:37:15Z

Awesome work!... Created a quick PR that exports your getSnapshotFileName function so that i can use it in my custom test function.

PR Here: #1841

digitalmaster · 2017-09-15T17:45:33Z

@igor-dv Is it just me or does .toMatchSpecificSnapshot seem to ignore serializers?

Reproduce:

Add a serializer (for example enzyme-to-json/serializer). Either by Jest config's snapshotSerializers or inside tests using expect.addSnapshotSerializer(serializer)?
Generate a single file snapshot using default match toMatchSnapshot()
Generate a multi file snapshot using new matcher toMatchSpecificSnapshot()

Expected: Storyshots in separate files but content remains exactly the same.
Result: Storyshots in separate files but content is not serialized.

Any thoughts?

igor-dv · 2017-09-15T19:20:04Z

hm.. I need to check..

igor-dv · 2017-09-17T12:53:39Z

@digitalmaster , indeed there is a problem with the toMatchSpecificSnapshot.. looks like the instance of the serializers collection is different during the time toMatchSpecificSnapshot is being called.. Probably it's related to the jest modules life cycle. I have some workaround in mind, but it will take a bit time to fix it.

@tmeasday, @ndelangen what do you think, is this issue a blocker ?

A quick fix could be providing the serializes separately for storyshots...

tmeasday · 2017-09-18T00:22:03Z

Using the toMatchSpecificSnapshot is opt-in right? So if it doesn't work in certain use cases it's unfortunate but I don't think it's a blocker..

digitalmaster · 2017-09-18T07:33:10Z

Hmm... I do think that this would be a bad bug to ship to users - at least not without some kind of warning letting them know that they can't use serializers if they opt in to our new multi file snapshot test function. Or maybe if there's a work around they can use in the mean time?.. if that's the case maybe we should include this in the readme?

igor-dv · 2017-09-18T07:42:52Z

I'll try to add some kind of support until the 3.3 release

digitalmaster · 2017-09-18T12:43:08Z

Awesome.. thanks for looking into this @igor-dv 🙏🏽

ndelangen · 2017-09-20T20:47:34Z

@igor-dv perhaps you could contact @cpojer on this? Storybook + Snapshots with splitted across files is such a fantastic feature, maybe he's able to help you or guide you into how to implement?

dozoisch · 2017-10-24T22:01:06Z

I just tried the 3.3.0-alpha.2 releas and whenever I use multiSnapshotWithOptions it gives me

New snapshot was not written. The update flag must be explicitly passed to write a new snapshot.

Doesn't matter if I pass the flag -u or not. I tried requiring my stories with both explicit requires (eg: require('./myfile.stories.js') or req.context like I was doing before and still getting the same issue. If I comment out multiSnapshot it works properly. If I change it to snapshotWithOptions it generate the snap file properly in all cases.

====

Edit: Seems like it's because my Jest was out of date! Updating to latest Jest fixed it.

ndelangen · 2017-10-24T22:43:17Z

Did something break this? what could be going on @igor-dv ?

dozoisch · 2017-10-25T00:28:00Z

@ndelangen Oh sorry should've made the edit more obvious. Was due to my jest dep being outdated. Updating it to >20 Fixed it. Sorry about that. I'll leave my comment there so that if there are people hitting the same thing they know to update Jest.

tabby-or-not · 2017-11-29T04:54:06Z

Hi, I'm having a problem picking up obsolete snapshots using 3.3.0-alpha.4 and multiSnapshotWithOptions

I pulled the repo and edited cra-kitchen-sink to use multiSnapshotWithOptions and then generated the individual files.
If you then delete one of the cases (an .add or a storiesOf), it doesn't seem to pick up that there is an obsolete snapshot, and you need to manually delete the storyshot file and re-run it.

Any ideas?

On my own project, I also ran into Storybook integrity errors when deleting entire story files

zvictor · 2017-11-30T09:59:19Z

Hmm... I do think that this would be a bad bug to ship to users - at least not without some kind of warning letting them know that they can't use serializers if they opt in to our new multi file snapshot test function. Or maybe if there's a work around they can use in the mean time?.. if that's the case maybe we should include this in the readme?

I agree with @digitalmaster. We cannot underestimate the use of serializers in Jest and I foresee I lot of bug reports coming because of people who can't use e.g jest-styled-components in their tests.

I made a PR to clarify it in jest-specific-snapshot but it will rarely be noticed there. Something better would need to be done in this repo.

zvictor · 2017-11-30T13:06:19Z

Thank you guys for the great work on this PR. It makes such a big difference in my project that we would not be able to use snapshots without it. Still, I would like to suggest an improvement 🙃

The issue

In my setup, I always have a cases.spec.js that contains different props for my components. This file provides cases scenarios for both unit tests and storybook. With that in place, my components always have a standard stories.js file next to it, that looks like that:

import React from 'react';
import { storiesOf } from '@storybook/react';

import Component from './component';
import cases from './cases.spec.js';

const stories = storiesOf('Category Icon Hover Menu', module)

for (const [caseName, caseData] of cases) {
  stories.addCentered(caseName, () => (
    <Component {...caseData} />
  ));
}

I like organising the project like this because I get storybook "for free" while writing tests. However, the multiSnapshotWithOptions does not work the way I intended with such setup. I don't like that it always create a __snapshots__ folder with a single file on it.

The alternative

I rewrote getStoryshotFileName to use the stories kind and name instead of the original filename:

I personally like the results, and now the question is: should we make this the default behaviour and rewrite getStoryshotFileName to make use of the stories kind and name instead of the source filename, as shown above, to define the snapshot filename?

It would make the snapshot files smaller and easier and faster to read.

I can make a PR and push my code to replace getStoryshotFileName, but I would like to have discussed it here before.

igor-dv · 2017-11-30T14:11:29Z

@zvictor , I don't know about the default behavior, but maybe you can make a PR that will allow customizing this behavior?

zvictor · 2017-11-30T15:14:48Z

@igor-dv customising in what way? do you mean having a parameter to toggle/select one style or another?

igor-dv · 2017-11-30T15:21:25Z

You said that you've rewritten the getSnapshotFileName , so maybe instead of rewriting it, to add an option for providing a custom getSnapshotFileName ?

zvictor · 2017-11-30T15:59:04Z

Good idea. So I propose to change the signature of multiSnapshotWithOptions to accept getStoryshotFileName as second parameter (optional), so the following should achieve the results I proposed:

import _ from 'lodash'
import path from 'path'
import sanitize from 'sanitize-filename'
import initStoryshots, { multiSnapshotWithOptions } from '@storybook/addon-storyshots'

initStoryshots({
  test: multiSnapshotWithOptions({}, ({ fileName, kind, story }) => {
    const { dir } = path.parse(fileName)
    const name = sanitize(`${_.kebabCase(kind)}--${_.kebabCase(story)}`)

    return path.format({ dir: path.join(dir, '__snapshots__'), name, ext: '.storyshot' })
  }),
})

Once you agree, I will start working on that.

igor-dv · 2017-11-30T16:39:06Z

I am ok with this solution but would like to hear what others have to say

igor-dv · 2017-12-05T10:57:02Z

FYI, I've added support for the custom serializers to the plugin. Still didn't publish it to npm though.

zvictor · 2017-12-05T11:28:34Z

FYI, I've added support for the custom serializers to the plugin. Still didn't publish it to npm though.

Amazing! I have been working on that for 2 days. I tried many different things and I thought I was about to find a solution so many times, but none of my attempts worked. I am very curious to see your solution :)

brentmclark · 2017-12-08T14:47:51Z

@zvictor You can see how @igor-dv solved the issue here

@igor-dv is there anything I can do to help marshal your recent change into storybook? Testing, documentation, bump of a thread? If there is, let me know.

igor-dv · 2017-12-08T19:17:42Z

I've noticed that there was this PR - now it's possible to add serializer to the storyshots. I don't know if it covers all the needs, though...

zvictor · 2017-12-18T19:25:33Z

Would it be possible to move Storyshots Integrity - Abandoned Storyshots inside a conditional that checks if test is not a custom test function before adding these integrity tests?

Some defensive code like that is needed because getPossibleStoriesFiles only works well if the custom test function doesn't implement it's own version of a getStoryshotFile, which is not the case of the solutions I proposed above.

igor-dv · 2017-12-18T19:51:10Z

I think it's possible. Would you like to PR this with the proposal of the custom getSnapshotFileName ?

zvictor · 2017-12-19T14:18:23Z

I think it's possible. Would you like to PR this with the proposal of the custom getSnapshotFileName ?

done 🙂 please check #2517.

igor-dv added 2 commits August 2, 2017 17:27

Use experemental jest-specific-snapshot to generate snapshot per stor…

917f314

…y file

Add multiSnapshotWithOptions + upgrade jest-specific-snapshot to 0.2.0

1d54e27

igor-dv added addon: storyshots ci: do not merge feature request labels Aug 3, 2017

tmeasday self-assigned this Aug 3, 2017

Fix the case there is no "?"

810a63b

igor-dv added 2 commits August 6, 2017 10:41

Merge branch 'master' into 887-generate-snapshot-per-story-file

5e60a9c

Use patchStoriesOfFunction for RN

dc8bbd7

ndelangen requested a review from tmeasday August 7, 2017 12:29

Merge branch 'master' into 887-generate-snapshot-per-story-file

8f9926f

Merge branch 'master' into 887-generate-snapshot-per-story-file

3a78efe

tmeasday reviewed Aug 9, 2017

View reviewed changes

tmeasday requested changes Aug 9, 2017

View reviewed changes

igor-dv and others added 5 commits August 9, 2017 12:04

Merge branch 'master' into 887-generate-snapshot-per-story-file

9e20cb7

Add fileName prop to the storybook client api

0eb3093

Try storyshot example pass on CI

9f6e573

Save snapshots

1c1c9db

Merge branch 'master' into 887-generate-snapshot-per-story-file

ca294e9

shilman mentioned this pull request Aug 31, 2017

release/3.3 #1724

Merged

igor-dv mentioned this pull request Jan 10, 2018

Separate snapshots into the folders close to each story #887

Closed

milahu mentioned this pull request Oct 30, 2022

tests as docs: split snapshot files to make test outputs better accessible BuilderIO/mitosis#856

Open

887 Generate snapshot per story file #1584

887 Generate snapshot per story file #1584

Conversation

igor-dv commented Aug 3, 2017 • edited Loading

What I did

This is an experimental feature, PR is for visibility and discussion (meanwhile).

How to test

TBD

(3) Add custom reporter for storyshots: ❌

ndelangen commented Aug 3, 2017

igor-dv commented Aug 3, 2017 • edited Loading

ndelangen commented Aug 7, 2017

tmeasday Aug 9, 2017

Choose a reason for hiding this comment

igor-dv Aug 9, 2017

Choose a reason for hiding this comment

tmeasday Aug 9, 2017

Choose a reason for hiding this comment

tmeasday Aug 9, 2017

Choose a reason for hiding this comment

igor-dv Aug 9, 2017

Choose a reason for hiding this comment

tmeasday Aug 9, 2017

Choose a reason for hiding this comment

tmeasday commented Aug 9, 2017

igor-dv commented Aug 9, 2017

tmeasday commented Aug 9, 2017

codecov bot commented Aug 9, 2017 • edited Loading

Codecov Report

tmeasday commented Aug 10, 2017

igor-dv commented Aug 10, 2017

digitalmaster commented Sep 14, 2017

digitalmaster commented Sep 15, 2017

igor-dv commented Sep 15, 2017

igor-dv commented Sep 17, 2017 • edited Loading

tmeasday commented Sep 18, 2017

digitalmaster commented Sep 18, 2017 • edited Loading

igor-dv commented Sep 18, 2017

digitalmaster commented Sep 18, 2017

ndelangen commented Sep 20, 2017

dozoisch commented Oct 24, 2017 • edited Loading

ndelangen commented Oct 24, 2017

dozoisch commented Oct 25, 2017 • edited Loading

tabby-or-not commented Nov 29, 2017

zvictor commented Nov 30, 2017

zvictor commented Nov 30, 2017

The issue

The alternative

igor-dv commented Nov 30, 2017

zvictor commented Nov 30, 2017

igor-dv commented Nov 30, 2017

zvictor commented Nov 30, 2017 • edited Loading

igor-dv commented Nov 30, 2017

igor-dv commented Dec 5, 2017

zvictor commented Dec 5, 2017

brentmclark commented Dec 8, 2017

igor-dv commented Dec 8, 2017

zvictor commented Dec 18, 2017

igor-dv commented Dec 18, 2017

zvictor commented Dec 19, 2017

igor-dv commented Aug 3, 2017 •

edited

Loading

igor-dv commented Aug 3, 2017 •

edited

Loading

codecov bot commented Aug 9, 2017 •

edited

Loading

igor-dv commented Sep 17, 2017 •

edited

Loading

digitalmaster commented Sep 18, 2017 •

edited

Loading

dozoisch commented Oct 24, 2017 •

edited

Loading

dozoisch commented Oct 25, 2017 •

edited

Loading

zvictor commented Nov 30, 2017 •

edited

Loading