-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Add built storybooks to official storybook #2958
Conversation
We now need the other examples to be built too for the chromatic tests
@@ -0,0 +1 @@ | |||
../../angular-cli/storybook-static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work?
There was a problem hiding this comment.
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).
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 |
Let's add this as failing test. Someone should fix polymer in another PR then |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Should we remove the existing screenshot tests? |
If you are talking about integration tests, I think we should |
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? |
Can we add a delay before taking screenshot? |
@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 My apologies for the annoyance in the meantime. |
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. |
@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 |
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. |
Did you mean the preview bundle? Manager gets rendered OK |
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 ;) |
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.