Switching the entity storage class should not be considered a definition update that requires schema changes

Yes, that is the relevant part of my patch from #2336895: Allow entity type and field storage definition objects to be compared for definition equality :)

What we need now is a unit test. Looks like this method has no unit test coverage yet.

Some feedback on the tests structure, what they are testing looks great to me as well.

1. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -1163,6 +1163,102 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition,
   +    $updated_entity_type_definition = $this->getMockBuilder('\Drupal\Core\Entity\ContentEntityTypeInterface')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $original_entity_type_definition = $this->getMockBuilder('\Drupal\Core\Entity\ContentEntityTypeInterface')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   

   No need for the constructor stuff on an Interface, just use getMock() ?

2. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -1163,6 +1163,102 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition,
   +    // Case 1: Class names change, should not result in required schema change.
   

   Multiple scenarios are usually either tested with data providers (would be very hard I guess here) or multiple methods. So this one would be come testRequiresEntityStorageSchemaChangesStorageClassChange() or so.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -1163,6 +1163,102 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition,
   +      ->willReturn('Drupal\Core\Entity\Sql\SqlContentEntityStorage');
   ...
   +      ->willReturn('\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema');
   

   it is correct that any class works, but reads strange. There's a null implementation of the storage, you could use that, for example..

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -1163,6 +1163,92 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition,
+      ->getMock();
+    $original_entity_type_definition = $this->getMockBuilder('\Drupal\Core\Entity\ContentEntityTypeInterface')
+      ->getMock();

Nitpick: Sorry, what I meant is just $this->getMock('\Drupal\Core\Entity\ContentEntityTypeInterface'); We don't need to get the mockbuilder here.

Did not review the rest yet.

Updated issue summary, better now?

The reason I didn't comment on the test yet is that I've had bad experiences with mock objects that are set up in a data provider, because they are all created before a test runs and then kinda-survive multiple test executions, but there is code that runs at the end of a test method on all mocks, for example to check that methods were called as often as expected.

The test seems to work, but I suspect it will no longer work as expected if you change those $this->any() to $this->once(), which would be more correct. The only alternative I can offer is to only set up some sort of instrumentalisation of the mock expections and only create the mocks in the test method. Or have multiple test methods with helper methods to pass the mocks into. But I'm think I'm OK with moving forward here if it works, but that is IMHO something that @alexpott/@catch should know about and make a final decision if they're OK with that.

Reviewed this again. Agreed that this is probably OK like that (Although I'm not 100% sure the never() is working as expected, you call it on the clone and pass to multiple methods, but I'm not sure how that will actually be verified)

Anyway, +1 RTBC on the test coverage, actual change is what I wrote, although you came up with it separately I think. Leaving it to @plach to set RTBC.