Use PHPUnit to test the Entity, ConfigEntityBase, and ContentEntityBase classes. In order to do that, add wrappers to access services and procedural code.

CommentFileSizeAuthor
#116 drupal_2134857_116.patch57.29 KBXano
#114 2134857-114-interdiff.txt12.92 KBBerdir
#114 2134857-114.patch57.56 KBBerdir
#111 interdiff-2134857-111.txt827 bytesdamiankloip
#111 2134857-111.patch57.61 KBdamiankloip
#109 drupal_2134857-108-toolbar.patch59.25 KBWim Leers
#109 drupal_2134857-108.patch58.8 KBWim Leers
#106 drupal_2134857-106-toolbar.patch59.26 KBWim Leers
#106 interdiff.txt7.46 KBWim Leers
#106 drupal_2134857-106.patch58.81 KBWim Leers
#94 interdiff.txt1.53 KBWim Leers
#94 drupal_2134857-94-toolbar.patch60.27 KBWim Leers
#94 drupal_2134857-94.patch59.82 KBWim Leers
#85 drupal_2134857-82-toolbar.patch58.96 KBWim Leers
#85 drupal_2134857-82-toolbar.patch58.96 KBWim Leers
#82 drupal_2134857-82-toolbar.patch58.96 KBBerdir
#82 drupal_2134857-82-interdiff.txt15.29 KBBerdir
#82 drupal_2134857-82.patch58.57 KBBerdir
#73 interdiff.txt7.37 KBXano
#73 drupal_2134857_73.patch57.64 KBXano
#70 drupal_2134857_70.patch56.9 KBXano
#68 drupal_2134857_68.patch57.4 KBXano
#62 drupal_2134857_62.patch57.46 KBXano
#57 interdiff.txt726 bytesXano
#57 drupal_2134857_57.patch57.81 KBXano
#55 interdiff.txt846 bytesXano
#55 drupal_2134857_55.patch57.35 KBXano
#53 drupal_2134857_51.patch56.66 KBXano
#48 drupal_2134857_48.patch1.42 KBXano
#43 interdiff.txt21.74 KBXano
#43 drupal_2134857_43.patch57.36 KBXano
#41 interdiff.txt4.27 KBXano
#41 drupal_2134857_41.patch57.03 KBXano
#36 drupal_2134857_36.patch57.05 KBXano
#36 interdiff.txt886 bytesXano
#34 interdiff.txt2.65 KBXano
#34 drupal_2134857_34.patch57.05 KBXano
#31 interdiff.txt29.02 KBXano
#31 drupal_2134857_31.patch58.38 KBXano
#29 interdiff.txt4.37 KBXano
#29 drupal_2134857_29.patch62.46 KBXano
#27 drupal_2134857_27.patch59.55 KBXano
#25 drupal_2134857_25.patch57.46 KBXano
#18 interdiff.txt13.51 KBXano
#17 drupal_2134857_17.patch61.63 KBXano
#13 drupal_2134857_13.patch62.01 KBXano
#13 interdiff.txt32.6 KBXano
#10 drupal_2134857_10.patch47.79 KBXano
#10 interdiff.txt8.06 KBXano
#8 drupal_2134857_8.patch9.27 KBXano
#8 interdiff.txt47.28 KBXano
#5 interdiff.txt10.17 KBXano
#5 drupal_2134857_5.patch41.38 KBXano
#3 interdiff.txt16.43 KBXano
#3 drupal_2134857_3.patch38.5 KBXano
#2 drupal_2134857_1.patch28.51 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Assigned: Unassigned » Xano
Xano’s picture

Title: Make ContentEntityBase use DI and test with PHPUnit » Make the entity base classes use DI and test them with PHPUnit
Status: Active » Needs work
FileSize
28.51 KB

Work is still in progress, but feedback is welcome. \Drupal\Core\Entity\Entity is tested and I will continue work on ContentEntityBase.

Xano’s picture

Status: Needs work » Needs review
FileSize
38.5 KB
16.43 KB

There should be exactly 50 failures.

Status: Needs review » Needs work

The last submitted patch, 3: drupal_2134857_3.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
41.38 KB
10.17 KB
Xano’s picture

I would love some help with testing the methods used for/with typed data. They're sometimes complex and sometimes require magic methods and I'm not sure how to deal with that.

Status: Needs review » Needs work

The last submitted patch, 5: drupal_2134857_5.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
47.28 KB
9.27 KB

I really need some help with the typed data stuff. This patch also removes a few Simpletest tests that are no longer necessary and I believe we can remove some (parts of) others as well.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_2134857_8.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
8.06 KB
47.79 KB

The previous patch was incomplete.

Status: Needs review » Needs work

The last submitted patch, 10: drupal_2134857_10.patch, failed testing.

Xano’s picture

ConfigEntityBase::sort() needs work, because it depends on a public weight property, that doesn't necessarily have to be present on the entities the callback is supposed to check. We can either extend ConfigEntityInterface with weight methods now, or we can introduce a Weighted(Entity)Interface with a trait now that core will require PHP 5.4, so config entities can optionally implements weights.

Another issue is ConfigEntityBase::preSave(), which requires an EntityStorageControllerInterface::getQuery() method. However, that method only exists on ConfigStorageController.

Xano’s picture

Status: Needs work » Needs review
FileSize
32.6 KB
62.01 KB
  • ConfigEntityBase now has full test coverage.
  • ConfigEntityBase::sort() only accepts array items that implement ConfigEntityInterface, partly because it requires the label() method.
  • ConfigEntityBase::preSave() now checks if the storage controller extends ConfigStorageController before calling the class-specific getQuery() method on it.
  • Part of the patch has been re-rolled because #2133469: Replace path-based entity links with route names was committed in the meantime.

