-
Notifications
You must be signed in to change notification settings - Fork 69
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
E2E Playwright Migration: convert Payment Gateways Confirmation spec #10090
E2E Playwright Migration: convert Payment Gateways Confirmation spec #10090
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: 0 B Total Size: 1.36 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the E2E spec and the tests passed. The code modifications make sense and look solid.
> woocommerce-payments@8.7.0 test:e2e-pw
> ./tests/e2e-pw/test-e2e-pw.sh merchant-payment-gateways-confirmation
🎭 Running Playwright e2e tests in default headless mode, skipping @todo.
Running 5 tests using 1 worker
…etup] › auth.setup.ts:37:6 › authenticate as admin
Using existing merchant state.
…p] › auth.setup.ts:83:6 › authenticate as customer
Using existing customer state.
5 passed (11.7s)
There is a minor typo that should be fixed before merging, but I'm happy to approve this PR once that's done.
changelog/dev-9966-e2e-convert-payment-gateways-confirmation-spec
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comment. LGTM!
@@ -8,21 +8,18 @@ import { test, expect, Page } from '@playwright/test'; | |||
*/ | |||
import { useMerchant } from '../../utils/helpers'; | |||
|
|||
// Skipping the test for now as it is flaky on GH action runs. See #8875. | |||
test.skip( 'payment gateways disable confirmation', () => { | |||
test.describe( 'payment gateways disable confirmation', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I'm unsure if it is in scope for this PR or project, but have you considered adding @critical
tags to these tests as described in this RFC for measuring critical flow e2e coverage paJDYF-dVT-p2 (authored by @alopezari)?
test.describe( 'payment gateways disable confirmation', () => { | |
test.describe( 'payment gateways disable confirmation', { | |
tag: '@critical', | |
}, | |
() => { |
Mind you, this is not currently documented in the Playwright E2E README, but we have utilised @critical
tagging this for some e2e tests e.g. https://github.com/Automattic/woocommerce-payments/blob/develop/tests/e2e-pw/specs/merchant/merchant-admin-account-balance.spec.ts.
I'm also assuming that this is a critical flow, which may be incorrect. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging this. I see you also commented on the project thread about it. We can move the discussion forward there. That said, I checked and I don't believe these tests are part of the current critical flows.
Fixes #9966
Changes proposed in this Pull Request
This PR converts the Payment Gateways Confirmation spec from Puppeteer to Playwright. It also removes the old Puppeteer spec file.
Testing instructions
npm run test:e2e-pw merchant-payment-gateways-confirmation
npm run changelog
to add a changelog file, choosepatch
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.Post merge