Convert most entity tests to DrupalUnitTestBase

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiTest.php
@@ -46,10 +37,10 @@ public function testCRUD() {
-   * @param \Drupal\user\Plugin\Core\Entity\User $user1

@param docs always have to be fully namespaced.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityCrudHookTest.php
@@ -190,6 +200,8 @@ public function testCommentHooks() {
+    $this->installSchema('file', 'file_managed');

I don't like setup tasks in the actual test method. On the other hand, you would have to create separate classes for each then, which is also tedious :(

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitBaseTest.php
@@ -0,0 +1,47 @@
+/**
+ * @file
+ * Definition of Drupal\system\Tests\Entity\EntityApiTest.

Should be "Contains ...", see http://drupal.org/node/1354#files

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitBaseTest.php
@@ -0,0 +1,47 @@
+   * @return Drupal\user\Plugin\Core\Entity\User

missing "\"

Otherwise this looks like a good idea.

This looks good from my perspective, beside of the ugly hook test case already mentioned before.

This halves the time the Entity API group of tests takes to tun... great work!

With patch

Drupal test run
---------------

Tests to be run:
 - Entity display configuration entities (Drupal\entity\Tests\EntityDisplayTest)
 - Entity properties (Drupal\field\Tests\EntityPropertiesTest)
 - Entity access (Drupal\system\Tests\Entity\EntityAccessTest)
 - Entity info (Drupal\system\Tests\Entity\EntityApiInfoTest)
 - Entity CRUD (Drupal\system\Tests\Entity\EntityApiTest)
 - Entity CRUD hooks (Drupal\system\Tests\Entity\EntityCrudHookTest)
 - Entity Field API (Drupal\system\Tests\Entity\EntityFieldTest)
 - Entity form (Drupal\system\Tests\Entity\EntityFormTest)
 - Entity Query relationship (Drupal\system\Tests\Entity\EntityQueryRelationshipTest)
 - Entity Query (Drupal\system\Tests\Entity\EntityQueryTest)
 - Entity revisions (Drupal\system\Tests\Entity\EntityRevisionsTest)
 - Entity translation form (Drupal\system\Tests\Entity\EntityTranslationFormTest)
 - Entity Translation (Drupal\system\Tests\Entity\EntityTranslationTest)
 - Entity UUIDs (Drupal\system\Tests\Entity\EntityUUIDTest)

Test run started:
 Tuesday, January 22, 2013 - 12:25

Test summary
------------

Entity display configuration entities 29 passes, 0 fails, and 0 exceptions
Entity properties 4 passes, 0 fails, and 0 exceptions
Entity access 34 passes, 0 fails, and 0 exceptions
Entity info 6 passes, 0 fails, and 0 exceptions
Entity CRUD 24 passes, 0 fails, and 0 exceptions
Entity CRUD hooks 149 passes, 0 fails, and 0 exceptions
Entity Field API 640 passes, 0 fails, and 0 exceptions
Entity form 77 passes, 0 fails, and 0 exceptions
Entity Query relationship 17 passes, 0 fails, and 0 exceptions
Entity Query 57 passes, 0 fails, and 0 exceptions
Entity revisions 53 passes, 0 fails, and 0 exceptions
Entity translation form 36 passes, 0 fails, and 0 exceptions
Entity Translation 136 passes, 0 fail, and 0 exceptions
Entity UUIDs 76 passes, 0 fails, and 0 exceptions

Test run duration: 2 min 1 sec

without patch

Drupal test run
---------------

Tests to be run:
 - Entity display configuration entities (Drupal\entity\Tests\EntityDisplayTest)
 - Entity properties (Drupal\field\Tests\EntityPropertiesTest)
 - Entity access (Drupal\system\Tests\Entity\EntityAccessTest)
 - Entity info (Drupal\system\Tests\Entity\EntityApiInfoTest)
 - Entity CRUD (Drupal\system\Tests\Entity\EntityApiTest)
 - Entity CRUD hooks (Drupal\system\Tests\Entity\EntityCrudHookTest)
 - Entity Field API (Drupal\system\Tests\Entity\EntityFieldTest)
 - Entity form (Drupal\system\Tests\Entity\EntityFormTest)
 - Entity Query relationship (Drupal\system\Tests\Entity\EntityQueryRelationshipTest)
 - Entity Query (Drupal\system\Tests\Entity\EntityQueryTest)
 - Entity revisions (Drupal\system\Tests\Entity\EntityRevisionsTest)
 - Entity translation form (Drupal\system\Tests\Entity\EntityTranslationFormTest)
 - Entity Translation (Drupal\system\Tests\Entity\EntityTranslationTest)
 - Entity UUIDs (Drupal\system\Tests\Entity\EntityUUIDTest)

Test run started:
 Tuesday, January 22, 2013 - 10:42

Test summary
------------

Entity display configuration entities 29 passes, 0 fails, and 0 exceptions
Entity properties 4 passes, 0 fails, and 0 exceptions
Entity access 34 passes, 0 fails, and 0 exceptions
Entity info 6 passes, 0 fails, and 0 exceptions
Entity CRUD 26 passes, 0 fails, and 0 exceptions
Entity CRUD hooks 157 passes, 0 fails, and 0 exceptions
Entity Field API 682 passes, 0 fails, and 0 exceptions
Entity form 77 passes, 0 fails, and 0 exceptions
Entity Query relationship 20 passes, 0 fails, and 0 exceptions
Entity Query 62 passes, 0 fails, and 0 exceptions
Entity revisions 53 passes, 0 fails, and 0 exceptions
Entity translation form 36 passes, 0 fails, and 0 exceptions
Entity Translation 138 passes, 0 fails, and 0 exceptions
Entity UUIDs 77 passes, 0 fails, and 0 exceptions

Test run duration: 4 min 8 sec

EDIT: The entity translation test had one of it's random failures... replaced "with patch" test data.

Thanks for converting these!

However, since there are new calls to enableModules() and config_install_default_config() in this patch, let's wait for #1891516: Remove $install parameter from DrupalUnitTestBase::enableModules(), encourage individual schema tables, please.

Well, the other issue does allow you to specify an array of tables from a module, so the hunk shown in #15 could be one method call after that other issue goes in.

Rerolled.

+++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
@@ -251,3 +251,20 @@ function entity_test_entity_test_insert($entity) {
\ No newline at end of file

:(

Some small points which are not worth to be reviewed.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitBaseTest.php
@@ -0,0 +1,58 @@
+/**
+ * Tests the basic Entity API.
+ */
+class EntityUnitBaseTest extends DrupalUnitTestBase {

Documentation does not say it's just a base-test case, what it is - right? So should it be abstract also? Else improvements look just great!

Good point.

Some tiny nitpicking.

Great, I agree this is RTBC!

Faster tests++ :)

Committed and pushed to 8.x. Thanks!

Thanks all! This is a great improvement :)

Sorry for my late review, but I've discovered some issues:

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitBaseTest.php
@@ -0,0 +1,66 @@
+abstract class EntityUnitBaseTest extends DrupalUnitTestBase {

s/EntityUnitBaseTest/EntityUnitTestBase/

The *Test suffix normally denotes a final test class. The *Base suffix denotes an abstract/base class.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitBaseTest.php
@@ -0,0 +1,66 @@
+  protected function createUser($values = array(), $permissions = array()) {

Really not happy about the "dual-purpose" here.

For unit tests, there should really be a cleanly separated createUserRole() method.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitBaseTest.php
@@ -0,0 +1,66 @@
+    $account = entity_create('user', $values + array(
...
+      'status' => 1,
+    ));
+    $account->enforceIsNew();

1) $status defaults to 1 already, so I don't really see why we need to explicitly set it?

2) enforceIsNew() on a newly created entity object looks superfluous to me?

Much better, thanks! :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryAggregateTest.php
@@ -12,7 +12,7 @@
- * @todo Use EntityUnitBaseTest provided by http://drupal.org/node/1893108.
+ * @todo Use EntityUnitTestBase provided by http://drupal.org/node/1893108.

Eh? This @todo refers to this issue here? ;)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitTestBase.php
@@ -0,0 +1,73 @@
+    $account->enforceIsNew();