ContentEntityBase is still unfinished because of typed data.

tim.plunkett’s picture

The title is misleading. This isn't really DI, but it is a good idea! Can you retitle it?

For all of the service retrieval methods, please use the pattern we've adopted in core:

if (!$this->someService) {
  $this->someService = \Drupal::service('some_service');
}
return $this->someService;
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -185,21 +196,24 @@ public function getExportProperties() {
    +    if ($storage_controller instanceof ConfigStorageController) {
    

    When would this not be ConfigStorageController?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -167,6 +167,36 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
    +  protected function checkPlain($text) {
    +    return check_plain($text);
    ...
    +  protected function formatString($string, array $args = array()) {
    +    return format_string($string, $args);
    

    We already have String::checkPlain and String::format, no need for this

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -109,8 +136,8 @@ public function bundle() {
    -    if (isset($entity_info['label_callback']) && function_exists($entity_info['label_callback'])) {
    -      $label = $entity_info['label_callback']($this->entityType, $this, $langcode);
    +    if (isset($entity_info['label_callback']) && is_callable($entity_info['label_callback'])) {
    +      $label = call_user_func($entity_info['label_callback'], $this->entityType, $this, $langcode);
    
    @@ -191,8 +218,8 @@ public function uri($rel = 'canonical') {
    -    if (isset($uri_callback) && function_exists($uri_callback)) {
    -      $uri = $uri_callback($this);
    +    if (isset($uri_callback) && is_callable($uri_callback)) {
    +      $uri = call_user_func($uri_callback, $this);
    

    How is this in scope?

  4. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -251,13 +278,13 @@ public function uriRelationships() {
    -  public function access($operation = 'view', AccountInterface $account = NULL) {
    +  public function access($operation, AccountInterface $account = NULL) {
    
    +++ b/core/modules/block/block.module
    @@ -239,7 +239,7 @@ function _block_get_renderable_region($list = array()) {
    -      if ($block->access()) {
    +      if ($block->access('view')) {
    

    How is this in scope either?

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldSqlStorageTest.php
    @@ -258,7 +258,7 @@ function testFieldWrite() {
         // Use one of the longest entity_type names in core.
    -    $entity_type = $bundle = 'entity_test_label_callback';
    +    $entity_type = $bundle = 'entity_test_label';
    

    This change contradicts the comment.

  6. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +  protected $entity;
    ...
    +  protected $entityManager;
    ...
    +  protected $routeProvider;
    ...
    +   * @var \Drupal\Component\Uuid\UuidInterface
    ...
    +    $this->uuid = $this->getMock('\Drupal\Component\Uuid\UuidInterface');
    

    These should also have \PHPUnit_Framework_MockObject_MockObject separated by |

Status: Needs review » Needs work

The last submitted patch, 13: drupal_2134857_13.patch, failed testing.

damiankloip’s picture

Quick pass:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -515,13 +543,13 @@ public function isEmpty() {
    +  public function access($operation, AccountInterface $account = NULL) {
    

    Where's the default gone?

  2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +    $this->assertSame(spl_object_hash($this->entity), spl_object_hash($this->entity->enforceIsNew()));
    

    testIsNew, but this is also using enforceIsNew?

    Also, all of these calls to assertSame() don't need to do spl_object_hash(), PHPUnit will assert whether the object is the same instance or not for you.

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +    $this->entityManager->expects($this->at(0))
    

    This can just use $this->once()

  4. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +    @usort($list, array('\Drupal\Core\Config\Entity\ConfigEntityBase', 'sort'));
    

    This can just use one static string: 'MyClass::sortMethod'

  5. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +  public function testPreSave() {
    +    // This method is internal, so check for errors on calling it only.
    +    $storage = $this->getMock('\Drupal\Core\Entity\EntityStorageControllerInterface');
    +    $this->entity->preSave($storage);
    

    This doesn't really test alot, and fails if we are trying to adhere to strict standards, are we? not sure :)

  6. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,582 @@
    +    $values = array();
    

    Do we need a variable for this?

  7. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,402 @@
    +    $callback_container = $this->getMock(get_class());
    +    $callback_container->expects($this->once())
    +      ->method(__FUNCTION__)
    +      ->will($this->returnValue($uri));
    

    huh?

Also, Tim is right. The issue title is totally not what is going on here :-) it also feels a bit like most of the changes are solely for making it testable in phpunit.

Xano’s picture

Title: Make the entity base classes use DI and test them with PHPUnit » PHPUnit test the entity base classes
Issue summary: View changes
Status: Needs work » Needs review
FileSize
61.63 KB

For all of the service retrieval methods, please use the pattern we've adopted in core:

Done.

When would this not be ConfigStorageController?

Done. I know it was agreed upon that configuration entity storage is not swappable after all, but old habits die hard.

We already have String::checkPlain and String::format, no need for this

Done.

How is this in scope?

  1. It is an improvement.
  2. I am not (yet) a fan of using additional code for testing, so this allows us to use a mock method for testing.

How is this in scope either?

Where's the default gone?

#2095125: Use access constants in every access control context removed the default argument from the interface and I saw removing it from the classes as a clean-up. It also prevents us from having to test it.

This change contradicts the comment.

Do you have a suggestion? taxonomy_vocabulary?

These should also have \PHPUnit_Framework_MockObject_MockObject separated by |

Done.

testIsNew, but this is also using enforceIsNew?

The method tests the getter and the setter. Is there another way to test these separately without using Reflection to test the property values, which really tests the internals rather than the integration?

Also, all of these calls to assertSame() don't need to do spl_object_hash(), PHPUnit will assert whether the object is the same instance or not for you.

You're right that it doesn't matter for the assert, but in case of failures, it's easier to compare hashes than var_export() output, and it doesn't mess up the Simpletest UI as much.

This can just use $this->once()

Done.

This can just use one static string: 'MyClass::sortMethod'

Done.

This doesn't really test alot, and fails if we are trying to adhere to strict standards, are we? not sure :)

What do you mean with strict standards? At the very least this test ensures that the method can be called without trouble.

Do we need a variable for this?

Not necessarily, but I find it more descriptive than just passing on an empty array directly.

huh?

It might have been fubhy who called this wicked. All we need is a mock of an arbitrary class so we can test that the method is called. It does not matter what class the mock is of, so I chose an existing class, rather than adding a new one just for this.

Xano’s picture

FileSize
13.51 KB

Here's the interdiff of the last patch.

The last submitted patch, 17: drupal_2134857_17.patch, failed testing.

dawehner’s picture

Issue tags: +PHPUnit

Missing phpunit tag.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -57,6 +57,13 @@ public function __construct(array $values, $entity_type) {
       /**
    +   * Wraps format_string().
    +   */
    +  protected function formatString($string, array $args = array()) {
    +    return format_string($string, $args);
    +  }
    +
    +  /**
    

    Just an idea use \Drupal\Component\Utility\String::format()

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -515,13 +530,13 @@ public function isEmpty() {
    -  public function access($operation = 'view', AccountInterface $account = NULL) {
    +  public function access($operation, AccountInterface $account = NULL) {
    

    +1 to sync the signature.

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +/**
    + * Tests \Drupal\Core\Config\Entity\ConfigEntityBase;
    + */
    +class ConfigEntityBaseUnitTest extends UnitTestCase {
    

    Let's use @group Drupal, and maybe even @group Entity or Config.

  4. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +    $this->routeProvider = $this->getMockBuilder('\Drupal\Core\Routing\RouteProvider')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    

    Don't we also have an interface for that?

  5. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +
    +    $this->entity = $this->getMockBuilder('\Drupal\Core\Config\Entity\ConfigEntityBase')
    +      ->setConstructorArgs(array($values, $this->entityType))
    +      ->setMethods(array('entityManager', 'formatString', 'languageLoad', 'routeProvider', 'uuidGenerator'))
    +      ->getMock();
    +    $this->entity->expects($this->any())
    +      ->method('formatString')
    +      ->will($this->returnArgument(0));
    +    $this->entity->expects($this->any())
    +      ->method('entityManager')
    +      ->will($this->returnValue($this->entityManager));
    +    $this->entity->expects($this->any())
    +      ->method('routeProvider')
    +      ->will($this->returnValue($this->routeProvider));
    +    $this->entity->expects($this->any())
    +      ->method('uuidGenerator')
    +      ->will($this->returnValue($this->uuid));
    +    $this->entity->expects($this->any())
    +      ->method('languageLoad')
    +      ->will($this->returnValue(NULL));
    

    Instead of mocking the actual tested class you can also create a TestConfigEntityBase class in the same file as the test, which "mocks" these methods in real code.

  6. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,576 @@
    +/**
    + * Tests \Drupal\Core\Entity\ContentEntityBase;
    + */
    

    see @group Drupal

  7. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,576 @@
    +    $callback_container = $this->getMock(get_class());
    +    $callback_container->expects($this->once())
    +      ->method(__FUNCTION__)
    +      ->will($this->returnValue($callback_label));
    ...
    +        'label_callback' => array($callback_container, __FUNCTION__),
    

    Doesn't __FUNCTION__ refer to testLabel(), this code is damn complicated.

Berdir’s picture

As discussed in IRC, I think it's perfectly fine to ignore the Fields/TypedData stuff in ContentEntityBase or actually almost everything in that class (since it's almost completely about fields and field (list) classes). That's complicated stuff and shouldn't block this issue, but a follow-up would be great.

That said, I still think that #1867228: Make EntityTypeManager provide an entity factory is the most important issue right now in regards to all the factory/service/DIC stuff.

Xano’s picture

17: drupal_2134857_17.patch queued for re-testing.

The last submitted patch, 17: drupal_2134857_17.patch, failed testing.

Xano’s picture

FileSize
57.46 KB

Re-roll, plus I addressed some feedback, and removed some unfinished testing methods that were for typed data.

Just an idea use \Drupal\Component\Utility\String::format()

Done.

Let's use @group Drupal, and maybe even @group Entity or Config.

Added @group Drupal.

Don't we also have an interface for that?

Done.

Status: Needs review » Needs work

The last submitted patch, 25: drupal_2134857_25.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
59.55 KB

Minor changes. Uploading, because I am getting unexplained test failures locally. Maybe the testbot can give some more detailed information.

Status: Needs review » Needs work

The last submitted patch, 27: drupal_2134857_27.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
62.46 KB
4.37 KB

I'm still experiencing some PHPUnit errors locally that don't show up on screen or in my logs.

Status: Needs review » Needs work

The last submitted patch, 29: drupal_2134857_29.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
58.38 KB
29.02 KB

Status: Needs review » Needs work

The last submitted patch, 31: drupal_2134857_31.patch, failed testing.

dawehner’s picture

Sorry this was not a really good review to be honest, as the time is already ongoing.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -64,6 +64,8 @@ public function setStatus($status);
    +   * @return self
    
    @@ -109,6 +111,8 @@ public function get($property_name);
    +   * @return self
    

    We do use @return $this in those usecases now.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -529,13 +543,13 @@ public function isEmpty() {
    -  public function access($operation = 'view', AccountInterface $account = NULL) {
    

    Why do we change the default operation?

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    --- a/core/lib/Drupal/Core/Entity/Entity.php
    +++ b/core/lib/Drupal/Core/Entity/Entity.php
    
    +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -24,6 +24,27 @@
    +   * @var \Drupal\Core\Entity\EntityTypeInterface|\PHPUnit_Framework_MockObject_MockObject
    

    TF?

  4. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -65,6 +65,8 @@ public function isNew();
    +   * @return self
    

    see before

  5. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php
    @@ -67,7 +68,9 @@
    +  public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) {
    +    /** @var \Drupal\entity\EntityDisplayModeInterface $a */
    +    /** @var \Drupal\entity\EntityDisplayModeInterface $b */
    

    oh php (btw. "oh php" is really easy to type)

  6. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    + * @coversDefaultClass \Drupal\Core\Config\Entity\ConfigEntityBase
    

    <3

  7. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    + * @group Drupal
    

    What about using @group Config as well?

  8. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +   * @covers ::setOriginalId
    +   * @covers ::getOriginalId
    

    Where have you found that syntax? This is kinda cool

  9. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    + * @group Drupal
    

    @group Config

  10. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +      'description' => '',
    +      'name' => '\Drupal\Core\Entity\ContentEntityBase unit test',
    

    Maybe we could swap this two lines.

  11. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,335 @@
    + * @group Drupal
    + */
    +class EntityUnitTest extends UnitTestCase {
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +class ContentEntityBaseUnitTest extends UnitTestCase {
    
    +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,272 @@
    +class ConfigEntityBaseUnitTest extends UnitTestCase {
    

    @group Entity + I wanna use the "{$testedClass}Test" pattern as it allows you to switch with one key combination (ctrl shift t for me) between test and implementation. Beside of that handy feature this is kind of a standard already in core.

  12. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,335 @@
    +    $this->routeProvider = $this->getMockBuilder('\Drupal\Core\Routing\RouteProvider')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    

    Afaik we do have an interface for that.

  13. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,335 @@
    +  }
    +}
    

    Lets use a newline at the end.

Xano’s picture

Status: Needs work » Needs review
FileSize
57.05 KB
2.65 KB

Why do we change the default operation?

+1 to sync the signature.

@dawehner: both quotes are yours and from this issue ;-)

TF?

Which is short for...?

Where have you found that syntax? This is kinda cool

It's in the official PHPUnit documentation, but timplunkett made me aware of it.

Status: Needs review » Needs work

The last submitted patch, 34: drupal_2134857_34.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
886 bytes
57.05 KB

Status: Needs review » Needs work

The last submitted patch, 36: drupal_2134857_36.patch, failed testing.

tim.plunkett’s picture

I'm pretty sure TF is the last two letters of WTF.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
@@ -0,0 +1,272 @@
+    $properties = $this->entity->getExportProperties();
+    $this->assertInternalType('array', $properties);

+++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
@@ -0,0 +1,333 @@
+  public function testGetString() {
+    $this->assertInternalType('string', $this->entity->getString());
+  }

This isn't really testing *anything*. It seems like it's just here for code coverage percentage reasons?

Xano’s picture

I'm pretty sure TF is the last two letters of WTF

Even then I still don't know what the problem is. Please elaborate.

This isn't really testing *anything*. It seems like it's just here for code coverage percentage reasons?

It does test something, but I agree it's not much. All the interface says the method must do is Returns a string representation of the data., so I'm not sure there is much else we can test without getting very implementation-specific. Feedback and ideas are always welcome, though :)

tim.plunkett’s picture

You have \PHPUnit_Framework_MockObject_MockObject in core/lib/Drupal/Core/Entity/Entity.php.

Xano’s picture

Status: Needs work » Needs review
FileSize
57.03 KB
4.27 KB

Well, that's an embarrassing error...

Status: Needs review » Needs work

The last submitted patch, 41: drupal_2134857_41.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
57.36 KB
21.74 KB

Status: Needs review » Needs work

The last submitted patch, 43: drupal_2134857_43.patch, failed testing.

The last submitted patch, 43: drupal_2134857_43.patch, failed testing.

Xano’s picture

43: drupal_2134857_43.patch queued for re-testing.

The last submitted patch, 43: drupal_2134857_43.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

Re-roll because $entity_info was renamed to $entity_type in a few places.

Status: Needs review » Needs work

The last submitted patch, 48: drupal_2134857_48.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review

I think has been fixed in commit 878777a842d8c12545ce5152c8e117798a7ab4dd

Can someone confirm and close this issue

The last submitted patch, 2: drupal_2134857_1.patch, failed testing.

The last submitted patch, 2: drupal_2134857_1.patch, failed testing.

Xano’s picture

FileSize
56.66 KB

Well-spotted! However, the patch from #48 was faulty, as you can see by comparing the file size and contents to the patch from #43. I must have accidentally messed up when I created the re-roll.

Here is a proper re-roll from #43 that does contain the correct code and applies to HEAD.

Status: Needs review » Needs work

The last submitted patch, 53: drupal_2134857_51.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
57.35 KB
846 bytes

Added DependencySerialization to entities.

Status: Needs review » Needs work

The last submitted patch, 55: drupal_2134857_55.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
57.81 KB
726 bytes
dawehner’s picture

+++ b/core/modules/block/block.module
@@ -222,7 +222,7 @@ function _block_get_renderable_region($list = array()) {
-      if ($block->access()) {
+      if ($block->access('view')) {

What are the reasons for those changes?

Xano’s picture

What are the reasons for those changes?

You asked this before in #33 and I answered the question in #17 and #34. Please know that this is fine, but I just want to illustrate that more people noticed this change and I have not simply ignored questions about it).

You already agreed on this change in #21:

+1 to sync the signature.

But in short, the reason I changed this is that the interface no longer has a default operation and as the PHPUnit test tests the class's interface implementations, I wanted the code to reflect this.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

sorry for that.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

Xano’s picture

Status: Needs work » Needs review
FileSize
57.46 KB

Re-roll.

RTBC, as this was the only reject, because the change was already made in another issue:

diff a/core/lib/Drupal/Core/Entity/ContentEntityBase.php b/core/lib/Drupal/Core/Entity/ContentEntityBase.php	(rejected hunks)
@@ -490,7 +513,7 @@ public function getPropertyDefinition($name) {
   public function getPropertyDefinitions() {
     if (!isset($this->fieldDefinitions)) {
       $bundle = $this->bundle != $this->entityTypeId ? $this->bundle : NULL;
-      $this->fieldDefinitions = \Drupal::entityManager()->getFieldDefinitions($this->entityTypeId, $bundle);
+      $this->fieldDefinitions = $this->entityManager()->getFieldDefinitions($this->entityTypeId, $bundle);
     }
     return $this->fieldDefinitions;
   }

Xano’s picture

Status: Needs review » Reviewed & tested by the community
Xano’s picture

Issue tags: -Needs reroll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 62: drupal_2134857_62.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

62: drupal_2134857_62.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 62: drupal_2134857_62.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
57.4 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 68: drupal_2134857_68.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
56.9 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

My main concern is that not all the tests appear to be testing that much.

  1. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,227 @@
    +
    +  /**
    +   * @covers ::getExportProperties
    +   */
    +  public function testGetExportProperties() {
    +    $properties = $this->entity->getExportProperties();
    +    $this->assertInternalType('array', $properties);
    +  }
    

    This would lead to a false sense of coverage. We should be testing the implementation details of getExportProperties here - how about asserting that is does not contain private /protected properties (isSyncing for example) and contains expected public properties - for example uuid or status.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,316 @@
    +  public function testGetString() {
    +    $this->assertInternalType('string', $this->entity->getString());
    +  }
    

    This does not seem to be testing the functionality of getString()

  3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,325 @@
    +  public function testPostDelete() {
    +    // This method is internal, so check for errors on calling it only.
    +    $storage = $this->getMock('\Drupal\Core\Entity\EntityStorageControllerInterface');
    +    $this->entity->postDelete($storage, array($this->entity));
    +  }
    

    Should we not test that onSaveOrDelete is called? And should we not be testing the logic in the onSaveOrDelete method here? i.e asserting the resetCache method is called on the viewbuilder for referenced entities?

Xano’s picture

Status: Needs work » Needs review
FileSize
57.64 KB
7.37 KB

I addressed the feedback from #72.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Those changes looked like they addressed concerns of not really testing anything :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 236a40b and pushed to 8.x. Thanks!

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
@@ -0,0 +1,332 @@
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100644
new mode 100755
 

Huh? Not reason to do this. Fixed in commit

damiankloip’s picture

Xano, git core.filemode setting could be your friend :)

Xano’s picture

Awesome, thanks @alexpott!

@damiankloip: Will look into that. I probably did a bad job at resetting Git after re-installing core. Thanks :)

webchick’s picture

Status: Fixed » Needs work

Unfortunately, this patch caused random testbot failures in Toolbar module of all the silly things, see #2216701: Random test failure in ToolbarAdminMenuTest for an epic sleuthing adventure. The patch there rolls this one back, so back to "needs work."

alexpott’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -61,6 +76,39 @@ public function __construct(array $values, $entity_type) {
    +  protected function languageLoad($langcode) {
    +    return language_load($langcode);
    +  }
    
    +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,233 @@
    +    $this->entity = $this->getMockBuilder('\Drupal\Core\Config\Entity\ConfigEntityBase')
    +      ->setConstructorArgs(array($values, $this->entityType))
    +      ->setMethods(array('languageLoad'))
    +      ->getMock();
    +    $this->entity->expects($this->any())
    +      ->method('languageLoad')
    +      ->will($this->returnValue(NULL));
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,326 @@
    +    $this->entity = $this->getMockBuilder('\Drupal\Core\Entity\ContentEntityBase')
    +      ->setConstructorArgs(array($values, $this->entityType, $this->bundle))
    +      ->setMethods(array('languageList', 'languageLoad'))
    +      ->getMockForAbstractClass();
    +    $this->entity->expects($this->any())
    +      ->method('languageList')
    +      ->will($this->returnValue(array()));
    +    $this->entity->expects($this->any())
    +      ->method('languageLoad')
    +      ->will($this->returnValue(NULL));
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,332 @@
    +    $this->entity = $this->getMockBuilder('\Drupal\Core\Entity\Entity')
    +      ->setConstructorArgs(array($values, $this->entityTypeId))
    +      ->setMethods(array('languageLoad'))
    +      ->getMock();
    +    $this->entity->expects($this->any())
    +      ->method('languageLoad')
    +      ->will($this->returnValue(NULL));
    

    I think we should replace language_load() with a call to \Drupal::languageManager()->getLanguage($langcode); and we should mock the language manager. Then we can use PHPUnit's getMockForAbstract which means we can test all concrete implementations in these classes.

  2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,233 @@
    +  protected $entityInfo;
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,326 @@
    +  protected $entityInfo;
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,332 @@
    +  protected $entityInfo;
    

    This should be $entityType

  3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,233 @@
    +  /**
    +   * The type of the entity under test.
    +   *
    +   * @var string
    +   */
    +  protected $entityType;
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,326 @@
    +  /**
    +   * The type of the entity under test.
    +   *
    +   * @var string
    +   */
    +  protected $entityType;
    

    Similarly this should be $entityTypeId

I have no idea why this caused the segmentation faults though - but my guess is something to do with the serialization code. Perhaps in the service wrappers lets not store a reference to the object on the class.

alexpott’s picture

It seems this patch also might have caused #2216909: Random test failure in ConfigTranslationUiTest

xjm’s picture

Priority: Normal » Major

Per @Wim Leers, #2217749: Entity base class should provide standardized cache tags and built-in invalidation is currently blocked on this, and that issue is currently filed as a beta blocker. It might be more of a beta target, but let's consider this one at least a major for that reason.

Berdir’s picture

Status: Needs work » Needs review
FileSize
58.57 KB
15.29 KB
58.96 KB

So here's a an update to get started, addressed the review from above. Let's see if that still causes random fails in the toolbar test. (uploading a toolbar test for that, with the hacks from https://drupal.org/node/2216701 to just test that class).

That said, I'm not so sure about certain parts here, I'd prefer not adding fake test coverage for methods when we know that w'ere not properly testing them (some special cases not being tested is fine, I mean stuff like testIsNew() and testUuid().

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 82: drupal_2134857-82-toolbar.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.96 KB
58.96 KB

Re-uploading the test patch in #82, just to see if we get the same or similar results. Because at least for me, the failed tests output in #82 makes no sense.

Wim Leers’s picture

They came back green. That's 40 times no test failure. It would almost seem like #82 suffered from a random testbot problem. Let's see if we can get another 40 test runs that are green. In which case I think it's safe to assume that #82 indeed contains the solution.

Wim Leers’s picture

Wim Leers’s picture

Xano’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -68,30 +75,30 @@ public static function getInfo() {
    +      ->will($this->returnValue(NULL));
    

    Isn't this redundant? Mocked methods return NULL anyway, and the expectation does not need to be set.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -107,22 +114,22 @@ public function setUp() {
    +      ->will($this->returnValue(NULL));
    

    Same here.

  3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -77,28 +84,29 @@ public function setUp() {
    -      ->will($this->returnValue(NULL));
    

    And here.

We should also add setters instead of setting a custom container in the test, but we may want to fix the failures before we address this particular issue.

alexpott’s picture

@Xano

We should also add setters instead of setting a custom container in the test

Why?

tim.plunkett’s picture

->will($this->returnValue(NULL)); is redundant when the matcher is $this->any(), which is true in some places in the patch.
$this->once(), $this->atLeastOnce(), or $this->exactly($count) might be better choices.

There are some cases it's used with $this->at($index), which is good

Berdir’s picture

Yep, and some of those cases are IMHO actually wrong. getLanguage() for example should verify that a valid language is passed in and should return the corresponding language object, right now, it returns NULL and then falls back internally to not specified, which is not something that should happen during actual runtime.

Xano’s picture

Why?

Because during tests we want to isolate classes as much as we can. If we use getters like these, we should also add setters, or they won't be useful at all. There is no way to make those protected properties contain the required services now.

Wim Leers’s picture

So, #85 came back fully green again. That means a total of 80 test runs without a failure. Which implies #82 contains the solution. And I think we can address Xano's concern and restore Xano's code and still have it working, actually. (Local tests seem to indicate this at least.)

(While working on this, I was slowed down massively by #2216695: run-tests.sh broken on OS X ("Too many open files").)

Berdir’s picture

Hm, that is exactly the change that I made above to make it work. And yes, it works locally for all of use with and without that :)

My patch is just as capable as Xano's was before, if $this->entityManager exists, then it is used, the only difference is that I don't store a reference to it. There were no setters before, so I don't understand his comments as he didn't add them either ;)

Wim Leers’s picture

#95: I'm merely building upon what you did! I think Xano's concern is mainly that we'd always be returning things that live on the container — my tiny contribution here fixes that. I agree everything is just as capable as before. I'm merely cleaning up the temporary code you had in there, by respecting the original code pattern and storing a reference after all.
If you're saying you don't want those references to be stored, that's fine by me too — I don't care. If we do that though, then we must not ever pretend to store any references, and your code was still pretending that.

So that just leaves #89 to be addressed then.

The last submitted patch, 94: drupal_2134857-94.patch, failed testing.

Wim Leers’s picture

Random fail in Drupal\image\Tests\ImageAdminStylesTest, retesting.

Wim Leers’s picture

94: drupal_2134857-94.patch queued for re-testing.

The last submitted patch, 94: drupal_2134857-94.patch, failed testing.

webchick’s picture

Hm. Maybe not so random? Same fail again.

Berdir’s picture

Yes, this is one of the random fails this causes, see #80. It's in a different class this time but it's that error.

Again, that *exact* change that you reverted is what made it work, it's not cleanup. And as I said, it is exactly as flexible as the patch before, keeping a local reference does not change anything, if one is set, then it uses that. And I do agree with @Xano: that either a setter to set those properties for the unit test or mocking those methods (but then you don't need $this->entityManager at all) would be cleaner than going through the container, then we don't need those methods...

But again, that's how it was before :)

jessebeach’s picture

Priority: Major » Critical
Issue tags: -blocker +beta blocker

This blocks a beta blocker, so it is a beta blocker.

Berdir’s picture

See #81, this currently blocks that issue because it has unit tests that were written on top of this. It's more convenient to wait for this instead of rewriting them or changing them to a different type of test, but if we'd have to, we could, so I don't think it should be a hard dependency/this be a beta blocker.

damiankloip’s picture

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
@@ -0,0 +1,240 @@
+    $this->assertSame(spl_object_hash($this->entity), spl_object_hash($this->entity->set($name, $value)));
...
+    $this->assertSame(spl_object_hash($this->entity), spl_object_hash($this->entity->setStatus(FALSE)));
...
+    $this->assertSame(spl_object_hash($this->entity), spl_object_hash($this->entity->enable()));
...
+    $this->assertSame(spl_object_hash($this->entity), spl_object_hash($this->entity->disable()));
...
+    $this->assertSame(spl_object_hash($this->entity), spl_object_hash($this->entity->setSyncing(TRUE)));
...
+    $this->assertNotSame(spl_object_hash($this->entity), spl_object_hash($duplicate));
...
+    $this->assertSame(spl_object_hash($entity_b), spl_object_hash($list[0]));
...
+    $this->assertSame(spl_object_hash($entity_a), spl_object_hash($list[0]));

Sorry but I still don't think this looking better in the simpletest UI is a good reason to use this over PHPUnit handling the object comparison. We adopt a decent unit testing framework like this so we can utilise its functionality, no? :)

Wim Leers’s picture

FileSize
58.81 KB
7.46 KB
59.26 KB

Alright, so: #82 worked, #94 is a failed clean-up attempt (causes test failures), so this goes back to doing exactly the same as #82 but cleans it up.

Also addressed #105.

The last submitted patch, 106: drupal_2134857-106.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 106: drupal_2134857-106-toolbar.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.8 KB
59.25 KB

HEAD had changed in a single line.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

I think everything has been fixed and any concerns addressed. This is holding up some other stuff, so let's do this!

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
57.61 KB
827 bytes

Sorry, after #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray() got in, the ConfigEntityBaseTest needs to be updated to toArray(), minor change (missed that in the logs until now).

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #110. #111 was just for chasing HEAD.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, but this has some actual bugs (see first item) and still tons of bogus test methods.

I'm not saying that we need to implement all of them. but I'm against adding bogus test coverage so that it looks like we're covering those methods but we're not actually verying anything useful. Just dropping those that would be too much work would be fine with. I'd even be fine with just getting the necessary changes to the entity classes in as that would already unblock Wim's issue.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -544,7 +549,7 @@ public function language() {
           if (!isset($this->languages[$this->activeLangcode])) {
    -        $this->languages += language_list(Language::STATE_ALL);
    +        $this->languages += $this->languageList(Language::STATE_ALL);
           }
           $language = $this->languages[$this->activeLangcode];
    

    I think this method was removed?

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -23,6 +24,13 @@
       /**
    +   * The UUID generator.
    +   *
    +   * @var \Drupal\Component\Uuid\UuidInterface
    +   */
    +  protected $uuidGenerator;
    +
    
    @@ -44,6 +52,13 @@
     
       /**
    +   * The language manager.
    +   *
    +   * @var \Drupal\Core\Language\LanguageManagerInterface
    +   */
    +  protected $languageManager;
    
    @@ -61,6 +76,37 @@ public function __construct(array $values, $entity_type) {
    +   * Returns the UUID generator.
    +   *
    +   * @return \Drupal\Component\Uuid\UuidInterface
    +   */
    +  protected function uuidGenerator() {
    +    if (!$this->uuidGenerator) {
    +      $this->uuidGenerator = \Drupal::service('uuid');
    +    }
    +
    +    return $this->uuidGenerator;
    +  }
    

    $this->languageManager is no longer used and uuidGenerator should then follow the same pattern?

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -347,6 +392,7 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
       public static function postDelete(EntityStorageControllerInterface $storage_controller, array $entities) {
    +    /** @var self[] $entities */
         foreach ($entities as $entity) {
           $entity->onSaveOrDelete();
    

    This overlaps with #2157467: Fix type hinting for base entity interfaces, wouldn't be needed anymore then? As it's a non-functional change, leave it for that issue to change?

  4. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,240 @@
    +    $this->languageManager = $this->getMock('\Drupal\Core\Language\LanguageManagerInterface');
    +    $this->languageManager->expects($this->any())
    +      ->method('getLanguage')
    +      ->will($this->returnValue(NULL));
    

    I just moved the mock definition but as mentioned above, this is IMHO wrong.

    This is not something that should happen. There's some strange fallback code in Entity::language() but that's not something that should really happen and we cleaned up most of what needed that when moving to content entities, which don't support that hack. This should really return an actual language object and verify the passed arguments, which should be the same as the langcode passed to the entity.

  5. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,240 @@
    +   */
    +  public function testGetOriginalId() {
    +    $id = $this->randomName();
    +    $this->assertSame($this->entity, $this->entity->setOriginalId($id));
    +    $this->assertSame($id, $this->entity->getOriginalId());
    +  }
    

    When you look at how this works, then you can see that setOriginalId() isn't really called from the outside, the more interesting part is initializing an entity object with id and then changing the id and verifying that getOriginalId() is correct. Or is that not possible as it's an abstract class?

    This method is btw also moved to EntityInterface in #2218033: Move getOriginalId()/setOriginalId() to EntityInterface.

  6. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,240 @@
    +    $duplicate = $this->entity->createDuplicate();
    ...
    +    $this->assertNull($duplicate->uuid());
    

    This isn't useful, createDuplicate() must result in a new UUID that is different from the original one. So the original entity should already have a UUID and the duplicate a new one. the uuid service must be mocked accordingly, otherwise we're not providing useful test coverage here.

  7. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
    @@ -0,0 +1,240 @@
    +   * @covers ::getExportProperties
    +   */
    +  public function testToArray() {
    

    Stale comment.

  8. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +    $this->languageManager->expects($this->any())
    +      ->method('getLanguages')
    +      ->will($this->returnValue(array()));
    +    $this->languageManager->expects($this->any())
    +      ->method('getLanguage')
    +      ->will($this->returnValue(NULL));
    

    Same here, we should be working with an actual language object, otherwise we can't test that things are actually working.

  9. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +    $this->entityType->expects($this->at(0))
    +      ->method('hasKey')
    +      ->with('revision')
    +      ->will($this->returnValue(FALSE));
    +    $this->entityType->expects($this->at(1))
    +      ->method('hasKey')
    +      ->with('revision')
    +      ->will($this->returnValue(TRUE));
    +
    +    $this->assertFalse($this->entity->isNewRevision());
    +    $this->assertTrue($this->entity->isNewRevision());
    

    This might be easier to understand when the second expects is after the first call, to see that they belong together() ? note that this would affect the at(1). Alternatively, a comment to explain it?

  10. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +    // We override the value, but it does not affect this call.
    +    $this->assertTrue($this->entity->isDefaultRevision(FALSE));
    

    Comment is misleading imho, should simply say "Verify that the old value is returned." ?

  11. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +  /**
    +   * @covers ::getRevisionId
    +   */
    +  public function testGetRevisionId() {
    +    $this->assertNull($this->entity->getRevisionId());
    +  }
    

    That's kind of weird :) Note that #2182239: Improve ContentEntityBase::id() for better DX will allow to write a more useful test.

  12. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +  /**
    +   * @covers ::getName
    +   */
    +  public function testGetName() {
    +    $this->assertNull($this->entity->getName());
    +  }
    +
    

    Most of the complex data interface methods are empty dummy implementations and will go away in #2002138: Use an adapter for supporting typed data on ContentEntities, testing them is kind of pointless :)

  13. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -0,0 +1,333 @@
    +      ->method(__FUNCTION__)
    +      ->will($this->returnValue($callback_label));
    +    $this->entityType->expects($this->once())
    +      ->method('getLabelCallback')
    +      ->will($this->returnValue(array($callback_container, __FUNCTION__)));
    

    Why do we need to use something like __FUNCTION__ here? This is IMHO very misleading and seems to indicate that this test method is called? but it just uses the same string as this method name for the callback. What about giving it a name like "exampleLabelCallback"

  14. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,340 @@
    +    $this->languageManager->expects($this->any())
    +      ->method('getLanguage')
    +      ->will($this->returnValue(NULL));
    

    Same here :)

  15. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,340 @@
    +  /**
    +   * @covers ::id
    +   */
    +  public function testId() {
    +    // @todo How to test this?
    +    $this->assertNull($this->entity->id());
    +  }
    

    $values = array('id' => $this->randomName()

    Then you you have something that you can actually test? Same for testUuid() and testIsNew() which in the default implementation is based on having an ID.

  16. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,340 @@
    +   */
    +  public function testEnforceIsNew() {
    +    $this->assertSame($this->entity, $this->entity->enforceIsNew());
    +  }
    

    this should then be used in combination with actually having an id(). isNew() should then return FALSE, after calling enforceIsNew(), it should return TRUE. Testing those methods on their own is useless, the only gain is that we can claim that it's covered, but that's a lie :)

  17. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,340 @@
    +    $this->assertInstanceOf('\Drupal\Core\Entity\EntityTypeInterface', $this->entity->getEntityType());
    

    Can't you compare this with $this->entityType? should be the same object.

  18. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -0,0 +1,340 @@
    +  public function testLanguage() {
    +    $this->assertInstanceOf('\Drupal\Core\Language\Language', $this->entity->language());
    +  }
    

    Same here. $values should have langcode => 'en', or so, the language manager should have a corresponding with() as mentioned above and this should then verify that the correct language object is returned.

Berdir’s picture

FileSize
57.56 KB
12.92 KB

Implemented my review except:

12. Most of the methods are correctly tested for now, will removed them in the adapter issue.
13. Got this now. Crazy, but I guess can't be easier.

Now I'm ok with this ;)

Berdir’s picture

Status: Needs work » Needs review
Xano’s picture

FileSize
57.29 KB

Re-roll because of block.module.

Most of the methods are correctly tested for now, will removed them in the adapter issue.

Even if they aren't, we agreed to not cover typed data methods in this issue, so this should be good.

Got this now. Crazy, but I guess can't be easier.

Crazy, wicked, and it saved us from having to create a dummy class with a dummy method instead. Glad this is sorted out :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Alright! Berdir thinks this is good to go now, Xano just chased HEAD and improved a comment (the one for Berdir's 13th point).

I only changed a few LoC to get the patch good & green again, so it should be fine for me to re-RTBC this patch as per the RTBCs at #110, #114 and the earlier commit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1370edf and pushed to 8.x. Thanks!

  • Commit 1370edf on 8.x by alexpott:
    Issue #2134857 by Xano, Wim Leers, Berdir, damiankloip: PHPUnit test the...
xjm’s picture

Issue tags: -beta blocker

In the end this one wasn't actually a beta blocker.

Wim Leers’s picture

Issue tags: +beta target

But then it at least was a beta target.

Status: Fixed » Closed (fixed)

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