Follow-up to #2894733: Convert Commerce Product and ProfileBilling tests to use CommerceMigrateTestTrait

Problem/Motivation

While working on the previous task, several improvements and even a bug was uncovered. Splitting that out to a separate issue. Plus we have #2899153: Cannot add variation to cart, due to failed store detection.

Proposed resolution

The data provider pattern would make these tests easier to read and maintain.

Remaining tasks

  • Code it.
  • Fix code.
  • Review it.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#20 interdiff.txt4.02 KBquietone
#20 2899321-20.patch35.13 KBquietone
#17 2899321-17.patch34.73 KBquietone
#15 2899426-23.patch6.85 KBquietone
#13 interdiff.txt4.79 KBquietone
#13 2899321-13.patch33.03 KBquietone
#8 interdiff.txt12.02 KBquietone
#8 2899321-8.patch30.41 KBquietone
#2 2899321.patch30.2 KBheddn

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue summary: View changes
StatusFileSize
new 2899321.patch30.2 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2899321.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Illinois
commented

I think we need to make d7_product dependent on d7_product_display and use the migration_lookup plugin to find the product_id. When we do that, we also need to make sure installations have at least drupal core 8.3.2 installed. See https://www.drupal.org/node/2861226. And in general, I think we need to start using migration_lookup more often.

quietone’s picture

quietone commented

Why don't we have a product and a product variation migration? The destination on the product migration is plugin: 'entity:commerce_product_variation'. Or is there some magic in the commerce code that will also save a product when adding a variation?

quietone’s picture

quietone commented

Oh heck, I got mixed up because of the migration names. I tend to expect the name to reflect the destination entity. I'd rather see d7_product_display.yml named d7_product.yml and d7_product_display_type.yml named to d7_product_type.yml. Oh and d7_product_type.yml --> d7_product_variation_type.yml and d7_line_item.yml --> d7_order_item.yml.

I think we need to make d7_product dependent on d7_product_display

But d7_product_display depends on d7_product.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

I've added a comment in #2897112: Rename commerce migration yml files. After it lands, I think we need to add a dependency on 8.3.2 and use the migrate_lookup a lot more. We cannot migrate variations and products in a single migration, but we should be able to reference the other migration using the lookup.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2899321-8.patch30.41 KB
new interdiff.txt12.02 KB

I've been looking at this again and using the data provider pattern presents problems. The first is that, as it is currently used in the patch, setup is being run for each input. So, for the ProductVariationTest setup is run 7 times and one product is tested each time. Instead, we should be running setup one and then test the migration of the 7 product variations.

Still, it is attractive to have all the source data in an array. So, I reworked the dataprovider array to pass all the product variations in one large array and then the test was a foreach to test each one. This works just fine. Then I made an error in the source array and ran the test again. The error was change a uid from 0 to 1. The line number where the error occurred is given is the assertProductVariation line, which doesn't help us know on which variation the error occured. Whereas, having a separate assertProductVariation for each entity will let us know which particular product variation is failing.

Because of that I reworked the patch to have the data as an array before each assertion. It is easier to read.
I also modified the ProductTest to execute assertProductEntity before using the product in the loop to test the variation. At least, to me, it seems logical to test the product is correct before using it.

Status: Needs review » Needs work

The last submitted patch, 8: 2899321-8.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review

Setting to NR for feedback on comments in #8

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work

I think data providers can provide all the data in a single pass. So we only run the tests a single time, instead of 7. Not sure if that makes things less readable though.

I'll go with either. But we do have a failed test so back to NW.

quietone’s picture

quietone commented

@heddn, In the second paragraph of #8, I did just that. Using the data provider I passed all the data in one array and tested in a foreach loop.
The problem now is that when an error occurs you don't know on which item the error occured. I don't want to add code that makes testing more difficult.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2899321-13.patch33.03 KB
new interdiff.txt4.79 KB

First, a reroll.

Status: Needs review » Needs work

The last submitted patch, 13: 2899321-13.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2899426-23.patch6.85 KB

Another reroll was needed and now an interdiff fails.

The changes are:
ProductTest - change the product id from 1 to 15 and to setup an array of product information before using assertProduct.
ProductVariationTest - change the product ids to be NULL.

Status: Needs review » Needs work

The last submitted patch, 15: 2899426-23.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2899321-17.patch34.73 KB

Ignore patch in #15, I uploaded the wrong one.

heddn’s picture

heddn
Primary language English
Location Illinois
commented 
  1. +++ b/modules/commerce/tests/src/Kernel/Migrate/d7/ProductTest.php
@@ -47,20 +47,93 @@ class ProductTest extends Commerce1TestBase {
+  public function productVariationTest(array $product) {

+++ b/modules/commerce/tests/src/Kernel/Migrate/d7/ProductVariationTest.php
@@ -42,14 +42,89 @@ class ProductVariationTest extends Commerce1TestBase {
+  public function testTimestamps() {

    Should this move to our test trait class? 

  2. +++ b/modules/commerce/tests/src/Kernel/Migrate/d7/ProductTest.php
@@ -47,20 +47,93 @@ class ProductTest extends Commerce1TestBase {
+      foreach ($productInstance->variations as $variationInstance) {
+        if ($variation['variation_id'] == $variationInstance->target_id) {
+          $found = TRUE;
+        }

    I'd like to see a test that the product id on the variation matches the product id on the product. It would catch what we found in #2894733-13: Convert Commerce Product and ProfileBilling tests to use CommerceMigrateTestTrait and #2899153: Cannot add variation to cart, due to failed store detection.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work
quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2899321-20.patch35.13 KB
new interdiff.txt4.02 KB

1. Moved
2. Testing for the product id on the product variations is already in the outer for loop, in assertProductVariationEntity.

  • heddn committed dd0bb1a on 8.x-2.x authored by quietone 
    Issue #2899321 by quietone, heddn: Refactor Commerce Product and...
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Reviewed & tested by the community

Better test coverage now.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.