We'll probably want to wait on #2675836: Expand the add to cart form test coverage to introduce some helpers for creating sets of attributes (Color/Size, for example).

CommentFileSizeAuthor
#8 commerce-2689915-variation_title_with_attributes-8.patch11.89 KBczigor
#6 commerce-2689915-variation_title_with_attributes-6.patch12.07 KBczigor
#4 commerce-2689915-variation_title_with_attributes-4.patch11.59 KBczigor
#2 commerce-2689915-variation_title_with_attributes-2.patch2.61 KBczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
Status: Active » Needs review
FileSize
commerce-2689915-variation_title_with_attributes-2.patch2.61 KB
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work 
--- a/modules/product/tests/src/Functional/ProductVariationTitleGenerationTest.php
+++ b/modules/product/tests/src/Functional/ProductVariationTitleGenerationTest.php
@@ -3,6 +3,8 @@
 namespace Drupal\Tests\commerce_product\Functional;
 
 use Drupal\commerce_product\Entity\ProductVariationType;
+use Drupal\Tests\commerce_cart\Functional\CartBrowserTestTrait;
+use Drupal\commerce_product\Entity\ProductVariation;
 
 /**
  * Tests the product variation title generation.
@@ -11,6 +13,8 @@ use Drupal\commerce_product\Entity\ProductVariationType;
  */
 class ProductVariationTitleGenerationTest extends ProductBrowserTestBase {
 
+  use CartBrowserTestTrait;

We're now using CartBrowserTestTrait, belonging to commerce_cart, in commerce_product, even though commerce_product doesn't depend on commerce_cart. For some reason this works, but it's still ugly.
Let's create a ProductAttributeTestTrait next to ProductBrowserTestBase and then use it from both this test and the cart tests.
This trait should have createAttributeSet and createAttributeValue.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
Status: Needs work » Needs review
FileSize
commerce-2689915-variation_title_with_attributes-4.patch11.59 KB

Done. Let's see what the testbot says.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work

Great!

One last thing. The trait has a dependency:

+    $this->attributeFieldManager->createField($attribute, $variation_type->id());

The pattern that we use for this is to let the trait define the property as well (protected $attributeFieldManager;), and then just assign that property in the constructor/setUp of classes that use the trait.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
Status: Needs work » Needs review
FileSize
commerce-2689915-variation_title_with_attributes-6.patch12.07 KB

Fixed that too

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented 
  1. +++ b/modules/cart/tests/src/Functional/CartBrowserTestBase.php
@@ -3,6 +3,7 @@
+use Drupal\Tests\commerce_product\Functional\ProductAttributeTestTrait;

+++ b/modules/cart/tests/src/FunctionalJavascript/CartWebDriverTestBase.php
@@ -4,6 +4,7 @@ namespace Drupal\Tests\commerce_cart\FunctionalJavascript;
+use Drupal\Tests\commerce_product\Functional\ProductAttributeTestTrait;

+++ b/modules/product/tests/src/Functional/ProductAttributeTestTrait.php
@@ -0,0 +1,106 @@
+namespace Drupal\Tests\commerce_product\Functional;

+++ b/modules/product/tests/src/Functional/ProductVariationTitleGenerationTest.php
@@ -3,6 +3,8 @@
+use Drupal\Tests\commerce_cart\Functional\CartBrowserTestTrait;

    Drupal actually supports Drupal\Tests\$module\Traits as a namespace for test traits, so they can be shared between test suites.

    See: http://cgit.drupalcode.org/drupal/tree/core/tests/bootstrap.php#n98

    Maybe we should place this trait there? I might be useful elsewhere. 

  2. +++ b/modules/product/tests/src/Functional/ProductVariationTitleGenerationTest.php
@@ -29,6 +34,7 @@ class ProductVariationTitleGenerationTest extends ProductBrowserTestBase {
+    $this->attributeFieldManager = \Drupal::service('commerce_product.attribute_field_manager');

    We're in a test and should use $this->container.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
FileSize
commerce-2689915-variation_title_with_attributes-8.patch11.89 KB

Fixed 1. We should fix the other traits in a followup issue.
@2: Everywhere in tests we use \Drupal::service() instead of $this->container. Maybe we should open a different issue to fix that.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented

Opened #3020704: Move test Traits to a Traits folder and namespace to fix @1.

  • bojanz committed 256771e on 8.x-2.x authored by czigor 
    Issue #2689915 by czigor: Expand ProductVariationTitleGenerationTest...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Thank you Andras, this looks great.

Status: Fixed » Closed (fixed)

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