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 support for storyshots to save files according to different patterns #2517

Closed
wants to merge 5 commits into from

Conversation

zvictor
Copy link
Contributor

@zvictor zvictor commented Dec 19, 2017

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

The API is quite simple:

import initStoryshots, { snapshotPerStoryAdded, snapshotPerStoryFile  } from '@storybook/addon-storyshots';

initStoryshots({
  // filesPattern: snapshotPerStoryAdded,
  filesPattern: snapshotPerStoryFile,
});

with that I also killed the multiSnapshotWithOptions. I hope nobody minds 😄

return getStoryshotFile(fileName);
}
const { dir } = path.parse(fileName);
const name = sanitize(`${kind}--${story}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is -- the separator we want to use?

@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

Merging #2517 into master will increase coverage by 59.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #2517       +/-   ##
==========================================
+ Coverage   35.74%     95%   +59.25%     
==========================================
  Files         472       6      -466     
  Lines       10134      40    -10094     
  Branches     1196       2     -1194     
==========================================
- Hits         3622      38     -3584     
+ Misses       5784       1     -5783     
+ Partials      728       1      -727
Impacted Files Coverage Δ
lib/ui/example/client/preview.js
app/angular/demo.js
app/polymer/src/client/preview/errorpreview.js
...ions/src/lib/types/function/createBoundFunction.js
addons/knobs/src/components/types/Select.js
addons/jest/src/components/Panel.js
lib/core/src/server/middleware.js
app/angular/src/client/index.js
lib/channel-postmessage/src/index.js
addons/actions/src/lib/decycle.js
... and 454 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 d0d6e4f...cb21f2a. Read the comment docs.

@igor-dv
Copy link
Member

igor-dv commented Dec 20, 2017

I will test it soon too. Let's not merge for now. We want to fix things with angular in 3.3 before + we have this that might conflict.

@zvictor
Copy link
Contributor Author

zvictor commented Dec 20, 2017

I will test it soon too. Let's not merge for now.

Cool. Just to make it clear, I am starting my vacations now and I will be out for a whole month. If you need me to make changes in the code it will need to wait. Sorry for that.

@igor-dv
Copy link
Member

igor-dv commented Dec 20, 2017

If it's not impeding your work - it's ok. I think there won't be a much work here during the holidays.

@ndelangen
Copy link
Member

Hey @zvictor can you please fix the merge conflict?

@ndelangen ndelangen changed the base branch from release/3.3 to master December 23, 2017 22:56
Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

You can't really remove the 'multiSnapshotWithOptions' it's already in 3.3 release, so renaming/removing it is a breaking change.

@igor-dv
Copy link
Member

igor-dv commented Jan 4, 2018

@zvictor , when you are back, let's revisit the implementation. I find it very useful to customize file names and extensions.

@igor-dv
Copy link
Member

igor-dv commented Feb 20, 2018

@zvictor , do you want to continue this PR? There are some conflicts because of the refactoring. Or I can take it from here.

@zvictor
Copy link
Contributor Author

zvictor commented Feb 21, 2018

@igor-dv sorry for my absence. I have been more busy than I expected since I came back from vacation and to be honest I cannot estimate when I would have some time available. Nonetheless, some of the changes that were made in the project since the creation of this PR are beyond my comprehension, so it would be a bit hard for me to do a proper merge.

with all of that said, I would be more than happy if you could take over this PR and work on the needed changes.

@igor-dv
Copy link
Member

igor-dv commented Feb 21, 2018

Sure

@bialesdaniel
Copy link

Any updates on this? My use case is that I have multiple index.stories.js files and if one of the stories change it gives me multiple failed tests. Being able to name the file based on the parent directory or something would be helpful.

@igor-dv
Copy link
Member

igor-dv commented Jun 10, 2018

@bialesdaniel, I am working on it.
I am closing this PR since there are too many changes in the repo to make it work.

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.

4 participants