Task to convert modules/system/lib/Drupal/system/Tests/Common to phpunit where possible.

Common/JsonUnitTest.php
Common/ValidUrlUnitTest.php
Common/ParseInfoFileUnitTest.php
Common/HtmlIdentifierUnitTest.php
Common/AutocompleteTagsUnitTest.php
Common/AttributesUnitTest.php
Common/SizeUnitTest.php
Common/TableSortExtenderUnitTest.php
Common/CascadingStylesheetsUnitTest.php
Common/DiffArrayUnitTest.php
Common/ValidNumberStepUnitTest.php
Common/ColorTest.php

See #1938068: Convert UnitTestBase to PHPUnit.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
35.8 KB

This patch is quite involved compared to some of the others. A lot of these tests required constants and functions defined in includes/bootstrap.inc, so that got added to tests/bootstrap.php--not sure if there's currently a way around that.

jhedstrom’s picture

I should have mentioned, Common/CascadingStylesheetsUnitTest.php could not be easily converted since it relies on comparing CSS files contents loaded from the file system, with those fetched from $GLOBALS['base_url'] via HTTP.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, system-common-phpunit-2002568-01.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, system-common-phpunit-2002568-01.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +PHPUnit

The last submitted patch, system-common-phpunit-2002568-01.patch, failed testing.

Berdir’s picture

