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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions test/e2e/specs/editor/editor__page-basic-flow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. 👍


const testAccount = new TestAccount( accountName );
await testAccount.authenticate( page );
Expand Down
Loading