child of #1938068: Convert UnitTestBase to PHPUnit

There are 12 files with UnitTestBase classes:

core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/ValidNumberStepUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/ParseInfoFileUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/HtmlIdentifierUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/SizeUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/JsonUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/DiffArrayUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/AutocompleteTagsUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/TableSortExtenderUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/ValidUrlUnitTest.php

Maybe to many to do in one Issue?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

franskuipers’s picture

First patch for one test in system/lib/Drupal/system/Tests/Common directory: ColorTest.php

My remarks/questions:

  • Are the any guidelines when and how we are going to move the testfile to /core/tests directory?
  • TODO: is assertSame (phpunit) exectly the same as assertIdentical (simpletest)
  • PHPUnit seems to stop the test function after first Exception, the asserts after the Exception where never executed. Thats why I moved it to the last part of the testHexToRgb function. Needs some refactoring?
  • I include includes/bootstrap.inc and includes/unicode.inc in tests/bootstrap.inc, is this the right wat to go?
franskuipers’s picture

Status: Active » Needs review
Xano’s picture

I did a manual review. The patch looks good.

TODO: is assertSame (phpunit) exectly the same as assertIdentical (simpletest)

Yes, these two are identical.

I include includes/bootstrap.inc and includes/unicode.inc in tests/bootstrap.inc, is this the right wat to go?

What was your reason for doing this?

franskuipers’s picture

Reason for bootstrap.inc
PHP Fatal error: Call to undefined function Drupal\Core\Utility\drupal_strlen() in /home/frans/workspace/drupal8/core/lib/Drupal/Core/Utility/Color.php on line 31

Reason for unicode.inc:

There was 1 error:

1) Drupal\Tests\Core\Utility\Color\ColorTest\ColorTest::testHexToRgb
Use of undefined constant UNICODE_MULTIBYTE - assumed 'UNICODE_MULTIBYTE'
Xano’s picture

FileSize
7.86 KB

Re-roll using DrupalUnitTestBase so we do not need workarounds.

clemens.tolboom’s picture

It's hard to see the changes in both patches :(

@Xano you should use "git diff -M --cached 8.x" ... or cf #1116900-4: git format-patch displays file moves as whole verbose diffs somehow.

See #1935908: Convert Graph tests to phpunit where the patch has a move and is still nice diffed.

Should we really move back to DrupalTestBase?

Xano’s picture

Should we really move back to DrupalTestBase?

Yes, as this is a dependency injection problem. See New, separate, extended DrupalUnitTestBase for unit-testing functionality whose dependencies can't be injected.

Xano’s picture

Title: Convert system/lib/Drupal/system/Tests/Common UnitTest's to phpunit » Convert Drupal\Core\Utility\Color\ColorTest to DrupalUnitTestBase
FileSize
1.09 KB

@clemens.tolboom: Thanks for explaining git diff -M!

The attached patch simply converts the test case to DrupalUnitTestBase, and it includes @franskuipers' original code comment updates.

franskuipers’s picture

#1: drupal-ColorTest-1938184-1.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Needs work

This is not correct. If it becomes a DUBT test, then it can not be moved to core/tests. Currently, only PhpUnit tests can be there.

You can see that this test class is no longer found in the output, there is a ColorTest but that's from color.module.

Xano’s picture

clemens.tolboom’s picture

