Problem/Motivation

Poking around to evaluate #2803247: Incorrect namespace for CacheExampleForm I found that its test implements \Drupal\simpletest\WebTestBase, which is currently being deprecated in core: #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Also, the cache test is named CacheExampleTestCase, which should end in Test instead of TestCache.

Proposed resolution

  1. Make sure that all tests subclass Drupal\Tests\BrowserTestBase.
  2. Tests should be moved into [module]/tests/src/Functional and out of src/Tests
  3. Make sure test classes end in *Test so PHPUnit autoloads them.
  4. Make that test functions start with test* so PHPUnit will run them.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#29 interdiff.txt9.23 KBmile23
#29 2821387_29.patch72.11 KBmile23
#22 interdiff.txt7.6 KBmile23
#22 2821387_22.patch71.33 KBmile23
#20 interdiff.txt8.28 KBmile23
#20 2821387_20.patch72.35 KBmile23
#17 2821387_17.patch72.56 KBmile23
#15 examples.rest_of_modules_to_phpunit_2821387_15.patch72.24 KBrfay
#13 examples.rest_of_modules_to_phpunit_2821387_13.patch94.51 KBrfay
#8 examples.rest_of_modules_to_phpunit_2821387_08.patch90.88 KBrfay
#6 examples.rest_of_modules_to_phpunit_2821387_06.patch90.83 KBrfay

Comments

Mile23 created an issue. See original summary.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Issue summary: View changes

Updated issue summary.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Issue summary: View changes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

I'm at file_example, and maybe I'll get the rest done on the plane this afternoon. We can hope.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Active » Needs review
StatusFileSize
new examples.rest_of_modules_to_phpunit_2821387_06.patch90.83 KB

Here is the conversion to phpunit for the remaining modules.

Status: Needs review » Needs work

The last submitted patch, 6: examples.rest_of_modules_to_phpunit_2821387_06.patch, failed testing.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review
StatusFileSize
new examples.rest_of_modules_to_phpunit_2821387_08.patch90.88 KB

Status: Needs review » Needs work

The last submitted patch, 8: examples.rest_of_modules_to_phpunit_2821387_08.patch, failed testing.

The last submitted patch, 8: examples.rest_of_modules_to_phpunit_2821387_08.patch, failed testing.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

From https://dispatcher.drupalci.org/job/default/242568/console

It looks like the FieldExample one is a test system failure??? or a failure in simpletest module?

16:51:47 Next exception 'Drupal\Core\Database\IntegrityConstraintViolationException' with message 'SQLSTATE[23000]: Integrity constraint violation: 19 NOT NULL constraint failed: simpletest.file: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (?, ?, ?, ?, ?, ?, ?, ?); Array
16:51:47 (
16:51:47     [0] => 9
16:51:47     [1] => Drupal\Tests\field_example\Functional\FieldExampleBrowserTestBase
16:51:47     [2] => fail
16:51:47     [3] => 
16:51:47     [4] => Other
16:51:47     [5] => Drupal\Tests\field_example\Functional\FieldExampleBrowserTestBase->Warning()
16:51:47     [6] => 0
16:51:47     [7] => 
16:51:47 )

The pager_example one is an assert on a link that works fine locally, but that probably is difference in the testing system somehow. I run nginx, it's running apache, etc.

Behat\Mink\Exception\ExpectationException: Link containing href ?q=%2Fexamples%2Fpager-example&page=1 found.
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

OK, so if you can't fix them here, leave those out of the patch and we can make extra issues for those modules.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review
StatusFileSize
new examples.rest_of_modules_to_phpunit_2821387_13.patch94.51 KB

You're so smart. This removes field_example and pager_example, which have been split out into separate patches.

Status: Needs review » Needs work

The last submitted patch, 13: examples.rest_of_modules_to_phpunit_2821387_13.patch, failed testing.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review
StatusFileSize
new examples.rest_of_modules_to_phpunit_2821387_15.patch72.24 KB
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: Unassigned » mile23
Status: Needs review » Needs work
Issue tags: +Needs reroll

