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 mocking of all decorators in storyshots #1244

Closed
wants to merge 1 commit into from

Conversation

ndelangen
Copy link
Member

Issue: -unittest for storyshots is failing-

What I did

I added code to mock storybook addons during storyshots.

How to test

run npm test

@ndelangen ndelangen force-pushed the fix-unittest-storyshots branch from e8a546f to 6358e03 Compare June 9, 2017 21:03
@codecov
Copy link

codecov bot commented Jun 9, 2017

Codecov Report

Merging #1244 into 1220-kitchen-sink will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##           1220-kitchen-sink    #1244      +/-   ##
=====================================================
- Coverage              13.74%   13.66%   -0.09%     
=====================================================
  Files                    207      207              
  Lines                   4633     4662      +29     
  Branches                 582      575       -7     
=====================================================
  Hits                     637      637              
- Misses                  3494     3542      +48     
+ Partials                 502      483      -19
Impacted Files Coverage Δ
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
addons/knobs/src/KnobManager.js 38.57% <0%> (ø) ⬆️
addons/info/src/components/Node.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
app/react/src/server/babel_config.js 44.82% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 8.06% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
... and 16 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 46cff5f...6358e03. Read the comment docs.

@tmeasday
Copy link
Member

Which decorators were causing errors? I don't know if mocking them is what we want to do. As per the trichotomy here:

  1. "modifiers" need to run for the test.
  2. "inspectors" tend to just post to the channel, which is mocked out for storybook.
  3. "wrappers" are needed for the story to execute. (None of these are wrappers). Ideally they wouldn't be used in the snapshot, but it's not critical that they currently are.

Is it addon-info that's the problem? That's an "inspector" that actually modifies things, but we should fix that

@shilman
Copy link
Member

shilman commented Jun 10, 2017

@ndelangen I see @tmeasday 's point. I think this is a really clever pattern but we shouldn't use it until we need it, and it sounds like once we fix addon-info (per #1147) then we probably don't need this pattern. I propose we keep this in our back pocket for now. What do you think?

@ndelangen
Copy link
Member Author

Hey @tmeasday If you want to reject this, that's fine. If you want to suggest some other way of fixing the unit tests in the branch this PR was targeting?

@tmeasday
Copy link
Member

@ndelangen - AFAICT, the errors on that branch are the same as the ones on #1259, is that correct? The last failing build was https://travis-ci.org/storybooks/storybook/builds/241898626?utm_source=github_status&utm_medium=notification which has the same two errors I posted about here: #1259 (comment)

I think we need to get to the bottom of whether npm@5 linking / lerna is actually working. My suspicion is that it is not, in general.

@tmeasday
Copy link
Member

Ok, the larger issue is: #1286

I don't think there's much point in mocking out the things we are trying to test. Let's just disable these tests until we resolve the above.

@ndelangen ndelangen closed this Jun 16, 2017
@Hypnosphi Hypnosphi deleted the fix-unittest-storyshots branch October 11, 2017 22:31
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.

3 participants