Add - Product View Report Test #2261
Conversation
github-actions
bot
added
changelog: add
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/refactor-product-stats #2261 +/- ##
==================================================================
+ Coverage 59.7% 59.9% +0.3%
- Complexity 4176 4178 +2
==================================================================
Files 457 457
Lines 17701 17708 +7
==================================================================
+ Hits 10559 10611 +52
+ Misses 7142 7097 -45
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Hey @jorgemd24, thanks for your work on these tests.
I've left a few comments but nothing is blocking and all tests are passing locally so approving ✅
| $page_size = 800; | ||
|
|
||
| add_filter( | ||
| 'woocommerce_gla_product_view_report_page_size', | ||
| function () use ( $page_size ) { | ||
| return $page_size; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Could you clarify why we're using the woocommerce_gla_product_view_report_page_size filter here? From what I can tell it has no impact on the test itself so is the intention of this to give a more complete picture of the functionality?
There was a problem hiding this comment.
so is the intention of this to give a more complete picture of the functionality?
That's right, this way allows me to test that the filter is used to set the page size, and that the search is receiving the correct page size.
| } | ||
|
|
||
| public function test_get_product_view_report_with_exception() { | ||
| $this->options->method( 'get_merchant_id' )->willReturn( 432 ); |
There was a problem hiding this comment.
We're also setting the return value for get_merchant_id on the options mock in test_get_product_view_report but beyond that we don't do anything with it so instead of repeating this had you considered creating a class property for it and then assigning it to the mock in setUp?
There was a problem hiding this comment.
Thanks for the suggestion, I added here: 6a9a0f9
| * | ||
| * @param string $mc_id The MC center id. | ||
| * @param string $status The status of the product. | ||
| * @param DateTime|null $expiration_date The expiration date of the product. |
There was a problem hiding this comment.
Do we need the $expiration_date parameter? When we set up the product view's in test_get_product_view_report it's not used.
There was a problem hiding this comment.
I use the $expiration_date to create a "GoogleDate", which is then used to simulate the expiration_date here:
I could set a DateTime in the expiration_date response, but I believe this way allows us to simulate a more realistic scenario.
Changes proposed in this Pull Request:
Part of #2146 & #2250
This PR introduces:
product_view.offer_idfrom the MerchantQuery, as we're using the MC-ID to extract theofferID. The product_view.id is necessary in the query and cannot be removed.$product_view_datafromnext_pagetonext_page_tokento maintain consistency with the rest of the code.It looks like the report for free listings isn't covered by tests, so I'll create a follow-up issue to add tests for that section.
Detailed test instructions:
./vendor/bin/phpunitChangelog entry