Add unit tests for addon storyshots #971

tmeasday · 2017-04-28T10:43:49Z

Issue:

We didn't have a good way to test storyshots.

What I did

Added storyshots to test-cra app.

How to test

npm install
npm run bootstrap
cd examples/test-cra
npm install
npm test

tmeasday · 2017-04-28T10:44:17Z

Note from commit:

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

ndelangen · 2017-04-28T11:51:47Z

I pushed some fixed and updated your branch, hope you don't mind 👼

shilman · 2017-04-28T12:47:27Z

examples/cra-storybook/package.json

-    "react-scripts": "0.9.5"
+    "react-scripts": "0.9.5",
+    "react-test-renderer": "^15.5.4",
+    "storyshots": "*"


@tmeasday should this also be a file://... dependency like the other ones @ndelangen added? or does lernajs automagically do the right thing here?

Lerna will do it's thing on that!

codecov · 2017-04-28T13:26:36Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6f4566c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #971   +/-   ##
=========================================
  Coverage          ?   12.51%           
=========================================
  Files             ?      196           
  Lines             ?     4458           
  Branches          ?      713           
=========================================
  Hits              ?      558           
  Misses            ?     3269           
  Partials          ?      631

Impacted Files	Coverage Δ
packages/storyshots/src/index.js	`0% <ø> (ø)`

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 6f4566c...b5d7247. Read the comment docs.

ndelangen · 2017-04-28T13:36:17Z

Can this be used to help along #896 ?

tmeasday · 2017-05-01T03:11:02Z

Can this be used to help along #896 ?

I think #896 would be served by the alternate approach mentioned in #917

tmeasday · 2017-05-01T03:44:44Z

Cannot find module '@kadira/storybook-addons' from 'index.js'

Hmmm. This test is failing because I changed the dependency in storyshots to a peer dep, and the example project isn't using an addon (so as a result addons never gets installed).

I can see two solutions to the problem:

Return @kadira/storybook-addons to be a proper dep of storyshots (this still feels wrong as it is a plugin, and making it a direct dep is asking for versioning problems).
Follow this technique (https://github.com/storybooks/storybook/blob/9471a71933af45aa24c0db8d189c410809645b94/packages/storyshots/src/index.js#L15-L20) to check if the user has installed the addon package (i.e. is using any addons), and only mock if so.

I'm not sure of the best way to do 2. If you make the package a peer dep, and the user isn't using it, they will get a warning from npm, but things will work fine. This seems suboptimal though. The alternative is to just not specify anything in package.json.

Any advice @ndelangen?

ndelangen · 2017-05-09T05:43:12Z

I think not having it as a listed dependency is OK in this case as long as we guard when requiring it.

ndelangen · 2017-05-09T15:47:28Z

@tmeasday Do you think you can finish this now? Any more feedback needed? Let me know!

tmeasday · 2017-05-10T06:26:39Z

I'll get something going with that technique now.

tmeasday · 2017-05-11T04:27:42Z

This test is failing because I changed the dependency in storyshots to a peer dep, and the example project isn't using an addon (so as a result addons never gets installed)

Actually this was wrong. Storybooks depends on addons by default. The issue is that due to nested node_modules structures in this development context, storyshots can't require the correct addons.

tmeasday · 2017-05-15T02:54:16Z

OK I think I finally resolved this by using the test-cra app. I suspect we should get rid of the confusing cra-storybook example @ndelangen.

shilman

~~Looks great!~~
~~Minor point: I prefer renderOnly to justRender, but it's all good.~~

OOPS WRONG PR.

shilman · 2017-05-15T06:31:47Z

packages/storyshots/src/index.js

-  (pkg.devDependencies && pkg.devDependencies['@kadira/react-native-storybook']) ||
-  (pkg.dependencies && pkg.dependencies['@kadira/react-native-storybook']);
+
+const hasDependency = function(name) {


tmeasday · 2017-05-15T06:36:56Z

Minor point: I prefer renderOnly to justRender, but it's all good.

Isn't that the other PR? I think I prefer that too..

tmeasday · 2017-05-15T06:52:21Z

LMK if you want me to rebase this onto #1031

ndelangen · 2017-05-15T12:32:10Z

I was actually working on this / checking it out last evening and got pretty far, but not quite so far tests were passing. I'll work on it some more.

I can also push my changes, and have you take a look at it?

tmeasday · 2017-05-15T12:38:39Z

@ndelangen Please!

ndelangen · 2017-05-15T13:23:04Z

pushed feel free to revert if it's 💩

tmeasday · 2017-05-21T10:05:59Z

Ok, I think I finally fixed this. The tests should pass 😬

tmeasday · 2017-05-21T10:06:18Z

See commit for more detail: 1c8f8e0

shilman

Does the options.mode flag need to be added to docs?

shilman · 2017-05-21T10:08:56Z

addons/storyshots/src/index.js


 export default function testStorySnapshots(options = {}) {
  addons.setChannel(createChannel());

+  const isStorybook = options.mode === 'react' || hasDependency('@storybook/react');
+  const isRNStorybook = options.mode === 'react' || hasDependency('@storybook/react-native');


should be react-native?

A few things involved: 1. Remove dependencies from storyshots Storyshots wants to plugin to the version used by the app, not have its own version. So it should probably use a peer dep; however as some deps are optional (i.e. react vs react-native) this will lead to confusing warnings to the user. This seems like an OK approach until we hear differently from users. 2. Update test-cra's react-scripts to get it's Jest version up to date 3. Add a `mode` option to storyshots to avoid it auto-detecting if `@storybooks/react[-native]` is installed (this was breaking Jest run from the top-level). 4. Ensure that travis installs the `test-cra` properly

nx-cloud · 2024-03-27T08:26:00Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b5d7247. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build -c production --parallel=8

Sent with 💌 from NxCloud.

tmeasday added the in progress label Apr 28, 2017

tmeasday requested a review from ndelangen April 28, 2017 10:43

tmeasday mentioned this pull request Apr 28, 2017

Add a noMatchSnapshots option. #917

Closed

shilman reviewed Apr 28, 2017

View reviewed changes

ndelangen assigned tmeasday May 9, 2017

tmeasday force-pushed the test-storyshots branch from e93c883 to b88fd53 Compare May 15, 2017 02:53

tmeasday removed their assignment May 15, 2017

tmeasday mentioned this pull request May 15, 2017

Added custom test function for storyshots #1035

Merged

shilman approved these changes May 15, 2017

View reviewed changes

tmeasday force-pushed the test-storyshots branch from b88fd53 to 87c33ca Compare May 15, 2017 06:51

tmeasday force-pushed the test-storyshots branch 2 times, most recently from 87c33ca to 1c8f8e0 Compare May 21, 2017 10:05

shilman requested changes May 21, 2017

View reviewed changes

tmeasday force-pushed the test-storyshots branch from 1c8f8e0 to d96c5ed Compare May 21, 2017 10:32

shilman approved these changes May 21, 2017

View reviewed changes

tmeasday force-pushed the test-storyshots branch from d96c5ed to b5d7247 Compare May 21, 2017 10:45

tmeasday merged commit 4026bd6 into master May 21, 2017

tmeasday removed the in progress label May 21, 2017

shilman added the misc label May 27, 2017

ndelangen changed the title ~~Test storyshots~~ Add unit tests for addon storyshots May 27, 2017

ndelangen added the maintenance User-facing maintenance tasks label May 27, 2017

ndelangen deleted the test-storyshots branch July 8, 2017 22:33

lili2311 mentioned this pull request Jul 8, 2023

[Snyk-dev] Security upgrade babel-loader from 8.0.6 to 9.1.3 lili2311/storybook#143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for addon storyshots #971

Add unit tests for addon storyshots #971

tmeasday commented Apr 28, 2017 •

edited

Loading

tmeasday commented Apr 28, 2017

ndelangen commented Apr 28, 2017

shilman Apr 28, 2017

ndelangen Apr 28, 2017

codecov bot commented Apr 28, 2017 •

edited

Loading

ndelangen commented Apr 28, 2017

tmeasday commented May 1, 2017

tmeasday commented May 1, 2017 •

edited

Loading

ndelangen commented May 9, 2017

ndelangen commented May 9, 2017

tmeasday commented May 10, 2017

tmeasday commented May 11, 2017

tmeasday commented May 15, 2017

shilman left a comment •

edited

Loading

shilman May 15, 2017

tmeasday commented May 15, 2017

tmeasday commented May 15, 2017

ndelangen commented May 15, 2017

tmeasday commented May 15, 2017

ndelangen commented May 15, 2017 •

edited

Loading

tmeasday commented May 21, 2017

tmeasday commented May 21, 2017

shilman left a comment

shilman May 21, 2017

nx-cloud bot commented Mar 27, 2024

Add unit tests for addon storyshots #971

Add unit tests for addon storyshots #971

Conversation

tmeasday commented Apr 28, 2017 • edited Loading

What I did

How to test

tmeasday commented Apr 28, 2017

ndelangen commented Apr 28, 2017

shilman Apr 28, 2017

Choose a reason for hiding this comment

ndelangen Apr 28, 2017

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2017 • edited Loading

Codecov Report

ndelangen commented Apr 28, 2017

tmeasday commented May 1, 2017

tmeasday commented May 1, 2017 • edited Loading

ndelangen commented May 9, 2017

ndelangen commented May 9, 2017

tmeasday commented May 10, 2017

tmeasday commented May 11, 2017

tmeasday commented May 15, 2017

shilman left a comment • edited Loading

Choose a reason for hiding this comment

shilman May 15, 2017

Choose a reason for hiding this comment

tmeasday commented May 15, 2017

tmeasday commented May 15, 2017

ndelangen commented May 15, 2017

tmeasday commented May 15, 2017

ndelangen commented May 15, 2017 • edited Loading

tmeasday commented May 21, 2017

tmeasday commented May 21, 2017

shilman left a comment

Choose a reason for hiding this comment

shilman May 21, 2017

Choose a reason for hiding this comment

nx-cloud bot commented Mar 27, 2024

☁️ Nx Cloud Report

tmeasday commented Apr 28, 2017 •

edited

Loading

codecov bot commented Apr 28, 2017 •

edited

Loading

tmeasday commented May 1, 2017 •

edited

Loading

shilman left a comment •

edited

Loading

ndelangen commented May 15, 2017 •

edited

Loading