+++ b/core/tests/Drupal/Tests/Core/Common/AutocompleteTagsUnitTest.phpundefined
@@ -33,7 +34,7 @@ public static function getInfo() {
   function testDrupalExplodeTags() {
...
@@ -44,8 +45,8 @@ function testDrupalImplodeTags() {

The test methods match the old function names. they should probably be renamed. Technically not strictly necessary but keeping them would be kinda weird I think.

+++ b/core/tests/Drupal/Tests/Core/Common/ColorTest.phpundefined
@@ -5,15 +5,15 @@
-class ColorTest extends UnitTestBase {
+class ColorTest extends UnitTestCase {

Wondering if I haven't seen this already somewhere, didn't that get in yesterday or so?

+++ b/core/tests/Drupal/Tests/Core/Common/ParseInfoFileUnitTest.phpundefined
@@ -0,0 +1,34 @@
+class ParseInfoFileUnitTest extends UnitTestCase {
...
+      'name' => 'Parsing .info.yml files',
...
+    $path = __DIR__ . '/../../../../../modules/system/tests/common_test_info.txt';
+    $info_values = drupal_parse_info_file($path);

Not sure if we even still need this, given that we converted everything to yml, including the name of this test :p

And including bootstrap.inc/common.inc is a no-go I think, I'd rather convert those tests to DUBT or rewrite the code to use classes and static methods like we did for other examples.

dawehner’s picture

Title: Convert system module's Common unit tests to phpunit » Convert tags,attributes, diff and url validation unit tests to phpunit
Status: Needs work » Needs review
FileSize
22.18 KB

Another classical example that you should never do too much in one issue.

I removed the conversions which didn't had conversions in bootstrap.inc and moved the explode/implode to a tags class.

jhedstrom’s picture

On a few of the other component conversions, the @deprecated tag has been added to the wrapper function. I think we should do that in this case as well.

Also, great idea on reducing the scope of this issue.

dawehner’s picture

FileSize
1.32 KB
22.85 KB

Good idea, let's do it here.

ParisLiakos’s picture

+++ b/core/includes/common.incundefined
@@ -772,6 +774,7 @@ function valid_email_address($mail) {
+ *
  * Verifies the syntax of the given URL.

@@ -5581,47 +5586,28 @@ function watchdog_severity_levels() {
+ *
  * Explodes a string of tags into an array.

unneeded changes

+++ b/core/tests/Drupal/Tests/Core/Common/AttributesTest.phpundefined
@@ -0,0 +1,56 @@
+    $this->assertSame((string) new Attribute(array('title' => '&"\'<>')), ' title="&amp;&quot;&#039;&lt;&gt;"', 'HTML encode attribute values.');
...
+    $this->assertSame((string) new Attribute(array('class' => array('first', 'last'))), ' class="first last"', 'Concatenate multi-value attributes.');
...
+    $this->assertSame((string) new Attribute(array('alt' => '')), ' alt=""', 'Empty attribute value #1.');
+    $this->assertSame((string) new Attribute(array('alt' => NULL)), ' alt=""', 'Empty attribute value #2.');
...
+    $this->assertSame((string) new Attribute($attributes), ' id="id-test" class="first last" alt="Alternate"', 'Multiple attributes.');
...
+    $this->assertSame((string) new Attribute(array()), '', 'Empty attributes array.');
...
+      $this->assertSame((string) $value, 'value1', 'Iterate over attribute.');

this should use a dataprovider

+++ b/core/tests/Drupal/Tests/Core/Common/AutocompleteTagsTest.phpundefined
@@ -31,21 +33,21 @@ public static function getInfo() {
+  function testImplodeTags() {

needs visibility

+++ b/core/tests/Drupal/Tests/Core/Common/ValidUrlTest.phpundefined
@@ -0,0 +1,154 @@
+  public function validAbsoluteProvider() {

lets prefix this with provider:)

+++ b/core/tests/Drupal/Tests/Core/Common/ValidUrlTest.phpundefined
@@ -0,0 +1,154 @@
+    $url_schemes = array('http', 'https', 'ftp');
+    foreach ($url_schemes as $scheme) {

those could be merged into the provider

+++ b/core/tests/Drupal/Tests/Core/Common/ValidUrlTest.phpundefined
@@ -0,0 +1,154 @@
+  public function invalidAbsoluteProvider() {

provider prefix instead too

+++ b/core/tests/Drupal/Tests/Core/Common/ValidUrlTest.phpundefined
@@ -0,0 +1,154 @@
+    $url_schemes = array('http', 'https', 'ftp');
...
+    foreach ($url_schemes as $scheme) {

same as well

and all test below in same pattern

dawehner’s picture

FileSize
10.79 KB
23.2 KB

Great review! Fixed a couple of more points like renaming the test classes etc.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, drupal-2003568-13.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit

#13: drupal-2003568-13.patch queued for re-testing.

ParisLiakos’s picture

+++ b/core/tests/Drupal/Tests/Core/Common/AttributesTest.phpundefined
@@ -0,0 +1,76 @@
+  public function provideAttributeData() {

you missed an 'r'

also, i am sorry i failed to mention above..we tend to add the Test..
so providerTestAttributeData

+++ b/core/tests/Drupal/Tests/Core/Common/UrlValidatorTest.phpundefined
@@ -0,0 +1,174 @@
+  public function providerValidAbsolute() {

providerTestValideAbsolute
likewise

+++ b/core/tests/Drupal/Tests/Core/Common/UrlValidatorTest.phpundefined
@@ -0,0 +1,174 @@
+  public function providerInvalidAbsolute() {

providerTestInvalidAbsolute

+++ b/core/tests/Drupal/Tests/Core/Common/UrlValidatorTest.phpundefined
@@ -0,0 +1,174 @@
+    foreach (array('', '/') as $front) {
...
+    foreach (array('', '/') as $front) {

we should move this foreach to the provider instead.

+++ b/core/tests/Drupal/Tests/Core/Common/UrlValidatorTest.phpundefined
@@ -0,0 +1,174 @@
+  public function dataEnhanceWithScheme($urls) {

should be protected right?

dawehner’s picture

FileSize
6.21 KB
23.8 KB

Fixed all the points

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

cool, its ready to go, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 573068f and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

ParisLiakos’s picture

seems this test fails on 5.5 #2068593: JsonTest fails with pecl-json

jhedstrom’s picture