child of #1938068: [Meta] 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?

Files: 
CommentFileSizeAuthor
#28 drupal-color_test_phpunit-1938184-28.patch9.21 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,771 pass(es).
[ View ]
#28 interdiff.txt1.8 KBParisLiakos
#24 drupal-color_test_phpunit-1938184-23.patch8.84 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,676 pass(es).
[ View ]
#12 drupal-ColorTest-1938184-12.patch4.53 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 52,943 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#8 drupal_1938184_03.patch1.09 KBXano
PASSED: [[SimpleTest]]: [MySQL] 52,513 pass(es).
[ View ]
#5 drupal_1938184_02.patch7.86 KBXano
PASSED: [[SimpleTest]]: [MySQL] 52,744 pass(es).
[ View ]
#1 drupal-ColorTest-1938184-1.patch4.44 KBfranskuipers
FAILED: [[SimpleTest]]: [MySQL] 52,785 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Issue tags:+#SprintWeekend
StatusFileSize
new4.44 KB
FAILED: [[SimpleTest]]: [MySQL] 52,785 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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?

Status:Active» Needs review

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?

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'

StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 52,744 pass(es).
[ View ]

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

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?

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.

Title:Convert system/lib/Drupal/system/Tests/Common UnitTest's to phpunitConvert Drupal\Core\Utility\Color\ColorTest to DrupalUnitTestBase
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 52,513 pass(es).
[ View ]

@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.

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

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.

StatusFileSize
new4.53 KB
FAILED: [[SimpleTest]]: [MySQL] 52,943 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

<?php
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

Status:Needs work» Needs review

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.

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 :)

The patch from #11 has no PHPUnit base class.

<?php
abstract class DrupalUnitTestBase extends UnitTestBase {
abstract class
UnitTestBase extends TestBase {
abstract class
TestBase {
?>

while
<?php
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.

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.

Issue tags:-#SprintWeekend+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: [Meta] 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] Replace the testing framework with PHPUnit and possibly rewrite Simpletest but I am a bit lost now :)

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.

... deleted ....

Title:Convert Drupal\Core\Utility\Color\ColorTest to DrupalUnitTestBaseConvert Drupal\Core\Utility\Color\ColorTest to PHPUnit

Status:Postponed» Needs work

as per #18

Status:Needs work» Needs review
StatusFileSize
new8.84 KB
PASSED: [[SimpleTest]]: [MySQL] 55,676 pass(es).
[ View ]

much better

Status:Needs review» Needs work

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

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)

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.

StatusFileSize
new1.8 KB
new9.21 KB
PASSED: [[SimpleTest]]: [MySQL] 56,771 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Thank you!

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.

Issue summary:View changes

Updated issue summary.