Add e2e tests for attributes count #9500
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. |
7acd251 to
7f4e47f
Compare
|
Size Change: +8.65 kB (+1%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work 🎉 !
I ran the tests locally, and they are all passing, but I have a few notes:
- What do you think about updating the name/description for those tests to something shorter so the entire content can be read (note the ellipsis at the beginning of each line)?
- I'm getting a
Failed to load resource: the server responded with a status of 500 (Internal Server Error)for each one of the tests: are you getting the same?
| '.regenerate_product_attributes_lookup_table input' | ||
| ); | ||
|
|
||
| await page.click( '.regenerate_product_attributes_lookup_table input' ); |
There was a problem hiding this comment.
Instead of regenerating the product attributes lookup table at this point, how about enabling lookup direct updates? That's what we did in our pre-existing e2e tests to circumvent the same problem of not having this table populated.
This setting ensures that the lookup table is immediately populated whenever a new product is created instead of scheduling an action to do so async.
There was a problem hiding this comment.
If there isn't a way to fix this issue in another way, I would prefer to have this step at the setup level.
Do you have any suggestions so that it is descriptive enough otherwise if we have a fail somewhere, all the descriptions would be the same and you would have to count to know which failed. I had previously thought about
No, I am not seeing those 500 errors (sometimes 404s but that happens on other tests too). Are you able to see the PHP logs? Also did you try it with a fresh env? Like
Yes I'v already tried that but it doesn't work. I believe it is because when you import with CSV via the WP importer, that setting is not triggered. I am not sure how the non PW testings are importing the products to know why it works over there. |
|
I think I know why you're seeing the 500s and/or 400s. It is because the Though I am not sure yet why the attributes are not being generated all of a sudden. When I submitted the PR, it was working fine. I am not sure if something changed with the environment like WP version or something else. If I manually create the attributes and page. The tests work fine as shown here:
Note that even with |
|
After some more digging, I found that if you import via the admin UI, it works as expected to create all the product attributes. But when it is imported via the CLI (as configured in the env), it fails to create the product attributes. I don't yet know why though 🤷 |
|
@nefeline - ok I think I fixed it. It was because the CLI command didn't use the |
There was a problem hiding this comment.
Do you have any suggestions so that it is descriptive enough otherwise if we have a fail somewhere, all the descriptions would be the same and you would have to count to know which failed. I had previously thought about 'Filter by Attributes Block - with All products Block 1...2...3'. But it decided against it.
Gotcha! Well, I don't have any strong preferences/suggestions here. If you think there's no way to re-write these phrases without losing their meaning, we can keep them as-is. An example I thought of was:
should count the products correctly based on the filter (color=blue|query_type_color=or)
as a replacement for:
should show the correct count number of products based on the filter (color=blue|query_type_color=or)
But that's just a minor/optional suggestion to improve readability while the tests are running.
I think I know why you're seeing the 500s and/or 400s. It is because the Filter by Attributes Block page wasn't created. The reason why it isn't created successfully is because during import, the product attributes were not created. You can check by going to http://localhost:8889/wp-admin/edit.php?post_type=product&page=product_attributes after you have run npm run env:start. If it's empty, that is the reason.
Though I am not sure yet why the attributes are not being generated all of a sudden. When I submitted the PR, it was working fine. I am not sure if something changed with the environment like WP version or something else. If I manually create the attributes and page. The tests work fine as shown here
ok I think I fixed it. It was because the CLI command didn't use the wp cli wrapper. If you don't mind taking a look at this again.
Awesome 🕵️ work! I can confirm all tests are now passing without any server errors:
Yes I'v already tried that but it doesn't work. I believe it is because when you import with CSV via the WP importer, that setting is not triggered. I am not sure how the non PW testings are importing the products to know why it works over there.
In this particular case, I believe generating the attribute as soon as the products are created is the ideal path and also faster than having them regenerated later on (after the products were already imported), but unfortunately, I don't have much experience with PW, so I can't think of any immediate solution at the top of my head, but tagging @gigitux and @nielslange (considering your work on pdToLP-wu-p2) in case you have any ideas.
I'm pre-approving the PR as the tests are now passing, but I believe ideally we should optimize the way the meta-lookup table is populated at some point so the tests can run a bit faster.
There was a problem hiding this comment.
In #9500 (comment) case, I believe generating the attribute as soon as the products are created is the ideal path and also faster than having them regenerated later on (after the products were already imported), but unfortunately, I don't have much experience with PW, so I can't think of any immediate solution at the top of my head, but tagging @gigitux and @nielslange (considering your work on pdToLP-wu-p2) in case you have any ideas.
@nefeline and @roykho Isn't this part work as expected:
As for the testing names, below I listed a few suggestions. To increase the readability, I listed them in pairs. Each first snippet shows the current name, each second snippet shows my suggestion.
Filter by Attributes Block - with All products BlockFilter by Attributes Block
should show the correct count number of products based on the filter (color=blue|query_type_color=or)shows correct product count for: color=blue, query_type_color=or
should show the correct count number of products based on the filter (color=blue,gray|query_type_color=or)shows correct product count for: color=blue,gray, query_type_color=or
should show the correct count number of products based on the filter (color=blue|query_type_color=or|min_price=15|max_price=40)shows correct product count for: color=blue, query_type_color=or, min_price=15, max_price=40
That said, I also reviewed the PR and the E2E tests work as expected. Therefore, I'm also approving this PR. Thanks for working on this, @roykho. 🙌
tests/e2e-pw/tests/attributes-filter/filter-products-by-attributes-count.block_theme.spec.ts
Outdated
Show resolved
Hide resolved
tests/e2e-pw/tests/attributes-filter/filter-products-by-attributes-count.block_theme.spec.ts
Outdated
Show resolved
Hide resolved
| '.regenerate_product_attributes_lookup_table input' | ||
| ); | ||
|
|
||
| await page.click( '.regenerate_product_attributes_lookup_table input' ); |
There was a problem hiding this comment.
If there isn't a way to fix this issue in another way, I would prefer to have this step at the setup level.
|
Regarding: There's a difference between populating the lookup table at the same time new products are created versus regenerating the lookup tables later on via a separate task. This is an extra step we didn't have to run while working on Puppeteer, but now we do it on Playright. If folks don't have any ideas to solve this at the top of their head, my recommendation is to create a separate issue so we can investigate later on, as performance will certainly be impacted. |
|
@nefeline @nielslange @gigitux Thanks for the feedbacks! I've updated the PR to address as much as possible. Here are the notable changes from the original PR.
With that, if ya'll don't mind giving it another look. Thanks! |
|
Thanks for addressing the feedback and for shortening the tests description as suggested. 🙏 My only remaining concern is still related to #9500 (comment) : since none of us involved in this discussion had any ideas/suggestions to update this configuration at the top of our heads, my recommendation is to create an issue so we can investigate later on (during a cooldown). All tests are passing, but I'm deferring the approval to Luigi & Niels since they requested additional changes regarding the tests setup 👍 . |
|
@nefeline - created a separate issue woocommerce/woocommerce#42355 |
tests/e2e-pw/tests/attributes-filter/filter-products-by-attributes-count.block_theme.spec.ts
Outdated
Show resolved
Hide resolved
Thanks! I added this issue into the scope of woocommerce/woocommerce#42413 🚀 |
There was a problem hiding this comment.
Thanks for addressing all changes, @roykho. I left a comment on a potential future improvement, but that's out of the scope for this PR.
* Add e2e tests for attributes count * Add test with pricing filter and turn on debug to prevent cache * Prevent tests from passing if test page is not loaded * Use WP wrapper to call WC CLI * Refactor to use more of PW methods * Use existing active filters block post for testing * Move prepareAttributes function to global setup
Closes #8873
This does not cover all the tests required from above mentioned issue. It is a starter to gauge if we're going in the right direction as Playwright was just recently added. Open to any suggestions if there are better approaches.
Note that I had to use some hacky solution to get around the generation of the attributes lookup table.
Tests covered:
Testing
Automated Tests
User Facing Testing
npm run env:startnpx playwright installnpm run test:e2e-pw ./tests/e2e-pw/tests/attributes-filter/filter-products-by-attributes-count.block_theme.spec.tsWooCommerce Visibility
Performance Impact
Changelog