Add Playwright tests for All Reviews, Reviews by Product and Reviews by Category blocks #11975
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 1.61 MB ℹ️ View Unchanged
|
Aljullu
added
skip-changelog
Aljullu
changed the title
Aljullu
force-pushed
the
add/reviews-tests
branch
from
41ab3d9
to
f6f4bf9
Compare
woocommercebot
requested review from
a team and
imanish003
and removed request for
a team
| const api = new WooCommerceRestApi( { | ||
| url: baseURL, | ||
| consumerKey: process.env.CONSUMER_KEY, | ||
| consumerSecret: process.env.CONSUMER_SECRET, | ||
| version: 'wc/v3', | ||
| } ); |
There was a problem hiding this comment.
Interesting! I've got a couple of questions about using "WooCommerceRestApi" in our Playwright tests:
- Is it okay to integrate "WooCommerceRestApi" for testing purposes? Specifically, we have some tests for the Product Collection block that require data not available in the sample. Is it a good idea to use this API to create new products before the tests run and then clean them up with an
afterAll? For instance, we need to test scenarios like “Out of Stock” products, as you can see here - Also, do you think there will be any impact when we run all tests in parallel? Currently, our tests run serially, but I'm guessing we'll be shifting to parallel testing soon. What are your thoughts on this?
There was a problem hiding this comment.
Good questions, I'm not 100% sure, to be honest. I copied this approach from WC core, but I'm not sure whether there is a better way of doing that. Pinging @dinhtungdu as he knows a lot about e2e tests.
There was a problem hiding this comment.
Understood. I raised this question primarily to explore whether we could apply the same approach to other areas of our codebase, including the tests for Product Collection. Gaining some clarity on this aspect would be extremely helpful.
@sunyatasattva I would really appreciate your input here 🙂
There was a problem hiding this comment.
We're still searching for a good way to prepare the data for testing. I think using WooCommerce REST API is a good approach we should consider standardizing. For now we're creating test data at the provisioning time, and there is clear needs to create test data/fixture at the test run time. Manish also did some research here for the Seville meetup: per0F9-13G-p2.
Currently, our tests run serially, but I'm guessing we'll be shifting to parallel testing soon.
Even we aim to run the test in parallel and we should write tests in isolation, I think we should only enable parallelism on local, not CI. According to the Playwright docs:
We recommend setting workers to "1" in CI environments to prioritize stability and reproducibility. Running tests sequentially ensures each test gets the full system resources, avoiding potential conflicts. However, if you have a powerful self-hosted CI system, you may enable parallel tests. For wider parallelization, consider sharding - distributing tests across multiple CI jobs.
https://playwright.dev/docs/ci#workers
There was a problem hiding this comment.
From my personal point of view, we should try to find an alternative approach to using the REST API on setup and teardown phases of the test. We have all found that E2E tests already take a long time, and creating fixtures on a test-by-test basis using the API is going to increase the run time of the test. Also, using the API is going to create another dependency, and thus another point of failure (i.e. increase flakiness).
With that said, standardization of course is of great importance. So, establishing repeatable patterns should also be a design priority.
In this case: is there a reason why can't we add a product with reviews on our initial fixture to test these use-cases?
There was a problem hiding this comment.
In this case: is there a reason why can't we add a product with reviews on our initial fixture to test these use-cases?
AFAIK there was a related discussion #10099, where I proposed a standard approach for adding new fixture data, leading to task #10189. The main reason we haven't done this yet is our limited bandwidth during the cooldown period. Maybe we can prioritize this task to work on it during the cooldown 🤔
There was a problem hiding this comment.
Thanks for all your feedback, folks! 🙌
In this case: is there a reason why can't we add a product with reviews on our initial fixture to test these use-cases?
Not really, it was mostly because I took WC core tests as an example. I guess one advantage of using the API is that we can have different tests using different test data. Ie: if at some point we wanted to test how the Reviews blocks behave when there are no reviews in the store, it would be easier to do so if there are no reviews created during the setup.
In any case, that's not the case right now, as we are only testing these blocks when there are reviews. 🙂 So in cc89fa6 I refactored this PR to create the reviews on setup instead of at the beginning of the test. I also removed WCRestApiUtils for now, as it's no longer necessary.
@imanish003 I will need to ask for another review. 🙏
tests/e2e/tests/product-collection/compatibility-layer.block_theme.side_effects.spec.ts
Outdated
Show resolved
Hide resolved
tests/e2e/tests/reviews-by-category/reviews-by-category.block_theme.spec.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the review, @imanish003, I implemented some of the changes. Would you mind taking another look? |
|
Thanks for the reviews, @imanish003! I added some more changes so I asked for another look. 🙏
Added. 👍 |
There was a problem hiding this comment.
Hi @Aljullu,
Changes looks good to me. Also, I really appreciate the reusable utility WCRestApiUtils that you've developed. It's a great addition! 🙌🏻
P.S.: There's an ongoing discussion in the #11975 (comment). I don’t see it as a blocker, so I'm approving the PR.
Aljullu
force-pushed
the
add/reviews-tests
branch
2 times, most recently
from
cbf0d39
to
d18c487
Compare
Aljullu
force-pushed
the
add/reviews-tests
branch
from
d18c487
to
cc89fa6
Compare
|
This PR has been ported to the monorepo: #11975. |
What
Blocked by #11976.Fixes woocommerce/woocommerce#42065.
This PR adds Playwright tests to the All Reviews, Reviews by Product and Reviews by Category blocks and removes the Puppeteer tests for the same blocks.
Why
Hopefully, these tests would have caught these issues: #11912, #11917 and #11974.
Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
Screenshots or screencast
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog