This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
E2E: Product Query: Popular filters presets #7749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dinhtungdu
added
skip-changelog
|
The release ZIP for this PR is accessible via: |
|
Size Change: -532 B (0%) Total Size: 1.01 MB
ℹ️ View Unchanged
|
TypeScript Errors ReportFiles with errors: 439 🎉 🎉 This PR does not introduce new TS errors. |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
This reverts commit 7259f2d.
sunyatasattva
suggested changes
Dec 30, 2022
Comment on lines
+135
to
+137
| await expect( $productFiltersPanel ).toMatch( 'In stock' ); | ||
| await expect( $productFiltersPanel ).toMatch( 'Out of stock' ); | ||
| await expect( $productFiltersPanel ).toMatch( 'On backorder' ); |
There was a problem hiding this comment.
Not a big deal, but should this also be perhaps related to the fixture data?
We could have a loop on something like getFixtureProductsData( 'stock_statii' ) and make it more future proof?
There was a problem hiding this comment.
- Fixture doesn't have any information about all available statuses right now, we need to hard-code it if we want to get stock statuses from it.
- In the future, if WC Core changes the default stock statuses, we will need to update the fixture as well. To me, updating this test or updating the fixture is similar. Test should be updated as the WC and WC Blocks update.
- I checked WooCommerce REST API to see if there is any endpoint returning available stock status, but sadly there is none.
- We can centeralize the reference for available stock status options, but it should be a utility or a constants, not in the fixture data.
| it( 'Products are displayed in the correct order', async () => { | ||
| const { productQueryProducts, shortcodeProducts } = | ||
| await setupProductQueryShortcodeComparison( | ||
| '[products orderby="title" order="ASC" limit="9"]' |
There was a problem hiding this comment.
I would refactor these shortcodes in a variable above, to make them less buried and easier to spot.
There was a problem hiding this comment.
I addressed this and your other comments in e60cf03
Comment on lines
55
to
59
| it( 'Editor preview and block frontend display the same products', async () => { | ||
| const { previewProducts, frontEndProducts } = | ||
| await setupEditorFrontendComparison(); | ||
| expect( frontEndProducts ).toEqual( previewProducts ); | ||
| } ); |
There was a problem hiding this comment.
This seems to be repeated in every test case, why not just refactoring it to avoid duplicates?
Comment on lines
99
to
105
| it( 'Products are displayed in the correct order', async () => { | ||
| const { productQueryProducts, shortcodeProducts } = | ||
| await setupProductQueryShortcodeComparison( | ||
| '[products best_selling="true" limit="9"]' | ||
| ); | ||
| expect( productQueryProducts ).toEqual( shortcodeProducts ); | ||
| } ); |
There was a problem hiding this comment.
This is also repeated over and over, the only thing that's changing is the shortcode. So you could refactor it into a function that accepts a shortcode string and that would save lines of code and make it more maintainable.
nefeline
suggested changes
Jan 1, 2023
There was a problem hiding this comment.
Awesome to see these tests in place for the Advanced and Popular Filters, great work! I've added a few recommendations, mostly regarding code reuse and suggestions for the way the tests are organized, so they are more atomic/single-purposed.
Regarding the advanced filters tests, I've added a question and shared some insights over here.
When running the tests locally for popular-filters.test.ts, all of them are consistently passing, except for Best Selling > Editor preview and block frontend display the same products: previewProducts and frontEndProducts are not exactly the same.
| 'Show only products on sale' | ||
| ); | ||
| await toggleAdvancedFilter( 'Sale status' ); | ||
| await expect( $productFiltersPanel ).not.toMatch( |
There was a problem hiding this comment.
What do you think about reusing more code here by extracting common/duplicated lines into utility functions to make the tests easier to read and maintain?
There was a problem hiding this comment.
Can you suggest something here? At the moment, I don't have a good idea to refactor this further.
From the test code I can see the test perform these step:
- Toggle the Sale status filter.
- Verify the panel contains the option label.
- Toggle the Sale status filter again.
- verify the panel doesn't contain the option label anymore.
I'm curious about what you read from the test. If we can do anything to increase the readability, I'm all in.
There was a problem hiding this comment.
I was thinking something along these lines (for the editor preview test, for example):
const toggleSaleStatusFilter = async () => {
await toggleAdvancedFilter( 'Sale status' );
await setCheckbox( await getToggleIdByLabel( 'Show only products on sale' ) );
};
const removeSaleStatusFilter = async () => {
await toggleAdvancedFilter( 'Sale status' );
await unsetCheckbox( await getToggleIdByLabel( 'Show only products on sale' ) );
};
But that is super minor/a nit! The tests are now structured nicely, a-ok to keep them as-is :)
|
Thank you so much for your reviews! @sunyatasattva @nefeline. I addressed your reviews, can you please take another look at this?
I have the same issue running the test locally but can't reproduce it on the CI. Somehow, the tests are only flaky in the local machine, not in CI. |
3 tasks
sunyatasattva
approved these changes
Jan 3, 2023
nefeline
approved these changes
Jan 5, 2023
gigitux
added
the
type: technical debt
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7933
This PR adds E2E tests to verify the functionalities of Popular filters presets. The failed test is related to Gutenberg 14.8.0. The Popular Filter tests are passing: https://github.com/woocommerce/woocommerce-blocks/actions/runs/3805026894/jobs/6472662871#step:12:241
Testing
Automated Tests
To run the Gutenberg only test locally:
wp-envinstance:npm run wp-env destroywp-env:node bin/wp-env-with-gutenberg.jsnpm run wp-env startGUTENBERG_EDITOR_CONTEXTset togutenberg:GUTENBERG_EDITOR_CONTEXT=gutenberg npm run test:e2e tests/e2e/specs/backend/product-query/popular-filters.test.ts