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

887 Generate snapshot per story file #1584

Merged
merged 31 commits into from
Aug 27, 2017

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Aug 3, 2017

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.

storyshots-example

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
image

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

@ndelangen
Copy link
Member

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
Copy link
Member Author

igor-dv commented Aug 3, 2017

@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:

  1. Test that checks conventions by story - snapshot name
  2. eslint rule

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

@ndelangen ndelangen requested a review from tmeasday August 7, 2017 12:29
@ndelangen
Copy link
Member

@tmeasday Can you take a look at this?

davidmason added a commit to zanata/zanata-platform that referenced this pull request Aug 7, 2017
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
davidmason added a commit to zanata/zanata-platform that referenced this pull request Aug 8, 2017
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
}

function getSnapshotFileName(context) {
const fileName = context.storyFileName || __filename;
Copy link
Member

Choose a reason for hiding this comment

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

Surely __filename isn't right here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

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

function patchStoriesOfFunction(storybookImpl) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@tmeasday
Copy link
Member

tmeasday commented Aug 9, 2017

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
Copy link
Member Author

igor-dv commented Aug 9, 2017

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
Copy link
Member

tmeasday commented Aug 9, 2017

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
Copy link

codecov bot commented Aug 9, 2017

Codecov Report

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

Impacted file tree graph

@@               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
Copy link
Member

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
Copy link
Member Author

igor-dv commented Aug 10, 2017

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 ?

@shilman shilman mentioned this pull request Aug 31, 2017
@digitalmaster
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Member Author

igor-dv commented Sep 15, 2017

hm.. I need to check..

@igor-dv
Copy link
Member Author

igor-dv commented Sep 17, 2017

@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
Copy link
Member

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
Copy link
Contributor

digitalmaster commented Sep 18, 2017

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
Copy link
Member Author

igor-dv commented Sep 18, 2017

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

@digitalmaster
Copy link
Contributor

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

@ndelangen
Copy link
Member

@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
Copy link

dozoisch commented Oct 24, 2017

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
Copy link
Member

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

@dozoisch
Copy link

dozoisch commented Oct 25, 2017

@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
Copy link

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
Copy link
Contributor

zvictor commented Nov 30, 2017

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
Copy link
Contributor

zvictor commented Nov 30, 2017

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.

screen shot 2017-11-30 at 13 25 24

The alternative

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

screen shot 2017-11-30 at 13 31 20

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
Copy link
Member Author

igor-dv commented Nov 30, 2017

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

@zvictor
Copy link
Contributor

zvictor commented Nov 30, 2017

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

@igor-dv
Copy link
Member Author

igor-dv commented Nov 30, 2017

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

@zvictor
Copy link
Contributor

zvictor commented Nov 30, 2017

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
Copy link
Member Author

igor-dv commented Nov 30, 2017

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

@igor-dv
Copy link
Member Author

igor-dv commented Dec 5, 2017

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

@zvictor
Copy link
Contributor

zvictor commented Dec 5, 2017

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
Copy link

@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
Copy link
Member Author

igor-dv commented Dec 8, 2017

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
Copy link
Contributor

zvictor commented Dec 18, 2017

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
Copy link
Member Author

igor-dv commented Dec 18, 2017

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

@zvictor
Copy link
Contributor

zvictor commented Dec 19, 2017

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

done 🙂 please check #2517.

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.

9 participants