Improve/Refactor TestBase Through Expanded Unit Testing

Awesome work!

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -231,6 +253,23 @@ protected function checkRequirements() {
   +   * @see assert()
   

   I think this should be better like self::assert or static::assert

2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -397,7 +433,7 @@ protected function getAssertionCall() {
   -    return _drupal_get_last_caller($backtrace);
   +    return \_drupal_get_last_caller($backtrace);
   

   Are you sure that this change is needed?

3: 2144669_3_testbase_unit_test.patch queued for re-testing.

Rerolling of the patch is done.. :)

9: 2144669_9_testbase_unit_test.patch queued for re-testing.

Patch has been removed. I have removed the Php syntax issue.

I did a review and I have some problems with the patch:

1. Can we rename the randomStringValidateProvider to providerRandomStringValidate.
2. Can the documentation for the provider return values move from the test to the provider.
3. Can the expected value be the first parameter. As we do with other tests.
4. For the function testRandomStringValidate: can we remove the documentation line "Tests the random strings validation rules". We have @covers for that. The same for "@see \Drupal\simpletest\TestBase::randomStringValidate()".
5. For the function: testRandomString. Remove the documentation lines: "Tests that the random string contains a non-alphanumeric character." and "@see \Drupal\simpletest\TestBase::randomString()." We have @covers for that.

6. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
   @@ -80,4 +81,387 @@ public function testRandomString() {
   +   * Test checkRequirements().
   

   Use @covers. Also for other functions

7. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
   @@ -80,4 +81,387 @@ public function testRandomString() {
   +  public function providerTestAssert() {
   

   This is the provider for the function assert(). So shouldn't we call it providerAssert(). Also for other functions.

8. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
   @@ -80,4 +81,387 @@ public function testRandomString() {
   +  /**
   +   * Data provider for assertTrue().
   +   */
   +  public function providerTestAssertTrue() {
   

   Document the return array. Also for other functions.

9. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
   @@ -80,4 +81,387 @@ public function testRandomString() {
   +  public function testError($group) {
   +    $test_base = $this->getTestBaseForAssertionTests();
   +    $this->assertEquals(
   +        FALSE,
   +        $this->invokeProtectedMethod($test_base, 'error', array('msg', $group))
   +    );
   +  }
   

   The only thing that is being tested is the function returns a value of FALSE.

10. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
    @@ -80,4 +81,387 @@ public function testRandomString() {
    +  public function testCopyConfig() {
    +    $mock_storage_source = $this->getMock('Drupal\Core\Config\StorageInterface');
    +    $mock_storage_source->expects($this->any())
    +      ->method('deleteAll');
    +    $mock_storage_source->expects($this->any())
    +      ->method('listAll')
    +      ->will($this->returnValue(array('name')));
    +    $mock_storage_target = clone $mock_storage_source;
    +    $mock_storage_source->expects($this->any())
    +      ->method('read')
    +      ->will($this->returnValue(array('thing')));
    +    $mock_storage_target->expects($this->any())
    +      ->method('write');
    +
    +    $test_base = $this->getTestBaseForAssertionTests();
    +    $this->assertNull($test_base->copyConfig($mock_storage_source, $mock_storage_target));
    +  }
    

    Can we make two distinct mocks. The cloning results in mocks that have functions that they do not use. It is also a bit confusing for me.

If I read the best practices from #2057905: [policy, no patch] Discuss the standards for phpunit based tests, I get the idea that it is testAssert() and providerAssert(). I know that we have done a lot of providerTestAssert() constructions, so I thought I should mention it. But if I am wrong please let me know.

Thank you for updating your patch. I have reviewed it again.

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +17,479 @@
+  public function getTestBaseForAssertionTests($test_id = 'test_id') {
...
+  public function invokeProtectedMethod($object, $method_name, array $arguments) {

Can we move these functions to the top of the class. They are helper functions.

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +17,479 @@
+  /**
+   * Data provider for testAssertEqual().
+   */
+  public function providerAssertIdentical() {

Data provider for testAssertIdentical

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +17,479 @@
+  public function testError($status, $group) {
+    // Mock up a TestBase object.
+    $mock_test_base = $this->getMockBuilder('Drupal\simpletest\TestBase')
+      ->setMethods(array('assert'))
+      ->getMockForAbstractClass();
+
+    // Set expectations for assert().
+    $mock_test_base->expects($this->once())
+      ->method('assert')
+      // The first argument to assert() should be the expected $status.
+      ->with($status)
+      // Arbitrary return value.
+      ->willReturn("$status:$group");
+
+    // Invoke error().
+    $this->assertEquals(
+        "$status:$group",
+        $this->invokeProtectedMethod($mock_test_base, 'error', array('msg', $group))
+    );
+  }

Can you explain this to me. The function error returns the assert function. And it is the assert function that you mock. So now you are testing if willReturn("$status:$group") is equal to "$status:$group". Can we concentrate on how the group is 'User notice' is treated differently.

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +17,479 @@
+  public function testRandomObject() {

Can we add a provider and test for null, 1, 2, 4 and 7. Also for randomObject and randomString. Sorry, should have mentioned it in the previous review. :-(

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +17,479 @@
+    $mock_target_storage->expects($this->never())
+      ->method('listAll');
...
+    $mock_target_storage->expects($this->never())
+      ->method('read');
...
+    $mock_source_storage->expects($this->never())
+      ->method('deleteAll');
...
+    $mock_source_storage->expects($this->never())
+      ->method('write');

Just curious: why add these?

Thank you for explaining the testError() function to me. I was wrong and somehow I looked cross eyed when I reviewed it.
For the testCopyConfig(): your explanation is also valid. I would not have added them. But I can also see your point of view.
It all looks good to me.
I have only one question and one nitpick left:

/**
 * @file
 * Contains \Drupal\simpletest\TestBaseTest.
 */

Should it not be: Contains \Drupal\Tests\simpletest\Unit\TestBaseTest.

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +16,501 @@
+    $test_id = 'test_id';
+    $test_base = $this->getTestBaseForAssertionTests($test_id);

Can we rewrite this to: $test_base = $this->getTestBaseForAssertionTests();

It all looks good to me.
All comments are in order.
The patch fixed the problem described in the issue summary.
The patch gets a RTBC from me.

Back to RTBC

Back to RTBC

1. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -1231,7 +1247,6 @@ private function restoreEnvironment() {
      public function errorHandler($severity, $message, $file = NULL, $line = NULL) {
        if ($severity & error_reporting()) {
   -      require_once DRUPAL_ROOT . '/core/includes/errors.inc';
   
   @@ -1266,7 +1281,6 @@ public function errorHandler($severity, $message, $file = NULL, $line = NULL) {
      protected function exceptionHandler($exception) {
   -    require_once DRUPAL_ROOT . '/core/includes/errors.inc';
        $backtrace = $exception->getTrace();
   

   This is fine now that we don't have a unit test class that extends TestBase

2. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
   @@ -16,68 +16,504 @@
   +  /**
   +   * Provides data for testCopyConfig().
   +   *
   +   * @return array
   +   *   - A keyed array which maps a key to a nested array.
   +   */
   +  public function providerCopyConfig() {
   +    return [
   +      [[]],
   +      [['key' => ['value']]],
   +      [['key' => ['value'], 'another_key' => ['another_value']]],
   +    ];
   +  }
   +
   +  /**
   +   * @covers ::copyConfig
   +   * @dataProvider providerCopyConfig
   +   */
   +  public function testCopyConfig($source_map) {
   +    // Mock the target storage.
   +    $mock_target_storage = $this->getMockBuilder('Drupal\Core\Config\StorageInterface')
   +      ->setMethods(array('deleteAll', 'listAll', 'write', 'read'))
   +      ->getMockForAbstractClass();
   +    // Set method expectations. We always want deleteAll(), never want
   +    // listAll(), never want read(), and expect the count of source storage for
   +    // write().
   +    $mock_target_storage->expects($this->once())
   +      ->method('deleteAll');
   +    $mock_target_storage->expects($this->never())
   +      ->method('listAll');
   +    $mock_target_storage->expects($this->exactly(count($source_map)))
   +      ->method('write');
   +    $mock_target_storage->expects($this->never())
   +      ->method('read');
   +
   +    // Mock source storage.
   +    $mock_source_storage = $this->getMockBuilder('Drupal\Core\Config\StorageInterface')
   +      ->setMethods(array('deleteAll', 'listAll', 'write', 'read'))
   +      ->getMockForAbstractClass();
   +    // Set method expectations. These are backwards from $mock_target_storage.
   +    // We never want deleteAll(), always want listAll(), never want write(),
   +    // and expect the count of source storage for read().
   +    $mock_source_storage->expects($this->never())
   +      ->method('deleteAll');
   +    $mock_source_storage->expects($this->once())
   +      ->method('listAll')
   +      // listAll() should return the keys of $source_map.
   +      ->willReturnCallback(
   +        function () use ($source_map) {
   +          return array_keys($source_map);
   +        }
   +      );
   +    $mock_source_storage->expects($this->never())
   +      ->method('write');
   +    $mock_source_storage->expects($this->exactly(count($source_map)))
   +      ->method('read')
   +      // read() should map a key to a value from $source_map.
   +      ->willReturnCallback(
   +        function ($key) use ($source_map) {
   +          return $source_map[$key];
   +        }
   +      );
    
   -    // Ensure that we can generate random strings with a length of 1.
   -    $string = $this->stub->randomString(1);
   -    $this->assertEquals(1, strlen($string));
   +    // Exercise copyConfig(). Assert NULL because copyConfig() is void and only
   +    // has side-effects.
   +    $test_base = $this->getTestBaseForAssertionTests('test_id');
   +    $this->assertNull($test_base->copyConfig($mock_source_storage, $mock_target_storage));
      }
   

   Let's not add this test - it just ties us to the implementation making copy_config hard to change for no benefit.

   In fact this is a general criticism of all of the tests - we are tying down internal implementation details making changing harder than necessary.

@alexpott wants testCoptConfig() and providerCopyConfig() removed from the patch and that is done. So back to RTBC.

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +16,435 @@
+  /**
+   * Data provider for testAssertEqual().
+   */
+  public function providerAssertEqual() {
+    return array(
+      array(TRUE, 0, 0),
+      array(FALSE, 'foof', 'yay'),
+    );
+  }
...
+  /**
+   * Data provider for testAssertIdentical and testAssertNotIdentical().
+   */
+  public function providerAssertIdentical() {
+    return array(
+      array(TRUE, 0, 0),
+      array(FALSE, 'foof', 'yay'),
+    );
+  }

Let's add to these so that the difference between assertEqual and assertIdentical is exposed and tested.

It all good good to me with one exception:

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -16,68 +16,459 @@
+   * @dataProvider providerAssertIdentical

Should this test not use providerAssertIdentical as its dataprovider?

@Mile23: My typo mistake! No, I mean that it should be: "@dataprovider providerEqualityAssertions".

@Mile23: Yes, that is what I had in mind.

It all looks good to me.
All documentation is in order.
This is about tests so as a beta change it is allowed.
It get a RTBC from me.

+++ b/core/modules/simpletest/src/TestBase.php
@@ -1334,7 +1350,6 @@ private function restoreEnvironment() {
-      require_once DRUPAL_ROOT . '/core/includes/errors.inc';

@@ -1369,7 +1384,6 @@ public function errorHandler($severity, $message, $file = NULL, $line = NULL) {
-    require_once DRUPAL_ROOT . '/core/includes/errors.inc';

Are you sure about this? Like do you know why this is here?

Lol I answered this myself in #36 :)

Committed a954b05 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

PHP Notice:  Undefined variable: x in /Volumes/devdisk/dev/drupal/core/modules/simpletest/tests/src/Unit/TestBaseTest.php on line 321

After committing this patch.

Mile23 agrees with my change so back to RTBC.

Thanks for your review Mile23.

Committed 9305a47 and pushed to 8.0.x. Thanks!