Product Query unit tests #7747
Product Query unit tests #7747
Conversation
dinhtungdu
added
skip-changelog
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 429 🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 975 kB ℹ️ View Unchanged
|
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
rubikuserbot
requested review from
a team and
sunyatasattva
and removed request for
a team
There was a problem hiding this comment.
Love how you did the tests! Great job. I have a few comments, but I'm pre-approving.
Another comment would be: I am not too familiar with unit testing in PHP and the tools, but it'd be nice if we can more clearly divide certain tests. Is it possible to give them more explanatory names or descriptions? If not, perhaps more clearly dividing the test cases that are about the global query used with the query params on the front-end by the filters, and the ones used in the editor.
Also, we should add test cases in which these overlap. Right now we are testing each function, but the mess happens when we try and set some price or stock status settings in the editor, and then the user will change the front-end filters (or use the FSE templates). We should have a few test cases for those and make sure things come out as expected.
Though we don't need to add to this PR, which is already great. We can create another one as well.
| 'attrs' => array( | ||
| 'namespace' => 'woocommerce/product-query', | ||
| 'query' => array( | ||
| 'posts_per_page' => 6, | ||
| 'orderby' => 'date', | ||
| 'order' => 'desc', | ||
| 'offset' => 0, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Should we perhaps populate this with all the defaults from QUERY_DEFAULT_ATTRIBUTES?
There was a problem hiding this comment.
Make sense! I actually use the wrong data, fixed in 8995798
tests/php/mocks/ProductQueryMock.php
Outdated
| use Automattic\WooCommerce\Blocks\Integrations\IntegrationRegistry; | ||
|
|
||
| /** | ||
| * ProductQueryMock used to test cart block functions. |
There was a problem hiding this comment.
I suppose cart block function was a copy+paste mistake?
There was a problem hiding this comment.
Yes, it is. Thank you for the catch!
tests/php/mocks/ProductQueryMock.php
Outdated
| class ProductQueryMock extends ProductQuery { | ||
|
|
||
| /** | ||
| * We initaite our mock class. |
There was a problem hiding this comment.
| * We initaite our mock class. | |
| * We initialize our mock class. |
initialize? instantiate?
| foreach ( $on_sale_product_ids as $id ) { | ||
| $this->assertContainsEquals( $id, $merged_query['post__in'] ); | ||
| } | ||
| $this->assertNotContains( 384123, $merged_query['post__in'] ); |
There was a problem hiding this comment.
Why are we asserting not containing this specific number?
There was a problem hiding this comment.
My intention is to avoid false-positive tests. But looking at again, this is obvious. I updated this assertion to check for the length of post__in array instead.
@sunyatasattva I'm late updating this PR (I'm sorry about that), so I decided to add tests for overlap cases in 4db2658. Can you take a look?
I'd love to make the test more descriptive. For now, this is what I use to distinguish between test cases:
I agree for now it's hard to scan through the file. We can divide it into multiple test files, but after reading this answer, I tend to keep the current organization. I'm open to suggestions for improving the test name/description. Having fresh eyes on this is really valuable 🙇🏽 . |
| $parsed_block = $this->get_base_parsed_block(); | ||
| $parsed_block['attrs']['query']['orderBy'] = 'rating'; | ||
| $parsed_block['attrs']['query']['__woocommerceStockStatus'] = array( | ||
| 'instock', | ||
| 'onbackorder', | ||
| ); | ||
| $parsed_block['attrs']['query']['__woocommerceAttributes'] = array( |
There was a problem hiding this comment.
I'm a bit confused by the = alignments here. Line 208 is aligned with 209, and 210 with 214. Why are they not all aligned? Does PHP Lint not complain here? These rules are confusing 😅
There was a problem hiding this comment.
Does PHP Lint not complain here?
phpcs will complains if we align = signs, I agree that's they're confusing 😂
This PR adds PHP tests that cover merging queries logic used in the
ProductQueryblock.Fixes #7627
Other Checks
Testing
Automated Tests
WooCommerce Visibility