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.

Sign up for GitHub

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

Jump to bottom

fix(storybook): enable e2e tests #15709

Merged
merged 1 commit into from Mar 19, 2023
Merged

fix(storybook): enable e2e tests #15709

merged 1 commit into from Mar 19, 2023

Conversation

mandarini
Copy link
Member

@mandarini mandarini commented Mar 16, 2023 •
edited

Storybook e2e tests still had some tests and assertions that corresponded to outdated code!

  • Remove two tests that do not make sense any more
  • Remove react tests from Angular tests
  • Re-enable tests
  • Remove assertions that don't make sense any more
  • Skip tests that are running or building storybook, since there are issues with Storybook v6 and pnpm, and with Node 18.

@mandarini mandarini self-assigned this Mar 16, 2023
@vercel
Copy link

vercel bot commented Mar 16, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
nx-dev ⬜️ Ignored (Inspect) Mar 17, 2023 at 3:02PM (UTC)

@nx-cloud
Copy link

nx-cloud bot commented Mar 16, 2023 •
edited

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@meeroslav
Copy link
Contributor

Closed my PR in favor of this one. The remaining failing test was depending on the removed should not overwrite global storybook config files to install configuration on the workspace.

Now that it's gone the second test is failing. The difference in setup to both is using buildable=true so that is somehow messing up with the storybook configuration workflow

@meeroslav meeroslav requested a review from vsavkin as a code owner March 16, 2023 15:07
@mandarini mandarini force-pushed the fix/storybook-e2e-tests branch 4 times, most recently from 3916039 to 6704148 Compare March 16, 2023 16:59
@mandarini
Copy link
Member Author

@meeroslav it's not the buildable=true. I removed it and the issue is still there.

@mandarini
Copy link
Member Author

@meeroslav ok I fixed all. Pushing now

@mandarini mandarini changed the title fix(storybook): e2e - remove old tests and assertions fix(storybook): enable e2e tests and use v7 Mar 16, 2023
@mandarini mandarini force-pushed the fix/storybook-e2e-tests branch 2 times, most recently from e9a9e6c to 433ac31 Compare March 16, 2023 17:52
meeroslav
meeroslav requested changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@meeroslav meeroslav left a 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.

@mandarini
Copy link
Member Author

@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!

@mandarini mandarini force-pushed the fix/storybook-e2e-tests branch 2 times, most recently from 545558a to a2945d1 Compare March 17, 2023 10:13
@mandarini mandarini force-pushed the fix/storybook-e2e-tests branch 6 times, most recently from 3e6b260 to 1a63638 Compare March 17, 2023 10:23
@mandarini mandarini changed the title fix(storybook): enable e2e tests and use v7 fix(storybook): enable e2e tests Mar 17, 2023
@mandarini mandarini mentioned this pull request Mar 17, 2023
Closed
meeroslav
meeroslav requested changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@meeroslav meeroslav left a 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.

@mandarini @meeroslav
fix(storybook): enable e2e tests and use v7
4c9d36d 
Co-authored-by: Miroslav Jonaš <meeroslav@users.noreply.github.com>
This was referenced Mar 17, 2023
Merged
Closed
@meeroslav meeroslav merged commit 61fb737 into nrwl:master Mar 19, 2023
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@meeroslav meeroslav meeroslav approved these changes

@Coly010 Coly010 Coly010 approved these changes

@barbados-clemens barbados-clemens barbados-clemens approved these changes

@FrozenPandaz FrozenPandaz Awaiting requested review from FrozenPandaz FrozenPandaz is a code owner

@vsavkin vsavkin Awaiting requested review from vsavkin vsavkin is a code owner

Assignees

@mandarini mandarini

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mandarini @meeroslav @Coly010 @barbados-clemens