Working on a re-roll.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2821387_17.patch72.56 KB

Reroll.

Also:

renamed:    config_entity_example/tests/Functional/ConfigEntityExampleTest.php -> config_entity_example/tests/src/Functional/ConfigEntityExampleTest.php
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

PHPCS says:

FILE: ..._entity_example/tests/src/Functional/ConfigEntityExampleTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 15 | ERROR | [x] Expected 1 space before opening brace; found 2
    |       |     (Drupal.Classes.ClassDeclaration.SpaceBeforeBrace)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...dules/examples/cron_example/tests/Functional/CronExampleTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 15 | ERROR | [x] Expected 1 space before opening brace; found 2
    |       |     (Drupal.Classes.ClassDeclaration.SpaceBeforeBrace)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...examples/dbtng_example/tests/src/Functional/DbtngExampleTest.php
----------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
----------------------------------------------------------------------
  16 | ERROR | [x] Expected 1 space before opening brace; found 2
     |       |     (Drupal.Classes.ClassDeclaration.SpaceBeforeBrace)
 120 | ERROR | [x] There should be no white space before a closing
     |       |     "]"
     |       |     (Drupal.WhiteSpace.CloseBracketSpacing.ClosingWhitespace)
 165 | ERROR | [x] Inline comments must end in full-stops, exclamation
     |       |     marks, colons, question marks, or closing parentheses
     |       |     (Drupal.Commenting.InlineComment.InvalidEndChar)
 190 | ERROR | [x] Inline comments must end in full-stops, exclamation
     |       |     marks, colons, question marks, or closing parentheses
     |       |     (Drupal.Commenting.InlineComment.InvalidEndChar)
 195 | ERROR | [x] Inline comments must end in full-stops, exclamation
     |       |     marks, colons, question marks, or closing parentheses
     |       |     (Drupal.Commenting.InlineComment.InvalidEndChar)
 197 | ERROR | [x] Inline comments must end in full-stops, exclamation
     |       |     marks, colons, question marks, or closing parentheses
     |       |     (Drupal.Commenting.InlineComment.InvalidEndChar)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...xamples/fapi_example/tests/src/Functional/FapiExampleWebTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 15 | ERROR | [x] Expected 1 space before opening brace; found 2
    |       |     (Drupal.Classes.ClassDeclaration.SpaceBeforeBrace)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...s/node_type_example/tests/src/Functional/NodeTypeExampleTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 91 | WARNING | [x] There must be no blank line following an inline
    |         |     comment
    |         |     (Drupal.Commenting.InlineComment.SpacingAfter)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ugin_type_example/tests/src/Functional/PluginTypeExampleTest.php
