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

Convert the Order Full Refund spec to Playwright #10151

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ismaeldcom
Copy link
Contributor

@ismaeldcom ismaeldcom commented Jan 15, 2025

Fixes #9961

Changes proposed in this Pull Request

  • Convert merchant-orders-full-refund-spec from Puppeteer to Playwright.
  • Screenshot code has been commented as all the screenshots code in Playwright, using the same TODO.

Testing instructions

  • Checkout dev/9961-convert-merchant-orders-full-refund-spec
  • Run npm run test:e2e-pw merchant-orders-full-refund-spec
  • Test should pass locally.
  • Test should pass in GH actions.

  • Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

@ismaeldcom ismaeldcom self-assigned this Jan 15, 2025
@Automattic Automattic deleted a comment from botwoo Jan 15, 2025

This comment was marked as off-topic.

@ismaeldcom ismaeldcom requested a review from a team January 15, 2025 08:51
Copy link
Contributor

@eduardoumpierre eduardoumpierre left a comment

Choose a reason for hiding this comment

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

The tests are passing, but I encountered a scenario where the first step of deactivating multi-currency could lead to flaky tests.


// Disable multi-currency in the merchant settings. This step is important because local environment setups
// might have multi-currency enabled. We need to ensure a consistent environment for the test.
await deactivateMulticurrency( merchantPage );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be a good idea to check if multi-currency is enabled before proceeding? I’m mentioning this because the test fails when multi-currency is already disabled.

Alternatively, we could skip this step if the save button is disabled. I believe the Playwright version of those helpers behaves a bit differently and doesn’t include that extra check for the save button.

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 for noticing it, I initially missed the issue while dealing with the support phone thing. I think it's better to check if it's active instead of skipping the save button when it's disabled to avoid missing any 0981fae

@botwoo

This comment was marked as off-topic.

@ismaeldcom ismaeldcom requested review from eduardoumpierre and a team and removed request for eduardoumpierre January 16, 2025 08:03
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.

E2E Playwright Migration: convert Order Full Refund spec
4 participants