The mistake we made is to convert from class ColorTest extends UnitTestBase { to PHPUnit just because it was named UnitTestBase. That did not imply it is a standalone Unit test.

The effort @franskuipers did by adding bootstrap + unicode was breaking the unit test spirit.

The effort @Xano did was solving the includes but making it further down from unit test.

So the problem lies in fixing core/tests/bootstrap.php by adding the quirk like

if (!function_exists('drupal_strlen')) {
  define('UNICODE_MULTIBYTE', 1);

  function drupal_strlen($text) {
    global $multibyte;
    if ($multibyte == UNICODE_MULTIBYTE) {
      return mb_strlen($text);
    }
    else {
      // Do not count UTF-8 continuation bytes.
      return strlen(preg_replace("/[\x80-\xBF]/", '', $text));
    }
  }

}

Attached patch does the above but we need to discuss drupal_strlen for sure.

After applying the patch either of the below could work. I have no clue how to apply the reverse of
core$ git diff 8.x -M > ~/Downloads/drupal-ColorTest-1938184-12.patch
to make the file move :(

core$ vendor/bin/phpunit modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
core$ vendor/bin/phpunit tests/Drupal/Tests/Core/Utility/ColorTest.php
clemens.tolboom’s picture

Status: Needs work » Needs review
Berdir’s picture

The issue posted by Xano in #11 is actually the better approach, as it moves the functionality into a class that can also be used from a unit test.

clemens.tolboom’s picture

Argh .. I assumed I put the moved lines @franskuipers did in #1 back into place which is not the case. I hope @franskuipers can redo the patch from #1 with the spirit from #12 :)

clemens.tolboom’s picture

The patch from #11 has no PHPUnit base class.

abstract class DrupalUnitTestBase extends UnitTestBase {

abstract class UnitTestBase extends TestBase {

abstract class TestBase {

while

class UnitTestCase extends \PHPUnit_Framework_TestCase {

has. Or am I missing something vital now!?!

Status: Needs review » Needs work

The last submitted patch, drupal-ColorTest-1938184-12.patch, failed testing.

Xano’s picture

Status: Needs work » Postponed

The current test uses UnitTestBase, which is the most 'unit' Simpletest test we have, so making the test extend DrupalUnitTestBase was a bad decision. The next step is to make it a PHPUnit test, which is impossible, because \Drupal\Core\Utility\Color depends on code that cannot be lazy-loaded yet (see #1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode).

Postponing until that other issue is fixed. Then we can continue porting this test to PHPUnit.

franskuipers’s picture

Issue tags: -SprintWeekend2013 +PHPUnit
diff --git a/core/tests/bootstrap.php b/core/tests/bootstrap.php
index 6c947df..11ff65d 100644
--- a/core/tests/bootstrap.php
+++ b/core/tests/bootstrap.phpundefined
@@ -10,4 +10,5 @@
   $loader->add('Drupal\\' . $module, __DIR__ . "/../modules/" . $module . "/lib");
 }
 // Look into removing this later.
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+require_once __DIR__ . '/../includes/bootstrap.inc';
+require_once __DIR__ . '/../includes/unicode.inc';

Xanos diversion is about this code in #1. The lines

// Look into removing this later.
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);

where brought in with the core patch in #1901670: Start using PHPUnit for unit tests.
I changed this by loading the includes, because I needed drupal_strlen() (in unicode.inc) and 'REQUEST_TIME' (in bootstrap.inc)

My reasoning for this temporary hack are:

  1. If it already done once already in core, why not again?
  2. If we allow this we could carry on with #1938068: Convert UnitTestBase to PHPUnit and remove the hack(s) in an followup
  3. I feel the includes are better the the solution in #12

There are open questions from #1:

  • Are the any guidelines when and how we are going to move the testfile to /core/tests directory?
  • PHPUnit seems to stop the test function after first Exception, the asserts after the Exception where never executed. That is why I moved it to the last part of the testHexToRgb function. I did already some refactoring.

I would like to help/move on with #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest but I am a bit lost now :)

Xano’s picture

DrupalUnitTestBase is the official workaround for a case like this now. It prevents us from having to include everything explicitly, and from having to keep track of test-specific workarounds manually. You're absolutely right that it's not as performant as your solution, but it is much easier to maintain in the long run. Once the entire core codebase is either loaded by default or can be lazy-loaded, we can convert all DrupalUnitTestBase test cases to PHPUnit.

franskuipers’s picture

... deleted ....

ParisLiakos’s picture

Title: Convert Drupal\Core\Utility\Color\ColorTest to DrupalUnitTestBase » Convert Drupal\Core\Utility\Color\ColorTest to PHPUnit
ParisLiakos’s picture

Status: Postponed » Needs work

as per #18

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
8.84 KB

much better

clemens.tolboom’s picture

Status: Needs review » Needs work

Can you re-submit the patch with git -M as mentioned in #1938068: Convert UnitTestBase to PHPUnit

ParisLiakos’s picture

Status: Needs work » Needs review

i dont think it is that big patch, and the moved file is very different, so it helps to check whether the test was converted correctly
(usually git does this automatically for me, but this time the differences between files are many, there where no dataproviders before)

dawehner’s picture

I agree that git -M should be used, but if data providers are used (which seems way better in general) this kind of diff does may help less.

The tests are looking pretty great.

+++ /dev/nullundefined
@@ -1,100 +0,0 @@
-      catch (\InvalidArgumentException $e) {
-        $this->pass('Color::hexToRgb(' . var_export($test, TRUE) . ') threw an exception.');
-      }

Data providers will probably take care about showing failing data?

Just a couple of nitpicks.

+++ b/core/tests/Drupal/Tests/Core/Utility/ColorTest.phpundefined
@@ -0,0 +1,120 @@
+ * Contains Drupal\Tests\Core\Utility\ColorTest.

Needs a "\" as prefix.

+++ b/core/tests/Drupal/Tests/Core/Utility/ColorTest.phpundefined
@@ -0,0 +1,120 @@
+   * Tests Color::hexToRgb().
...
+  public function testHexToRgb($value, $expected, $invalid = FALSE) {
...
+   * Tests Color::rgbToHex().
...
+  public function testRgbToHex($value, $expected) {

Should this also have documentation?

+++ b/core/tests/Drupal/Tests/Core/Utility/ColorTest.phpundefined
@@ -0,0 +1,120 @@
+   *   An array containing:
...
+   *   An array containing:

Isn't it an array of arrays?

It seems to the case that "Color::rgbToHex" could need description how it works but this is for sure out of scope.

ParisLiakos’s picture

thanks for review!

Data providers will probably take care about showing failing data?

Yes, this how it looks in phpunit when something throws an unexpected exception:

12) Drupal\Tests\Core\Utility\ColorTest::testHexToRgb with data set #29 (9.2233720368548E+18, '')
InvalidArgumentException: '9.2233720368548E+18' is not a valid hex value.

fix rest points

about git -M..there is almost no similarity with those files, namespaces changed, functions changed, indentation changer

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7846ad6 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture