-
Notifications
You must be signed in to change notification settings - Fork 21
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 E2E tests from Puppeteer to Playwright #2041
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2041 +/- ##
===========================================
+ Coverage 58.2% 58.3% +0.1%
+ Complexity 4119 4114 -5
===========================================
Files 454 454
Lines 17701 17676 -25
===========================================
Hits 10297 10297
+ Misses 7404 7379 -25
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -0,0 +1,55 @@ | |||
<?php |
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 aware this is a test file but would be nice to document a bit the functions.
module.exports = defineConfig( { | ||
testDir: '../specs', | ||
|
||
/* Maximum time one test can run for. */ |
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.
💅
/* Maximum time in milliseconds one test can run for. */
|
||
expect: { | ||
/** | ||
* Maximum time expect() should wait for the condition to be met. |
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.
Maximum time, in milliseconds, expect() should wait for the condition to be met.
/* Fail the build on CI if you accidentally left test.only in the source code. */ | ||
forbidOnly: !! process.env.CI, |
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 don't understand well this comment.
I will suggest the one appearing below on the web:
https://playwright.dev/docs/test-configuration#:~:text=Whether%20to%20exit%20with%20an%20error%20if%20any%20tests%20are%20marked%20as%20test.only.%20Useful%20on%20CI.
Whether to exit with an error if any tests are marked as "test.only"
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, this is mostly copying and pasting from other examples. I think we can just remove this setting all together.
|
||
/* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ | ||
use: { | ||
/* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ |
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.
/* Maximum time in milliseconds each action such as click()
can take. Defaults to 0 (no limit). */
tests/e2e/global-setup.js
Outdated
console.log( 'Admin state file does not exist.' ); | ||
} else { | ||
console.log( 'Admin state file could not be deleted: ' + err ); | ||
} |
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.
Maybe console.error
might be more accurate here.
tests/e2e/global-setup.js
Outdated
console.log( | ||
`Admin log-in failed, Retrying... ${ i }/${ adminRetries }` | ||
); | ||
console.log( e ); |
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.
Maybe we can use console.warn.
Also, I don't understand well why we need to retry 5 times?
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.
These are the parts that we need to do the same as WC core, but it isn't reusable to the same extent it was when we had the e2e packages. So it's basically copied code from here with the comment further down:
This step was failing occasionally, and globalsetup doesn't retry, so make it retry
I've had it fail in one of the test runs I did in GitHub Actions, so it must be a bit flakey at times (which is resolved with the retry).
Maybe I should add some references where the code is copied from to help explain. I can change the to console.warn / error where appropriate though.
Same with the rest of Add to cart events... I will try to check tomorrow why. |
As mentioned I'd suggest to run the tests from PR #2043 as I did improve a few tests that were still a bit flakey. If they still fail there then I'd suggest to run them with |
@puntope I went ahead and fixed those suggestions you made. Regarding running the tests maybe you can try with the following commands to start with a clean slate (I just ran that locally and all the tests pass):
|
Some conflicts to be resolved in But ready to go. The issue was on my side likely. |
Changes proposed in this Pull Request:
This PR converts the E2E tests from Puppeteer to Playwright and removes any dependencies on the
@woocommerce/e2e-*
packages. This is a followup from #2030Overview of the changes to convert tests:
Conversion ID
The test Conversion ID used to be set through the Connection Test page. This has now been replaced with a custom plugin
tests/e2e/bin/test-data.php
which adds some API endpoints to set this data.Site Settings
The payment gateway COD used to be enabled through an API call at the start of the test, this is now done in the
wp-env
start up script.API requests
All API requests use the basic auth plugin to send requests to the API. We use this for both WordPress and WooCommerce endpoints, see
tests/e2e/utils/api.js
Avoiding
waitForSelector
In our previous tests we often used
waitForSelector
, while this is still compatible with Playwright, I've replaced it with other Locators. This was done to simplify the tests and reduce the need for utility functions.Mocking API requests
Instead of using a utility for mocking and spying on API requests, it's been replaced with the
route.fulfill
. We use this to mock connections to Jetpack or Google.Closes #1599
Detailed test instructions:
nvm use && npm ci
wp-env
environment:npm run wp-env:up
npx playwright install chromium
npm run test:e2e
npm run test:e2e-dev
Additional details:
Currently the E2E workflow is set to run only on release branches or manually. I've triggered a manual run, and all the tests passed: https://github.com/woocommerce/google-listings-and-ads/actions/runs/5753482975/job/15596778163
Changelog entry