----------------------------------------------------------------------
FOUND 3 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
----------------------------------------------------------------------
 50 | WARNING | [ ] Line exceeds 80 characters; contains 107
    |         |     characters (Drupal.Files.LineLength.TooLong)
 50 | WARNING | [x] There must be no blank line following an inline
    |         |     comment
    |         |     (Drupal.Commenting.InlineComment.SpacingAfter)
 50 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found
    |         |     0 (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
 50 | ERROR   | [ ] Comment indentation error, expected only 1 spaces
    |         |     (Drupal.Commenting.InlineComment.SpacingBefore)
 50 | ERROR   | [x] Inline comments must end in full-stops, exclamation
    |         |     marks, colons, question marks, or closing parentheses
    |         |     (Drupal.Commenting.InlineComment.InvalidEndChar)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../tablesort_example/tests/src/Functional/TableSortExampleTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 71 | ERROR | [x] Inline comments must end in full-stops, exclamation
    |       |     marks, colons, question marks, or closing parentheses
    |       |     (Drupal.Commenting.InlineComment.InvalidEndChar)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ules/examples/tour_example/tests/src/Functional/TourTestBase.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 13 | ERROR | [x] Expected 1 space before opening brace; found 2
    |       |     (Drupal.Classes.ClassDeclaration.SpaceBeforeBrace)
 17 | ERROR | [ ] Doc comment short description must be on a single line,
    |       |     further text should be a separate paragraph
    |       |     (Drupal.Commenting.DocComment.ShortSingleLine)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...les/examples/tour_example/tests/src/Functional/TourTestBasic.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
  4 | WARNING | [x] Unused use statement
    |         |     (Drupal.Classes.UnusedUseStatement.UnusedUse)
  9 | WARNING | [ ] Line exceeds 80 characters; contains 82
    |         |     characters (Drupal.Files.LineLength.TooLong)
 46 | ERROR   | [x] Missing function doc comment
    |         |     (Drupal.Commenting.FunctionComment.Missing)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 10.1 secs; Memory: 16Mb

More review-y:

  1. +++ b/block_example/src/Tests/BlockExampleMenuTest.php
similarity index 69%
rename from block_example/tests/src/Functional/BlockExampleTest.php

rename from block_example/tests/src/Functional/BlockExampleTest.php
rename to block_example/src/Tests/BlockExampleTest.php

    Wait... How is this going backwards from BTB to simpletest? 

  2. +++ b/content_entity_example/tests/src/Functional/ContentEntityExampleTest.php
similarity index 67%
rename from cron_example/src/Tests/CronExampleTestCase.php

rename from cron_example/src/Tests/CronExampleTestCase.php
rename to cron_example/tests/Functional/CronExampleTest.php

    Needs to go to cron_example/tests/src/ 

  3. +++ b/simpletest_example/src/Controller/SimpleTestExampleController.php
@@ -14,8 +14,15 @@ class SimpleTestExampleController {
+      '#markup' => t('Please note that the use of SimpleTest is deprecated. ¶
+            This example module will be removed in Drupal 9, and new tests ¶
+            should not be written using SimpleTest. In addition, all existing ¶
+            SimpleTest tests should be converted to PHPUnit functional tests. ¶
+            ¶
+            This Simpletest Example is designed to give an introductory tutorial to writing
+            a simpletest test. ¶
+            ¶
+            Please see the <a href="http://drupal.org/node/890654">associated tutorial</a>.'),

    Bunch of whitespace errors. This or #2799535: Convert SimpleTestExampleController to use DescriptionTemplateTrait will need a re-roll anyway. 

  4. +++ b/tablesort_example/tests/src/Functional/TableSortExampleTest.php
@@ -33,26 +33,53 @@ class TableSortExampleTest extends WebTestBase {
+    drupal_flush_all_caches();
...
+    drupal_flush_all_caches();
...
+    drupal_flush_all_caches();
...
+    drupal_flush_all_caches();
...
+    drupal_flush_all_caches();
...
+    drupal_flush_all_caches();

    Not especially happy with this in our test. Could the path controller need new caching directives?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Needs a re-roll after #2799535: Convert SimpleTestExampleController to use DescriptionTemplateTrait

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2821387_20.patch72.35 KB
new interdiff.txt8.28 KB

Reroll plus fixed all the CS errors.

Still to do: Review stuff from #18.

Status: Needs review » Needs work

The last submitted patch, 20: 2821387_20.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2821387_22.patch71.33 KB
new interdiff.txt7.6 KB

Dealt with all the stuff in #18 and more. Notable exception is #18.4 which needs a follow-up.

Poking through, I found a few modules that didn't have a conversion.

The notable exception being dealt with here: #2821773: Convert field_example tests to phpunit functional

Holy cow phpunit_example still has ::getInfo()! :-)

Torenware’s picture

Torenware commented

Notable exception is #18.4 which needs a follow-up.

Test fails if they're removed?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

I haven't investigated. That'd be the follow-up. :-)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

The tests fail if we remove drupal_flush_all_caches() so I filed #2843199: Figure out why TableSortExampleTest needs drupal_flush_all_caches() in the test as a follow-up, since this issue is about doing the conversion.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Somebody sorted that for us at the time, as I remember: #2821742: Tablesort Example nonfunctional for non-admin user

Torenware’s picture

Torenware commented

This is near finished. In general, the right way to do a port is to change as little as you can get away with. This port is mostly done the right way.

The one thing that personally I would change: I think it's important that asserts immediately tell you what went wrong if and when they fire. Mostly, you get this by using the correct assertion function. The more specific the assertion you use, the more informative it is.

The main problem in the current patch is where you don't have a good specific assertion, and need to do things like check the count of some object or record. In these cases, the assertion used is typically ::assertEquals(N, count(Something)). In these cases, I think that adding custom assertion text is best practice, since the error is not that "the numbers are not equal" -- the problem is that there is some number of Something != N.

YMMV, but since tests are intended to help developers track down problems in their code, "literate assertions" are a good practice.

Besides this, I don't see anything that needs changing before this is committed.

  1. +++ b/dbtng_example/tests/src/Functional/DbtngExampleTest.php
@@ -41,22 +41,22 @@ class DbtngExampleTest extends WebTestBase {
-    $this->assertEqual(
-      count($result), 2, 'Found two entries in the table after installing the module.'
-    );
+    $this->assertEquals(2, count($result));

    General point about omitting the "info" part of an assert: if the assert goes off, is it obvious what went wrong. In this case, the usage with the Simpletest assertion is better than the Mink assert, since "not 2" does not tell you what went wrong, as the original did. 

  2. +++ b/dbtng_example/tests/src/Functional/DbtngExampleTest.php
@@ -105,27 +108,28 @@ class DbtngExampleTest extends WebTestBase {
-    $this->assertEqual(
-      count($result), 1, 'Found one entry in the table with surname = "Anonymous".'
-    );
+    $this->assertEquals(1, count($result));

    Same as above; it won't be clear what "not 1" means. 

  3. +++ b/dbtng_example/tests/src/Functional/DbtngExampleTest.php
@@ -105,27 +108,28 @@ class DbtngExampleTest extends WebTestBase {
+    $this->assertEquals(1, count($rows));

@@ -150,36 +154,29 @@ class DbtngExampleTest extends WebTestBase {
+    $this->assertEquals(4, count($result));
...
+    $this->assertEquals(2, count($result));
...
+    $this->assertEquals(1, count($result));
...
+    $this->assertEquals(1, count($result));
...
+    $this->assertEquals(1, count($result));

@@ -190,27 +187,23 @@ class DbtngExampleTest extends WebTestBase {
+    $this->assertEquals(1, count($result));
...
+    $this->assertEquals(3, count($result));

    Several more examples of asserts that will have cryptic messages if they are triggered. 

+++ b/plugin_type_example/tests/src/Functional/PluginTypeExampleTest.php
@@ -38,20 +40,17 @@ class PluginTypeExampleTest extends WebTestBase {
+    $this->assertEquals(2, count($sandwich_plugin_definitions));

--- a/queue_example/src/Tests/QueueExampleTest.php
+++ b/queue_example/tests/src/Functional/QueueExampleTest.php

+++ b/tablesort_example/tests/src/Functional/TableSortExampleTest.php
@@ -33,26 +33,53 @@ class TableSortExampleTest extends WebTestBase {
+    $this->assertEquals(7, $item->getText());
...
+    $this->assertEquals('w', $item->getText());
...
+    $this->assertEquals('a', $item->getText());

A few more examples of asserts that won't explain themselves.

Torenware’s picture

Torenware commented
Status: Needs review » Needs work

Again, near done. Assertions just need to be a little more informative in some corner cases.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2821387_29.patch72.11 KB
new interdiff.txt9.23 KB

Hope this documents adequately.

Changed some assertion types while I was at it.

  • Torenware committed 3047137 on 8.x-1.x authored by Mile23 
    Issue #2821387 by Mile23, rfay: Convert all tests to BrowserTestBase
Torenware’s picture

Torenware commented

Gentlemen -- you are proud fathers of an... issue.

Committed.

Torenware’s picture

Torenware commented
Status: Needs review » Fixed
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Bit of a follow-up: #2843692: Drupal\Tests\cron_example\Functional\CronExampleTest is failing

Status: Fixed » Closed (fixed)

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