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

Gutenberg 20.0: Fix egde/nightly e2e tests #98581

Closed
wants to merge 1 commit into from
Closed

Gutenberg 20.0: Fix egde/nightly e2e tests #98581

wants to merge 1 commit into from

Conversation

gavande1
Copy link
Contributor

@gavande1 gavande1 commented Jan 20, 2025 •
edited
Loading

Related to #

Proposed Changes

  • Fix e2e tests by increasing the timeout for page tests.

Why are these changes being made?

  • E2E tests are failing on Edge/Nightly environments.

Testing Instructions

  • Follow E2E docs to set up your environment.
  • Run following e2e tests
TEST_ON_ATOMIC=true GUTENBERG_EDGE=true yarn workspace wp-e2e-tests test -- specs/editor/editor__page-basic-flow.ts

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 20, 2025 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-130259
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-130259&env=jetpack
Automattic for Agencies live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-130259&env=a8c-for-agencies

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

gavande1
gavande1 commented Jan 20, 2025
View reviewed changes
test/e2e/specs/editor/editor__page-basic-flow.ts
@@ -43,6 +43,7 @@ describe( DataHelper.createSuiteTitle( 'Editor: Basic Post Flow' ), function ()

beforeAll( async () => {
page = await browser.newPage();
page.setDefaultTimeout( 15000 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 @mmtr, Can you review this change? Since we forced e2e to use the WP Admin screen, the GB e2e tests are failing on TeamCity. I suspect the tests are failing because the test could not load the WP Admin screen in the stipulated time. The tests are passing locally after increasing the default timeout. However, I am not sure if this is a good solution to fix the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for investigating the test failures and suggesting a fix! However, I am not too sure about this change. I think there is a problem to be fixed in the tests. After testing locally using the trunk version, I was able to reproduce the error sometimes, but I have found that the problem happens when a Modal is displayed, please see attached screenshot
CleanShot 2025-01-20 at 11 19 33@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @epeicher, for quickly testing it. I tried to reproduce the modal case. However, the modal is not always visible. The primary problem here is that the previous navigation is timing out even though the selector is correctly resolved.
CleanShot 2025-01-20 at 17 11 32@2x

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what is happening:

  • As a result of this change, the Modal is displayed for some users sometimes, based on some rules
  • When the Modal is displayed, the test tries to click on Add New Page button here, and it timeouts because the Modal is blocking that. The test that calls that is the failing one here.

I think the proper fix would be either ensuring the Modal is not displayed for the test user or dismissing the Modal if it is displayed as a previous step in the test.

Copy link
Member

Choose a reason for hiding this comment

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

I think the proper fix would be either ensuring the Modal is not displayed for the test user or dismissing the Modal if it is displayed as a previous step in the test.

This is the way to go.

Notices are displayed only once, if you manually log in into the site and dismiss then, they shouldn't appear any more and therefore, the next run of the E2E test will pass.

I see the most recent runs are green, so I think we're good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @mmtr, for chiming in.

I see the most recent runs are green, so I think we're good?

Yes. I think so.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, a similar E2E test was failing here, and I prepared a PR to fix it (#98607).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @mmtr, for fixing the tests. 👍

@gavande1 gavande1 changed the title GB 20.0: Fix e2e tests Gutenberg 20.0: Fix egde/nightly e2e tests Jan 20, 2025
@gavande1 gavande1 requested a review from mmtr January 20, 2025 07:50
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 20, 2025
@gavande1 gavande1 closed this Jan 20, 2025
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epeicher epeicher epeicher left review comments

@mmtr mmtr Awaiting requested review from mmtr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gavande1 @matticbot @mmtr @epeicher