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 built storybooks to official storybook #2958

Merged
merged 6 commits into from
Feb 15, 2018

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Feb 11, 2018

Issue:

Screenshot tests for example apps were difficult to work with.

What I did

Added a iframe story to the official storybook for each example app. This results in these tests getting picked up by Chromatic.

How to test

Check these screenshots are correct: https://www.chromaticqa.com/build?appId=5a375b97f4b14f0020b0cda3&number=984 -- actually I am not sure what is going on with polymer there, perhaps I shouldn't have added it too?

I'm not completely on top of what the previous tests were used for (and I haven't actually removed them in this PR, maybe I should), but I wanted to try this out and run it by you to see if you think it's useful.

@@ -0,0 +1 @@
../../angular-cli/storybook-static
Copy link
Member

Choose a reason for hiding this comment

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

How does it work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just symlinked the built projects into the static directory of this storybook instead of needing to run a HTTP server for those files or importing them with a file:// (which I guess is conceptually what it does right now).

@tmeasday
Copy link
Member Author

So @ndelangen @Hypnosphi -- do we want to merge this PR? If it is not useful (I'm not sure I totally understand) we can also just close it, I was just feeling it out.

PS although the polymer-cli snapshot was denied by @ndelangen it is actually what the output of build-storybook in the polymer-cli looks like so... ;)

@Hypnosphi
Copy link
Member

Let's add this as failing test. Someone should fix polymer in another PR then

@codecov
Copy link

codecov bot commented Feb 15, 2018

Codecov Report

Merging #2958 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2958   +/-   ##
=======================================
  Coverage   37.37%   37.37%           
=======================================
  Files         426      426           
  Lines        9151     9151           
  Branches      881      869   -12     
=======================================
  Hits         3420     3420           
+ Misses       5205     5199    -6     
- Partials      526      532    +6
Impacted Files Coverage Δ
lib/ui/src/modules/ui/containers/routed_link.js 22.72% <0%> (ø) ⬆️
lib/core/src/client/manager/index.js 0% <0%> (ø) ⬆️
.../src/modules/ui/components/stories_panel/header.js 29.62% <0%> (ø) ⬆️
addons/actions/src/lib/types/nan/index.js 82.35% <0%> (ø) ⬆️
lib/core/src/server/build-dev.js 0% <0%> (ø) ⬆️
addons/info/src/components/types/Shape.js 66.03% <0%> (ø) ⬆️
lib/core/src/client/preview/syncUrlWithStore.js 40.9% <0%> (ø) ⬆️
lib/ui/src/modules/ui/index.js 0% <0%> (ø) ⬆️
...ddons/actions/src/components/ActionLogger/index.js 74.35% <0%> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 49.42% <0%> (ø) ⬆️
... and 61 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 7ea452d...cc46530. Read the comment docs.

@tmeasday tmeasday merged commit b7a77d5 into master Feb 15, 2018
@tmeasday tmeasday deleted the tmeasday/add-built-storybooks-to-official-storybook branch February 15, 2018 01:53
@tmeasday
Copy link
Member Author

Should we remove the existing screenshot tests?

@igor-dv
Copy link
Member

igor-dv commented Feb 15, 2018

If you are talking about integration tests, I think we should

@tmeasday
Copy link
Member Author

Hmm, it seems these tests are not entirely reliable, because Chromatic isn't always waiting long enough for the iframe to load properly. There's a boring technical reason for this but I'll need to go away and try and sort this out. Shall we back this out in the meantime?

@Hypnosphi
Copy link
Member

Can we add a delay before taking screenshot?

@tmeasday
Copy link
Member Author

tmeasday commented Feb 16, 2018

@Hypnosphi we don't currently have that option in Chromatic (although there are legit use-cases for it, it is often used as a bandaid, and we like to hear about problems like this one). In this case I think we can solve it properly by waiting for any iframes rendered by a story to load properly before taking a screenshot. I'm working on a fix; shouldn't be too far off.

My apologies for the annoyance in the meantime.

@igor-dv
Copy link
Member

igor-dv commented Feb 16, 2018

Maybe you are already doing this, so apologize me if I am just speaking obviously =) There is a technique that is used by Webpagetest to calculate a speed index, that compares screenshots during the loading until there is no difference... And no need for a timeout.

@Hypnosphi
Copy link
Member

@tmeasday the problem is that stories are loaded asynchronously. So the empty storybook interface that you see on the screenshots is exactly how the iframe looks like when fully loaded

@tmeasday
Copy link
Member Author

the problem is that stories are loaded asynchronously.

Yes, to be precise the problem is that although we wait for all network requests to complete (with a small timeout) before taking a screenshot, if an iframe loads a large JS file (like the SB manager bundle) it can take a while to parse and execute that JS file (which then renders and kicks off further network requests in this case).

It isn't probably possible to determine if "some JS is in progress that may lead to rendering", handling the special case of "story renders an iframe, we should wait for that iframe's JS to load" is something we can do quite easily.

@Hypnosphi
Copy link
Member

like the SB manager bundle

Did you mean the preview bundle? Manager gets rendered OK

@tmeasday
Copy link
Member Author

tmeasday commented Feb 17, 2018

It does but that's because there is some delay between us stopping waiting and the actual screenshot happening... we are getting far into the weeds now ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants