Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Assigned: Unassigned » Berdir

This is a bit tricky, more later.

Anyway, I have some code lying around (Actually, more than that, I can successfully run tests) so assigning to me to get the lock :)

Berdir’s picture

In case someone is up for some bike shedding:


namespace Drupal\simpletest\Tests;

abstract class Test {}

abstract class UnitTest extends Test {}

abstract class WebTest extends Test {}

Any objections?

Berdir’s picture

Status: Active » Needs review
Issue tags: +PSR-0
FileSize
396.56 KB

Yay, TrackerTest is commited.

So, the main issue here is that we need to add a use statement to all .test files and change the extends bit of all test class definitions.. I think it's best to do this before starting to convert the test code or we will have to first add a use for \DrupalWebTestCase and then fix it all over the place. Doing it this way round allows it to do it correctly from the beginning when moving the test code to PSR-0.

I was able to execute some example tests both in the UI and with run-tests.php, let's see what the testbot thinks. It's possible that a seldomly used method still has a missing use or something like that. Also possible that the testbot doesn't like this at all.

Berdir’s picture

Ups, the above patch contained a test file, removed that.

Status: Needs review » Needs work

The last submitted patch, convert-simpletest-to-psr0-1541676-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
374.03 KB

Strange, no idea why that function call is trying to be executed within the namespace. Let's try this for now.

Also, not sure about the locales_source not found pdo exception...

Status: Needs review » Needs work

The last submitted patch, convert-simpletest-to-psr0-1541676-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
375.33 KB

Because the function was defined in drupal_web_test_case.php and I forgot to move it.

Changed namespace from Drupal\simpletest\Tests to Drupal\simpletest because, as discussed with sun, these aren't tests, they are the API for other modules.

Let's see if this is any better.

neclimdul’s picture

At the cost of possibly opening a bikeshed, I've got 2 technical questions about PSR-0'd tests.

  1. Based on tracker moving its tests to core/modules/tracker/lib, I'm curious where unit tests, specifically unit tests for subsystem, will live. Would that be Drupal\system\Tests\SubsystemUnitTest registered by the system module?
  2. And if they live in Drupal\system\Tests\SubsystemUnitTest, how would that unit test use mock objects? Would my mock objects live in Drupal\system or Drupal\system\Tests as well or would I be responsible for registering a namespace manually or containing all mock objects in the test file?

Note: Since SimpleTest runs in a long running parent process the registering of namespaces is permanent unless the test has its own loader. Also auto-loading can't be undone so testing of loading functionality is in someways impossible or at best fragile. This isn't new though just to be considered.

kika’s picture

Time for a blast from the past: #249553: Rename SimpleTest to Testing (test.module). Should it be re-considered now?

xjm’s picture

I am not sure this issue belongs on http://drupal.org/node/1543796, because the base classes aren't test classes; they're API. (As we've pointed out in other contexts, but I think this issue should eventually have its own change notification instead.) I've left it on there for now as a reference or whatever, or in case I'm wrong. :)

Berdir’s picture

I did add there because once this has been commited, the examples there should be updated to use the new class names and namespaces IMHO. So it did make sense to me to add this one as a related issue. That it deserves it's own separate change record is possible, maybe it can also be grouped with something else.

Berdir’s picture

Issue tags: -PSR-0

Status: Needs review » Needs work
Issue tags: +PSR-0

The last submitted patch, convert-simpletest-to-psr0-1541676-8.patch, failed testing.

Berdir’s picture

Title: Convert Simpletest to PSR-0 » Convert Simpletest base test classes to PSR-0
Status: Needs work » Needs review
FileSize
173.81 KB

Ok, here is a re-roll.

@neclimdul: There have been discussions on extending the test class search to core/lib so that tests can be moved out of system.module. But yes, right now, you'd do that. Not sure about the mock objects, in the test namespace would probably make sense. However, this is not relevant for this issue, the detection and handling of PSR-0 tests has been defined in the TrackerTest issue, your questions should probably be handled in a new issue or maybe the meta issue linked above. All this issue does is converting the base test classes to PSR-0. Updated the title to clarify that.

@kika: Same, that shouldn't happen here imho but stay in that issue. If we decide to do that, then all we need to do is a search & replace for Drupal\simpletest to Drupal\testing (additional to converting the module itself) once this is in.

Patch is this time with git diff -M. Half the size, but imho less readable.

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-15.patch, failed testing.

sun’s picture

Thanks! This looks almost ready to me.

+++ b/core/modules/forum/forum.test
@@ -6,9 +6,10 @@
-  protected $admin_user;
...
+  erotected $admin_user;

Typo ;)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTest.php
@@ -1,768 +1,25 @@
+use PDO;
+use stdClass;
+use DOMDocument;
+use DOMXPath;
+use SimpleXMLElement;

I don't think these are necessary.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTest.php
@@ -3565,8 +2822,8 @@ class DrupalWebTestCase extends DrupalTestCase {
 function simpletest_verbose($message, $original_file_directory = NULL, $test_class = NULL) {

+++ b/core/modules/simpletest/simpletest.module
@@ -565,3 +570,51 @@ function simpletest_mail_alter(&$message) {
+function simpletest_verbose($message, $original_file_directory = NULL, $test_class = NULL) {

I never understood why these two are procedural functions. Should be tacked onto WebTest. But, separate issue.

+++ b/core/modules/simpletest/simpletest.module
@@ -176,6 +177,10 @@ function _simpletest_batch_operation($test_list_init, $test_id, &$context) {
+  // Prefix test classes with \ if it is missing.
+  if (strpos($test_class, '\\') === FALSE) {
+    $test_class = '\\' . $test_class;
+  }
   $test = new $test_class($test_id);

Is this necessary? If so, the comment should state why.

Doesn't look like it is needed though, because the equivalent change is missing in run-tests.sh, but testbot still executed.

Berdir’s picture

Status: Needs work » Needs review
FileSize
173.38 KB

Thanks for the review.

- Fixed the weird typo.
- The uses are necessary, otherwise PHP will look for e.g. Drupal\simpletest\PDO. The alternative would be using \PDO but according to the coding standards, this is only allowed if something is only used once, which is not true for most of them and core already hase various use PDO;'s.
- Weird, I am pretty sure the \\ hack was necessary and I only added it after it failed. But yes, you are correct, it is in the .module file and it works just fine without. Removed.

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-18.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
173.33 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
174.06 KB

Hm, looks like I forgot to save one of changed .test files previously.

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -565,3 +566,51 @@ function simpletest_mail_alter(&$message) {
+ *   The ID of the message to be placed in related assertion messages.
+ *
+ * @see Drupal\simpletest\Test->originalFileDirectory()
+ * @see Drupal\simpletest\WebTest->verbose()
+ */
+function simpletest_verbose($message, $original_file_directory = NULL, $test_class = NULL) {
+  static $file_directory = NULL, $class = NULL, $id = 1, $verbose = NULL;
+
+  // Will pass first time during setup phase, and when verbose is TRUE.
+  if (!isset($original_file_directory) && !$verbose) {
+    return FALSE;

Looks like we're moving simpletest_verbose() loosely into WebTest.php? Could we move that function in either as a function of Test or WebTest itself? Having loose functions in PSR-0 PHP files will mean the function won't be available unless we autoload the WebTest class. Seems like something we want to avoid. Let's just move it into the Test::verbose() function itself.

  Test::verbose(); // ?

-28 days to next Drupal core point release.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
175.38 KB

Looks like it was in both simpletest.module and WebTest.php. simpletest.module it is. Also removed an empty whitespace.

sun’s picture

This patch looks ready to me. But if possible, I'd like to do #249553: Rename SimpleTest to Testing (test.module) first (resulting in a simple in-patch-file rename here).

sun’s picture

Scratch #25. Don't want to block this.

sun’s picture

Issue tags: -PSR-0

#24: 1541676.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1541676.patch, failed testing.

Issue tags: +PSR-0

The last submitted patch, 1541676.patch, failed testing.

Berdir’s picture

Re-rolled. An interdiff doesn't make much sense I think, I mostly fixed merge conflicts (quite a few of them) and adapted for the changes to the setUp() method. Also renamed all references to WebTest in docs to use the fully qualified name. I found them searching for " WebTest::".

aspilicious’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/config/config.test
@@ -6,11 +6,15 @@
+use Drupal\Core\Config\SignedFileStorage;

This is an error from the merge. SignedFileStorage no longer exists.

+++ b/core/modules/config/config.test
@@ -6,11 +6,15 @@
+  protected $profile = 'testing';

Yay!

+++ b/core/modules/field/modules/options/options.test
@@ -5,6 +5,8 @@
 
+use Drupal\simpletest\WebTest;
+
 class OptionsWidgetsTestCase extends FieldTestCase {

This shouldn't be needed.

+++ b/core/modules/field_ui/field_ui.test
@@ -6,16 +6,17 @@
+    // to be passed in as either an array or a variable number of string arguments.

This should wrap after "string" due to the longer method name above.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Test.php
@@ -0,0 +1,676 @@
+ * Do not extend this class, use one of the subclasses in this file.

The "in this file" part is no longer accurate.

+++ b/core/modules/system/tests/unicode.test
@@ -5,10 +5,13 @@
+use Drupal\simpletest\WebTest;
+++ b/core/modules/system/tests/upgrade/upgrade.language.test
@@ -4,6 +4,8 @@
+use Drupal\simpletest\WebTest;
+++ b/core/modules/system/tests/uuid.test
@@ -1,11 +1,13 @@
+use Drupal\simpletest\WebTest;

Seems like these are all not needed.

sun’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
208.33 KB

Thanks for the review, looks like you actually went through it, awesome ;)

- Remove the merge mess in config.test. The profile is now testing by default, re-removed that as well ;)
- Updated the docblocks and removed the unecessary uses. I think I just added that one to all .test files without looking if it's actually needed ;)
- Added Base suffix to Test/UnitTest/WebTest.

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
208.6 KB

And another new test class :)

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-36.patch, failed testing.

tstoeckler’s picture

+++ b/core/modules/poll/poll.test
@@ -5,7 +5,9 @@
+class PollWebTestCase extends WebTestBase {

@@ -190,7 +192,7 @@ class PollWebTestCase extends DrupalWebTestCase {
+class PollCreateTestCase extends PollWebTestBaseCase {

Those don't match. Do we really want PollWebTestBaseCase though?
It seems that #1567920: Naming standard for abstract/base classes would imply PollWebTestBase, IINM.

Berdir’s picture

Status: Needs work » Needs review
FileSize
205.6 KB

Yeah, that was a wrong search & replace rename, sorry about that. Should test that stuff locally :)

Renaming the actual test classes will be part of the PSR-0 conversion of them.

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-39.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
205.7 KB

Missed some Test -> TestBase renames...

Status: Needs review » Needs work

The last submitted patch, simpletest-psr0-1541676-41.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
205.71 KB

Ok, one last remaining WebTest in there, this one should be green! I hope...

tstoeckler’s picture

Status: Needs review » Needs work
FileSize
11.27 KB

Alright this review was a bit lengthy, so I dumped it into a text file in order to not spam this issue too much. Most of the issues were unnecessary renames of Base (or not base...) classes.

Berdir’s picture

Status: Needs work » Needs review
FileSize
182.2 KB

Oh wow, that rename went totally sideways :( Sorry about that!

- Yeah, I noticed the weird stuff in CommentNodeAccessTest as well. I'm not sure but it might even be trying to go around CommentHelperCase::setUp() explicitly, because it does a node_access_rebuild() between the setUp() and creating the users/node. I'd agree separate issue though, because I would like us to start requiring an array for setUp() and stop dancing around :) sun even has an issue/patch where he moves the module declaration to a class property.
- Wasn't sure about the abstract WebTestBase but it makes sense to me.. it doesn't make any sense to instantiate that class directly. Also noticed that TestBase calls setUp() and tearDown() so these should be declared as abstract methods there, but I think that should be done in a follow-up.
- Removed all wrong Base's. Search for "Base extends" only gives me WebTestBase and UnitTestBase, searching for "BaseCase" returns no results. So I hope I got all of them.

Now we're almost back at the original file size. That should have made it obvious that something went wrong :)

Status: Needs review » Needs work
Issue tags: -PSR-0

The last submitted patch, simpletest-psr0-1541676-45.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +PSR-0

#45: simpletest-psr0-1541676-45.patch queued for re-testing.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
179.86 KB
+++ b/core/modules/comment/comment.test
@@ -1515,7 +1516,7 @@ class CommentNodeAccessTest extends CommentHelperCase {
-    DrupalWebTestCase::setUp('comment', 'search', 'node_access_test');
+    WebTestBase::setUp('comment', 'search', 'node_access_test');

I've read that one time too many now. Opened #1569856: Make CommentHelperCase support $modules like DrupalWebTestCase and make CommentNodeAccessTest use that

+++ b/core/modules/system/tests/upgrade/upgrade.test
index 0000000..83d85a0
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/upgrade/upgrade_bare.test

+++ b/core/modules/system/tests/upgrade/upgrade_bare.test
index 0000000..7a7c079
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/upgrade/upgrade_filled.test

These hunks basically revert #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x. I personally don't know why they were removed, but I guess it was for a reason.

Either way this patch is RTBC. I've attached a patch, which is exactly (!) the same as the above, I just deleted those two hunks from the patch file. It just needs to be decided, which one is correct.

Please do not revert RTBC status, unless there is something seriously wrong with this patch. I've read through the entire patch 3 times now, and it's really a pain. So if someone finds coding standards violations or something (I didn't), this is definitely a case where that would warrant a follow-up.

sun’s picture

I've reviewed all changes of #48 in detail. Truly RTBC.

catch’s picture

Title: Convert Simpletest base test classes to PSR-0 » Change notice for: Convert Simpletest base test classes to PSR-0
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed/pushed this one so it doesn't get broken again. This will need to be added to the overall PSR-0 change notice.

Berdir’s picture

Status: Active » Needs review

I have updated http://drupal.org/node/1543796, it makes IMHO sense to make that a general "How to change test classes from 7.x to PSR-0/8.x". Added a block about the renamed base test classes, added a point about adding a Base suffix to module base test classes and updated the examples.

Not sure about the general PSR-0 part of this, should this be added to http://drupal.org/node/1320394? Not sure if it makes sense to list all modules that have classes there, it's kinda obvious and unless we'd add all classes within that module, not really helpful because they have been renamed as well.

aspilicious’s picture

Watch out with backslahses in change notices! They dissapear sometimes.

sun’s picture

Priority: Critical » Normal
Status: Needs review » Fixed

Thanks! Change notice looks good to me. (minimally tweaked it)

webchick’s picture

Title: Change notice for: Convert Simpletest base test classes to PSR-0 » Convert Simpletest base test classes to PSR-0
tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
397 bytes

Briefly reopening this, the files[] entry for drupal_web_test_case.php was left in.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Upsie. Yep, that needs to go.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yep. Committed/pushed.

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