* Copies EntityUnitTestBase to a new EntityKernelTestBase (there are lots of subclasses in many core and contrib modules too)
* Moves subclasses in Entity and Field system tests and all other kernel tests in the Entity system tests to Drupal\KernelTests\Core\Entity
* Adds a fix to AssertLegacyTrait::assertEquals because various tests currently rely on that safe string casting behavior (translatable strings, mostly)

Some fails are remaining, will continue later.

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Category: Bug report » Task
Status: Active » Needs review
FileSize
47.36 KB
Berdir’s picture

This fixes a lot of "Indirect modification of overloaded property Drupal\KernelTests\Core\Entity\EntityTranslationTest::$field has no effect"

Also, assertEqual() in simpletest aparently things that [] == NULL. assertEquals() does not.

Previous patch was messed up, sorry.

The last submitted patch, 2: entity-kernel-tests-2679096-2679096-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: entity-kernel-tests-2679096-2679096-4.patch, failed testing.

Berdir’s picture

Forgot to fix the field tests.

dawehner’s picture

Great work!

+++ b/core/tests/Drupal/KernelTests/AssertLegacyTrait.php
@@ -59,7 +59,7 @@ public static function assertFalse($actual, $message = '') {
   protected function assertEqual($actual, $expected, $message = '') {
-    $this->assertEquals($expected, $actual, $message);
+    $this->assertEquals($this->castSafeStrings($expected), $this->castSafeStrings($actual), $message);
   }

IMHO we should fix this is KernelTestBase::assertEquals instead

Status: Needs review » Needs work

The last submitted patch, 6: entity-kernel-tests-2679096-6.patch, failed testing.

Berdir’s picture

A few more conversions. FieldSettingsTest is failing. Whatever that test is trying to do, it doesn't work the way it thinks:

I think assertEqual() in simpletest doesn't check types of nested array elements and phpunit does. I'm not sure if it wanted to test that the string is converted to a boolean but it doesn't work. After saving, the original object is kept. And even reloading, as I tried, doesn't make it work as then it's replaced again.

Berdir’s picture

This is what I get before making changes:

1) Drupal\KernelTests\Core\Field\FieldSettingsTest::testConfigurableFieldSettings
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'translatable_field_setting' => 'a translatable field setting'
-    'field_setting_from_config_data' => true
+    'field_setting_from_config_data' => 'TRUE'
 )

/home/berdir/Projekte/d8/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:62
/home/berdir/Projekte/d8/core/tests/Drupal/KernelTests/Core/Field/FieldSettingsTest.php:111

And after my changes:

1) Drupal\KernelTests\Core\Field\FieldSettingsTest::testConfigurableFieldSettings
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'translatable_field_setting' => 'a translatable field setting'
-    'field_setting_from_config_data' => true
-    'field_storage_setting_from_config_data' => true
+    'field_setting_from_config_data' => 'TRUE'
+    'storage_setting_from_config_data' => 'TRUE'
 )

Status: Needs review » Needs work

The last submitted patch, 9: entity-kernel-tests-2679096-7.patch, failed testing.

dawehner’s picture

In my debugging this seems to be really caused by \Drupal\field_test\Plugin\Field\FieldType\TestItem::fieldSettingsFromConfigData believing in runtime casting after loading, which of course doesn't exist.

Berdir’s picture

Yeah, I'm just updating the test to look for a string now. Also commented in #2236903: setSettings() on field definitions can unintentionally clear default values

Also fixed another test fail, tried to enable the same module twice. And fixed the namespace of the filter test moved that incorectly.

tim.plunkett’s picture

  1. +++ b/core/tests/Drupal/KernelTests/AssertLegacyTrait.php
    @@ -59,7 +59,7 @@ public static function assertFalse($actual, $message = '') {
    -    $this->assertEquals($expected, $actual, $message);
    +    $this->assertEquals($this->castSafeStrings($expected), $this->castSafeStrings($actual), $message);
    

    Is this in scope?

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityKernelTestBase.php
    @@ -101,7 +99,7 @@ protected function setUp() {
    -      $role = Role::create(array(
    +      $role = entity_create('user_role', array(
    
    @@ -110,10 +108,10 @@ protected function createUser($values = array(), $permissions = array()) {
    -    $account = User::create($values + [
    +    $account = entity_create('user', $values + array(
    ...
    -    ]);
    +    ));
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php
    @@ -2,16 +2,17 @@
    +use Drupal\KernelTests\Core\Entity\EntityLanguageTestBase;
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypeConstraintValidatorTest.php
    @@ -2,11 +2,12 @@
    +use Drupal\system\Tests\Entity\EntityUnitTestBase;
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php
    @@ -2,16 +2,17 @@
    +use Drupal\system\Tests\Entity\EntityUnitTestBase;
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldTranslationSqlStorageTest.php
    @@ -2,14 +2,15 @@
    +use Drupal\KernelTests\Core\Entity\EntityLanguageTestBase;
    

    These changes look wrong, or at least some of them.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldSettingsTest.php
    @@ -27,8 +27,8 @@ class FieldSettingsTest extends EntityUnitTestBase {
    -   * @covers \Drupal\Core\Field\BaseFieldDefinition::getSettings()
    -   * @covers \Drupal\Core\Field\BaseFieldDefinition::setSettings()
    +   * @covers \Drupal\Core\Field\BaseFieldDefinition::getSettings
    +   * @covers \Drupal\Core\Field\BaseFieldDefinition::setSettings
    
    @@ -52,8 +52,8 @@ public function testBaseFieldSettings() {
    -   * @covers \Drupal\field\Entity\FieldStorageConfig::getSettings()
    -   * @covers \Drupal\field\Entity\FieldStorageConfig::setSettings()
    +   * @covers \Drupal\field\Entity\FieldStorageConfig::getSettings
    +   * @covers \Drupal\field\Entity\FieldStorageConfig::setSettings
    
    @@ -79,8 +79,8 @@ public function testConfigurableFieldStorageSettings() {
    -   * @covers \Drupal\field\Entity\FieldStorageConfig::getSettings()
    -   * @covers \Drupal\field\Entity\FieldStorageConfig::setSettings()
    +   * @covers \Drupal\field\Entity\FieldStorageConfig::getSettings
    +   * @covers \Drupal\field\Entity\FieldStorageConfig::setSettings
    

    Out of scope

  4. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldSettingsTest.php
    @@ -106,7 +106,7 @@ public function testConfigurableFieldSettings() {
    -      'field_setting_from_config_data' => TRUE,
    +      'field_setting_from_config_data' => 'TRUE',
    

    This seems like it should be able to stay as a Boolean, is there a fix needed elsewhere?

dawehner’s picture

Out of scope

Well, they are needed in order to let phpunit not complain about it.

This seems like it should be able to stay as a Boolean, is there a fix needed elsewhere?

We debugged that earlier ... simpletest relied on casting 'TRUE' to TRUE, internally. The test though should actually expect that, see \Drupal\field_test\Plugin\Field\FieldType\TestItem::fieldSettingsFromConfigData

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I'm not sure about this being 8.1.x or 8.2.x, leaving here for now.

Duh, good point about @covers \Drupal\field\Entity\FieldStorageConfig::getSettings

Looks great!

dawehner’s picture

Thank you tim!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c3f14f9 and pushed to 8.1.x and 8.2.x. Thanks!

As this is test only code I've committed to 8.1.x. Not cherry-picking to 8.0.x because it feels like unnecessary disruption to the production branch this late in its life cycle.

diff --git a/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php b/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
index 4bdf30c..dbeb858 100644
--- a/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
+++ b/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
@@ -15,7 +15,7 @@
 /**
  * Defines an abstract test base for entity unit tests.
  *
- * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.2.x. Use
+ * @deprecated in Drupal 8.1.x, will be removed before Drupal 8.2.x. Use
  *   \Drupal\KernelTests\Core\Entity\EntityKernelTestBase instead.
  */
 abstract class EntityUnitTestBase extends KernelTestBase {

  • alexpott committed bf1a194 on 8.2.x
    Issue #2679096 by Berdir, dawehner, tim.plunkett: Convert Kernel Tests...
amateescu’s picture

Opened an issue to backport the new test class to 8.0.x: #2683391: Backport EntityKernelTestBase to 8.0.x

Status: Fixed » Closed (fixed)

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