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: Integrate global error modal #29318

Merged
merged 5 commits into from
Oct 10, 2024

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Oct 10, 2024

Closes #

What I did

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/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>

Greptile Summary

This PR integrates a global error modal and updates test management functionality in Storybook, focusing on improved error handling and UI updates for test execution and results display.

  • Added 'crashed' state to TestProviderState interface in code/core/src/types/modules/addons.ts
  • Implemented crash handling and UI updates in SidebarBottom component in code/core/src/manager/components/sidebar/SidebarBottom.tsx
  • Updated VitestManager in code/addons/test/src/node/vitest-manager.ts to handle test cancellation and modify test run behavior
  • Removed emitChannelEvent method from SbPage class in code/e2e-tests/util.ts
  • Enhanced component testing functionality in test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts, including watch mode and improved test result handling

@valentinpalkovic valentinpalkovic changed the base branch from next to unified-ui-testing October 10, 2024 09:42
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","build","dependencies"]

🚫

PR is not labeled with one of: ["ci:normal","ci:merged","ci:daily","ci:docs"]

Generated by 🚫 dangerJS against e224aa9

Copy link

nx-cloud bot commented Oct 10, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2eb23c8. 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 changed the base branch from unified-ui-testing to ui-testing-progress October 10, 2024 13:46
Base automatically changed from ui-testing-progress to unified-ui-testing October 10, 2024 13:49
@yannbf yannbf force-pushed the valentin/integrate-global-error-modal branch from ce82d9d to 11a0891 Compare October 10, 2024 15:51
@yannbf yannbf marked this pull request as ready for review October 10, 2024 15:52
@yannbf yannbf changed the title WIP: Integrate global error modal Addon Test: Integrate global error modal Oct 10, 2024
@yannbf yannbf merged commit 8c9be57 into unified-ui-testing Oct 10, 2024
3 checks passed
@yannbf yannbf deleted the valentin/integrate-global-error-modal branch October 10, 2024 16:04
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.

6 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -23,7 +23,7 @@ export class VitestManager {

this.vitest = await createVitest('test', {
watch: watchMode,
passWithNoTests: true,
passWithNoTests: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Changing passWithNoTests to false might cause issues if there are no tests present. Consider the implications of this change on the test execution flow.

Comment on lines +40 to +44
if (this.vitest) {
this.vitest.onCancel(() => {
// TODO: handle cancelation
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The onCancel handler is currently empty. Implement proper cancellation logic to ensure resources are cleaned up and the system is left in a consistent state.

Comment on lines 116 to +117
// TODO: This is just temporary, the UI will be different
await page.locator('#addons').getByRole('button').nth(2).click();
await page.locator("#addons").getByRole("button").nth(2).click();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: This line is duplicated from the previous test. Consider extracting this common setup into a helper function

Comment on lines +120 to 121
const errorFilter = page.getByLabel("Show errors");
await expect(errorFilter).toBeVisible({ timeout: 30000 });
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The timeout of 30000ms seems quite long. Consider if this can be reduced or made configurable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants