Problem/Motivation

EntityManagerTest is infamously brittle and hard to wrangle, due to the unnecessary expectations set up on the mocks.

Proposed resolution

Rewrite using phpspec/prophecy (which is included in PHPUnit now)

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Beta eval:
Unfrozen because it will only touch tests.

CommentFileSizeAuthor
#4 2544746-entity-4.patch89.68 KBtim.plunkett
#4 interdiff.txt3.28 KBtim.plunkett
#2 2544746-entity-2.patch89.67 KBtim.plunkett

Comments

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +DX (Developer Experience)

Everyone will agree that this increase the developer experience of working on the entity manager.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new 2544746-entity-2.patch89.67 KB

There are 3 different usages I couldn't figure out. 

$ ag getMock -- core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
177:    $class = $this->getMockClass(EntityInterface::class);
1245:    $storage = $this->getMock(DynamicallyFieldableEntityStorageInterface::class);
1281:    $storage = $this->getMock(DynamicallyFieldableEntityStorageInterface::class);
1321:    $storage = $this->getMock(DynamicallyFieldableEntityStorageInterface::class);
1347:    $storage = $this->getMock(DynamicallyFieldableEntityStorageInterface::class);
1394:    $storage = $this->getMock(DynamicallyFieldableEntityStorageInterface::class);
1521:    return get_class($this->getMockForAbstractClass(EntityHandlerBase::class));
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -343,16 +298,12 @@ public function testGetDefinitionInvalidException() {
-    $apple = $this->getMock('Drupal\Core\Entity\EntityTypeInterface');
-    $apple->expects($this->any())
-      ->method('hasHandlerClass')
-      ->with('storage')
-      ->will($this->returnValue(TRUE));
-    $banana = $this->getMock('Drupal\Core\Entity\EntityTypeInterface');
-    $banana->expects($this->any())
-      ->method('hasHandlerClass')
-      ->with('storage')
-      ->will($this->returnValue(FALSE));
+    $apple = $this->prophesize(EntityTypeInterface::class);
+    $apple->hasHandlerClass('storage')->willReturn(TRUE);
+
+    $banana = $this->prophesize(EntityTypeInterface::class);
+    $banana->hasHandlerClass('storage')->willReturn(FALSE);

So so sweet.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes
StatusFileSize
new interdiff.txt3.28 KB
new 2544746-entity-4.patch89.68 KB

Okay so prophecy doesn't have a mechanism like getMockClass() or getMockForAbstractClass(), just switching get_class($this->getMock()) to $this->getMockClass().

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This is sooooo much better.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -9,19 +9,38 @@
 use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
 use Drupal\Component\Plugin\Exception\PluginNotFoundException;
+use Drupal\Core\Cache\Cache;
+use Drupal\Core\Cache\CacheBackendInterface;
+use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
 use Drupal\Core\Config\Entity\ConfigEntityStorage;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\Entity\ContentEntityInterface;
+use Drupal\Core\Entity\ContentEntityTypeInterface;
+use Drupal\Core\Entity\DynamicallyFieldableEntityStorageInterface;
 use Drupal\Core\Entity\EntityHandlerBase;
 use Drupal\Core\Entity\EntityHandlerInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityManager;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
+use Drupal\Core\Entity\FieldableEntityInterface;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\Field\BaseFieldDefinition;
+use Drupal\Core\Field\FieldDefinitionInterface;
+use Drupal\Core\Field\FieldStorageDefinitionInterface;
+use Drupal\Core\Field\FieldTypePluginManagerInterface;
+use Drupal\Core\Field\Plugin\Field\FieldType\BooleanItem;
+use Drupal\Core\KeyValueStore\KeyValueFactoryInterface;
+use Drupal\Core\KeyValueStore\KeyValueStoreInterface;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Language\LanguageInterface;
+use Drupal\Core\Language\LanguageManagerInterface;
+use Drupal\Core\StringTranslation\TranslationInterface;
+use Drupal\Core\TypedData\TypedDataManager;
 use Drupal\Tests\UnitTestCase;
+use Prophecy\Argument;
 use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;

33 use statments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

See #2337191: Split up EntityManager into many services for a way to fix that :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I've confirmed that 51 test methods are run before and after this change and that the provider have not changed. Looks good and yes anything to make changing this test easier will be nice.

Committed bf8723f and pushed to 8.0.x. Thanks!

  • alexpott committed bf8723f on 8.0.x 
    Issue #2544746 by tim.plunkett: Rewrite EntityManagerTest to use phpspec...

Status: Fixed » Closed (fixed)

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