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

test: Delete site E2E case supports native dialogs #186

Merged
merged 4 commits into from
May 30, 2024

Conversation

dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented May 29, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/7236.

Proposed Changes

  • Mock the native delete dialog interaction, as Playwright does not currently
    support interactions with this OS UI.
  • Refactor onboarding interactions to improve test stability.

Testing Instructions

N/A, no user-facing changes.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

Playwright lacks support for interacting with native dialogs, so we mock
the dialog module to simulate the user clicking the "Delete site"
confirmation button with "Delete site files from my computer" checked.

See: microsoft/playwright#2143
See: microsoft/playwright#8278 (comment)
Combine `toBeVisible`'s interval with the ability to assert the precence
of one of multiple elements to ensure the UI is ready before
conditionally proceeding with the onboarding steps. This reduces the
likelihood that the UI is not ready when the test interactions begin and
also allows the Sites tests to succeed regardless of whether sites
already exists when the tests begin to run.
Comment on lines -8 to -9
// This fails, not sure why...
// return this.page.getByTestId( 'onboarding' );
Copy link
Member Author

Choose a reason for hiding this comment

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

My guess is that the unexpected failure may have related to attempting queries before the UI was visible. With the new approach proposed in these changes, the original query appears to work consistently.

@@ -29,7 +25,7 @@ export default class Onboarding {
}

get continueButton() {
return this.locator.getByRole( 'button', { name: 'Continue' } );
return this.locator.getByRole( 'button', { name: 'Add site' } );
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to match changes from #111.

const sidebar = new MainSidebar( onboardingMainWindow );

// Await UI visibility before proceeding.
await expect( onboarding.heading.or( sidebar.addSiteButton ) ).toBeVisible();
Copy link
Member Author

Choose a reason for hiding this comment

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

By relying upon toBeVisible's interval-based query and the or query, this approach should guarantee the app UI is visible before proceeding with interactions.

@dcalhoun dcalhoun marked this pull request as ready for review May 29, 2024 19:37
@dcalhoun dcalhoun requested review from a team May 29, 2024 19:37
Copy link
Contributor

@derekblank derekblank left a comment

Choose a reason for hiding this comment

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

LGTM. I observed the e2e tests have passed (both locally and on CI), and the code changes make sense. 🚀

Screenshot 2024-05-30 at 3 04 53 pm

@wojtekn
Copy link
Contributor

wojtekn commented May 30, 2024

@dcalhoun changes look good and work fine.

It caught me at first as I couldn't make e2e work locally today, either in this branch or trunk. I noticed that Studio was using x64 binary, and then I realized I needed to clean out/ and build fresh binary first. I opened another PR #190 to improve readme.

@wojtekn wojtekn merged commit cd5ff09 into trunk May 30, 2024
13 checks passed
@wojtekn wojtekn deleted the test/delete-site-e2e-case-supports-native-dialogs branch May 30, 2024 06:38
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