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

Remove duplicate views: Fix Page Basic Flow E2E test #98607

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Jan 20, 2025

See p1737384275008899-slack-C034JEXD1RD

Proposed Changes

Updates the logic to click on the "Add New Page" button for the Page Basic Flow E2E test since apparently it was broken when we started the "Remove duplicate views" experiment

Why are these changes being made?

Because there is a broken e2e test

Testing Instructions

Run this locally:

export TEST_ON_ATOMIC='true'        
export ATOMIC_VARIATION='ecomm-plan'  
export JETPACK_TARGET='wpcom-deployment'          
yarn workspace wp-e2e-tests build  
yarn workspace wp-e2e-tests test -- specs/editor/editor__page-basic-flow.ts

Make sure the E2E test passes

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)?

@mmtr mmtr self-assigned this Jan 20, 2025
@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Remove Duplicate Views labels Jan 20, 2025
@mmtr mmtr requested a review from anomiex January 20, 2025 15:55
@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.

@mmtr mmtr merged commit e861290 into trunk Jan 20, 2025
16 checks passed
@mmtr mmtr deleted the fix/e2e-remove-duplicate-views-experiment branch January 20, 2025 16:09
@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
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

Looks ok to me. 👍

Comment on lines 39 to 42
await Promise.all( [
this.page.waitForNavigation(),
this.page
.getByRole( 'link', { name: /Add New Page/ } )
.first()
.click(),
this.page.waitForNavigation( { url: /post-new.php\?post_type=page/, timeout: 20 * 1000 } ),
locator.click( { timeout: 20 * 1000 } ),
] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could improve the timeout situation by doing like this instead.

await this.page.waitForNavigation( { url: /post-new.php\?post_type=page/, timeout: 20 * 1000 } );
await locator.click( { timeout: 20 * 1000 } );

I think that way it won't start the timer for the click until after the page is navigated to, instead of starting both timers at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

waitForNavigation does not navigate to a different page, it only introduces a blocker until the URL changes. It's the click action what produces the navigation.

So if we wanted to have a sequence, it should be the other way around:

// Navigate to "Add New Page"
await locator.click( { timeout: 20 * 1000 } );
// Wait for "Add New Page" to be loaded
await this.page.waitForNavigation( { url: /post-new.php\?post_type=page/, timeout: 20 * 1000 } );

The two are intrinsically related, so I think it makes sense to group them in the same promise.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL, thanks!

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.

3 participants