-
Notifications
You must be signed in to change notification settings - Fork 2.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
fix(storybook): enable e2e tests #15709
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 4c9d36d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Closed my PR in favor of this one. The remaining failing test was depending on the removed Now that it's gone the second test is failing. The difference in setup to both is using |
3916039
to
6704148
Compare
@meeroslav it's not the |
df48757
to
7988477
Compare
@meeroslav ok I fixed all. Pushing now |
1c20cff
to
c3558fb
Compare
e9a9e6c
to
433ac31
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.
I left some notes that should be addressed.
I also think this PR diverged a bit. Let's have one PR that fixes existing tests and then have a followup that checks for storybook 7.
Mixing those two just leads to more moving parts that potentially break.
@meeroslav most of the tests (building/serving) were disabled, because Storybook cannot build/serve with v6.5+Node 18 for webpack/vite. This is why I thought that it's not worth it. But yes I can disable these, and then enable them in another PR! |
433ac31
to
ba97b43
Compare
545558a
to
a2945d1
Compare
3e6b260
to
1a63638
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.
I think the addition to newProject should be done in a separate PR and carefully planned out. I'm afraid the current implementation would create randomly flaky tests.
Co-authored-by: Miroslav Jonaš <meeroslav@users.noreply.github.com>
1a63638
to
4c9d36d
Compare
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Storybook e2e tests still had some tests and assertions that corresponded to outdated code!
pnpm
, and with Node 18.