I did not really understand your reply. I understand the concern about uid 1, but fail to see how and why this method call would work around that situation in any way. I fear that some non-trivial magic is going on with this additional method call.

We should make sure to document that magic right above this call.

Ah-ha! :)

So you're actually not preventing uid 1 from getting created (by default). Instead, you require all callers to pass a random 'uid' in $values, which in turn gets unset and enforced to be new? I guess that could use some additional clarification in the phpDoc?

That said... I still do not understand why this trick would skip over uid 1 from being created (if there is none yet). enforceIsNew() only unsets the entity ID, no? In turn, the new ID would still be 1 if there is none yet...?

So... actually less ah-ha than I thought :P :D

Thanks! Hm... Perhaps I slowly get it now... :)

So let me try:

 * @param yada $values
 *   Yada.
 *   To perform access checks and prevent the new user from being uid 1
 *   (which is exempt from all user access conditions), pass a value
 *    other than 1 as 'uid' in $values.

...

  // Force the entity to be new, so that callers are able to specify
  // a 'uid' in $values to skip over uid 1. If the specified uid exists
  // already, UserStorageController will automatically choose and insert
  // the next available ID.
  $account->enforceIsNew();

Right, or still wrong? Thanks for enlightening me/us! :)

Thank you!

I think my understanding level hovers around ~90% by now (still some gaps, but I'm also super busy with $dayjob right now), so I think we can move forward with this. :)

Perhaps @webchick wants to commit this ;)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -62,7 +62,10 @@ function assertEntityAccess($ops, AccessibleInterface $object, User $account = N
-    $user = $this->createUser(array('uid' => 2), array('view test entity'));
+    $role = $this->createUserRole(array('view test entity'));
+    $user = $this->createUser(array('uid' => 2));
+    $user->roles[$role->id()] = $role->id();
+    $user->save();

Going from 1 line of code to 4 lines of code makes me :( but I guess I can see the logic in granular methods.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitTestBase.phpundefined
@@ -0,0 +1,79 @@
+   * @param array $values
+   *   (optional) The values used to create the entity. To perform access checks
+   *   and prevent the new user from becoming uid 1 (which exempts from all user
+   *   access conditions), pass a unique value other than 1 as 'uid' in $values.
...
+  protected function createUser($values = array()) {

Whenever you need a book and a half to explain something, it's usually a sign that you should probably change the implementation. ;)

So I can understand totally why a user with the UID of 1 coming out of that function would totally screw any permission-based tests in an odd way. However, I would also posit that creating a non-UID1 user is the 99% case.

Therefore, how I would do this instead is something like:

if (!isset($values['uid'])) {
  // Always add one so you never end up with uid 1 unless you meant to.
  $values['uid'] = $database->getNextId() + 1; // I'm making up this function, but something like that.
}

So that you could still create a uid1 if you really wanted to for some reason, but you wouldn't need to worry about this detail at the "developer calling the function" level.

In other words, this:

$web_user = $this->createUser();

...is way cleaner than this:

$web_user = $this->createUser(array('uid' => 2)); // oops, no wait, 3, oops. I meant 4.. or 5 maybe?

Wondering if we should split that part out into its own patch and just do the rename here, which is totally uncontroversial and also blocks a few other things.

Thanks for the review, @webchick!

Note that the double-saving of $user is a bug... The roles can be specified in the values being passed to createUser() already. Well spotted.

Regarding uid 1: It's actually only access/permission-related tests that may want to skip over uid 1. The vast majority of tests should actually use uid 1 or uid X, because user permissions should be irrelevant.

Closely related anecdote: In D8, tests are finally able to log in with $this->root_user (uid 1), which allows plenty of web tests to skip over the creation of test users and just log in. Since the facility is new and isn't widely known yet, we still have plenty of tests that still create custom "admin" users, even though that's not strictly required anymore. Leveraging the root user not only speeds up tests but also simplifies them.

Due to that, I do not think that we generally want to skip over uid 1. Instead, I imagine that there can be many tests that just need a user record.

If we wanted to keep this super simple, I could foresee the following two paths:

A) Make the test setup automatically create a $this->root_user, similar to a web test, and case closed (though with negative test performance impact).

B) Make each test that needs to create non-uid 1 users simply create $this->root_user first. Provide a helper method for that, but otherwise, as usual, in a unit test, you're on your own, case closed.

Both options would be a lot more clear than the currently proposed "magic." ;) I personally don't have strong preference between both, but I think I'd lean more towards B), partially because I think that a unit test shouldn't do anything automatically.

couldnt find anything to complain about and since most webchick's concerns are in another issue, i am rtbc'ing this

Perfect, thanks!

Committed and pushed to 8.x.