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

Addon Test: Optimize internal dependencies #29595

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Nov 12, 2024

Closes #

What I did

This is an effort to fix E2E test flake that in our codebase, but turned out that addon-test was missing one bit of optimization which also contributed to flake (actually to all users) so this PR fixes that too.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.2 MB 78.2 MB 4.83 kB 1.7 0%
initSize 143 MB 143 MB 4.92 kB 1.4 0%
diffSize 65.2 MB 65.2 MB 85 B 0.62 0%
buildSize 6.88 MB 6.88 MB 85 B 2.85 0%
buildSbAddonsSize 1.51 MB 1.51 MB 0 B 0.58 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.9 MB 1.9 MB 85 B 2.96 0%
buildSbPreviewSize 271 kB 271 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.88 MB 3.88 MB 85 B 2.85 0%
buildPreviewSize 3 MB 3 MB 0 B - 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 7.6s 7.7s 113ms -1.02 1.5%
generateTime 23.4s 23s -374ms 0.1 -1.6%
initTime 16.9s 16.5s -457ms 0.67 -2.8%
buildTime 11s 8.2s -2s -805ms -0.68 -34.1%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5.3s 6.1s 786ms 0.37 12.8%
devManagerResponsive 3.3s 3.6s 369ms 0.1 10.1%
devManagerHeaderVisible 560ms 662ms 102ms 1.54 🔺15.4%
devManagerIndexVisible 633ms 694ms 61ms 0.84 8.8%
devStoryVisibleUncached 945ms 1.2s 267ms 0.66 22%
devStoryVisible 627ms 693ms 66ms 1.09 9.5%
devAutodocsVisible 519ms 651ms 132ms 1.81 🔺20.3%
devMDXVisible 482ms 599ms 117ms 0.95 19.5%
buildManagerHeaderVisible 703ms 597ms -106ms 0.3 -17.8%
buildManagerIndexVisible 725ms 607ms -118ms 0.19 -19.4%
buildStoryVisible 704ms 598ms -106ms 0.34 -17.7%
buildAutodocsVisible 566ms 529ms -37ms 1.02 -7%
buildMDXVisible 596ms 445ms -151ms -0.2 -33.9%

Greptile Summary

Based on the provided files and changes, here's a concise summary of the pull request:

Optimizes internal dependencies and test configurations for the Storybook Vitest plugin, focusing on improved dependency management and test environment handling.

  • Added react-dom/test-utils and test utilities to optimizeDeps.include in Vitest plugin for better dependency optimization
  • Added environment variable templates in playwright.config.ts to simplify local E2E testing setup
  • Reordered addons in test storybook configuration to maintain consistent addon loading order
  • Added test stories in AddonTest.stories.tsx to verify browser vs CLI environment behaviors

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 134 to 142
config.optimizeDeps ??= {};
config.optimizeDeps = {
...config.optimizeDeps,
include: [
...config.optimizeDeps.include,
'react-dom/test-utils',
'@storybook/experimental-addon-test/internal/test-utils',
],
};
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: potential error if config.optimizeDeps.include is undefined - should check and initialize as empty array if needed

Comment on lines 138 to 141
...config.optimizeDeps.include,
'react-dom/test-utils',
'@storybook/experimental-addon-test/internal/test-utils',
],
Copy link
Contributor

Choose a reason for hiding this comment

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

style: check if project uses React before including react-dom/test-utils to avoid unnecessary dependencies

Copy link

nx-cloud bot commented Nov 12, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 32fff17. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@yannbf yannbf force-pushed the yann/fix-e2e-flake-discrepancy-test branch from 005ca03 to f231139 Compare November 13, 2024 08:17
@yannbf yannbf added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Nov 13, 2024
@yannbf yannbf merged commit cb66c33 into next Nov 13, 2024
32 of 39 checks passed
@yannbf yannbf deleted the yann/fix-e2e-flake-discrepancy-test branch November 13, 2024 12:49
yannbf added a commit that referenced this pull request Nov 13, 2024
…ancy-test

Addon Test: Optimize internal dependencies

(cherry picked from commit cb66c33)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Nov 13, 2024
@github-actions github-actions bot mentioned this pull request Nov 16, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:normal patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants