-
-
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
CLI: use Jest to test CLI commands and remove outdated fixtures #12936
Conversation
7a7ff2f
to
45f9f2f
Compare
Running smoke-test in Starting storybook manually works fine (process completes with exit code It fails when invoked via /cc @shilman @ndelangen |
b492f70
to
8846884
Compare
Thanks for the PR @jamesgeorge007 💪🏻 I just pushed a small commit to fix a path, broken after a directory move, and left a few comments. I will take a look at the |
1026ab9
to
b85a883
Compare
@jamesgeorge007 I'm focusing on another PR for now but will take a new look at this one soon ;) |
fa21c72
to
3ced531
Compare
Found the issue with some fixtures! I will add commits to fix that in the next few days :) |
#13231 should fix things but a new error appeared, I'm investigating a bit more 🔍 |
b6611c7
to
922e9c3
Compare
To get the full result of the tests and not stop at the first failing one is really great 🎉 ! I think one more thing we need to improve is maybe how the underlying error/logs can be catch and display clearly in Jest test result. To have something more meaningful than: expect(received).toBe(expected) // Object.is equality
Expected: 0
Received: 1 |
922e9c3
to
8baeeb3
Compare
I discussed with @ndelangen and @shilman and it looks like all the fixtures are outdated. And so the result of these tests aren't really relevant nor reflect if Storybook properly works or not. As keeping up to date the fixtures is very time-consuming and as we have more robust e2e tests, it looks like we will remove these ones: #13318. I'm a bit disappointed to tell you that although you have spent time to them 😕 |
Co-authored-by: Gaëtan Maisse <gmaisse@solocal.com>
53dfd27
to
091ea50
Compare
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.
🚀
@shilman can I merge this one or are we still merging only PRs stabilizing 6.1? |
should be good to merge to next IMHO |
What I did
closes #13318