Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

As suggested in IRC, It would be great to be able to move most if not all of them to the Drupal\Core namespace.

The easiest would be to introduce Drupal\Core\Tests (or even Drupal\Tests) and then e.g. have Drupal\Core\Tests\Database\SelectQueryTest. That would just require a single, static classloader extension and path to look for test files..

RobLoach’s picture

  1. Make a separate issue for each of System module's .test files since there are way too many to do it all in one issue
  2. Move individual test files to their own namespace/directory at modules/system/lib/Drupal/system/Tests/* (tests in password.test move to modules/system/lib/Drupal/system/Tests/Password/*)
  3. Remove "Drupal" from the test name (DrupalAlterTest becomes AlterTest)
  4. Replace "TestCase" with just "Test" (ElementTestCase becomes ElementTest)
  5. Remove the prefixed namespace name if it's present in the class (PasswordHashingTest becomes Drupal\System\Tests\Password\HasingTest)

Here's an example of the transition:

modules/system/tests/common.test
CommonDrupalAlterTestCase -> modules/system/lib/Drupal/system/Tests/Common/AlterTest.php
CommonAutocompleteTagsTestCase -> modules/system/lib/Drupal/system/Tests/Common/AutocompleteTagsTest.php
CommonDrupalHTTPRequestTestCase -> modules/system/lib/Drupal/system/Tests/Common/HttpRequestTest.php
modules/system/tests/form.test
FormsTestCase -> modules/system/lib/Drupal/system/Tests/Form/FormsTest
FormElementTestCase -> modules/system/lib/Drupal/system/Tests/Form/Element.php
FormsElementsTableSelectFunctionalTest -> modules/system/lib/Drupal/system/Tests/Form/ElementsTableSelectFunctionalTest.php
modules/system/tests/session.test
SessionTestCase -> modules/system/lib/Drupal/system/Tests/Session/SessionTest.php
SessionHttpsTestCase -> modules/system/lib/Drupal/system/Tests/Session/HttpsTest.php

[EDIT] Actually, we could move those to Drupal\Core\Tests\* when appropriate. session.test's SessionHttpsTestCase becomes core/lib/Drupal/Core/Tests/Session/HttpsTest.php.

aspilicious’s picture

it's funny that we alrdy have a "tests" folder, but not "Tests"

RobLoach’s picture

core/tests
Deprecated and should be removed. Stuck in to mimic Symfony, but they've moved their tests back into the components, closer to the source.
core/lib/Drupal/Core/Tests
I talked briefly with sun, and he said SimpleTest does not look for tests in Drupal/Core/Tests. I'm also having thoughts that this might not be a place for them either, as these are tests for our procedural code.

Taking those to things into consideration, what if we were to just stick them in the system module's lib directory for now, and consider moving them later on. Spliting the files into PSR-0 classes seems like the first step. We can worry about the "proper" location later on as it's easier to move files. I've put together #1597630: Convert actions.test to PSR-0 as a start.

Crell’s picture

Test for OOPified code in /lib should, I suppose, live with it, or at least somewhere in /lib.

Tests for procedural code in /includes, shouldn't that go in simpletest, not in system? Either way, I'm fine with that being tied to a module for now.

RobLoach’s picture

I like the idea of it going in modules/simpletest/lib. Set #1597630: Convert actions.test to PSR-0 to needs work.

Berdir’s picture

We just moved them from simpletest to system, do you really want to revert that? See http://drupal.org/node/1540510

RobLoach’s picture

Let's keep them in system module then! Added issues for each task in the issue summary.

RobLoach’s picture

Issue summary: View changes

tasks

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Component: node.module » system.module
Issue tags: +Testing system
sun’s picture

1) Please make sure to (re-)roll all of those patches with:

git diff -C -M5%

2) Almost every of those patches will break major efforts happening in parallel currently. I wonder whether it wouldn't make sense to defer all of them to code freeze - or at least, a single, coordinated date.

RobLoach’s picture

Rebasing to update/reroll patches is quite simple. rfay's article and xjm's notes on it are great. If conflicts do occur, it is easy to reroll these PSR-0 patches. If you have any particular conflicts in mind, please feel free to post notes about them in their respective issues so that we can coordinate resolutions.

The PSR-0 RTBC queue is up and growing. It's up to catch to commit them, and he has been doing so.

webchick’s picture

Just a note that there is the Avoid commit conflicts tag for specifically flagging issues that are a) *really* close to (or at) RTBC, and b) a complete and utter pain in the patootie to re-roll. Core committers will check this queue for conflicts before going on a commit spree.

aspilicious’s picture

2) Almost every of those patches will break major efforts happening in parallel currently. I wonder whether it wouldn't make sense to defer all of them to code freeze - or at least, a single, coordinated date.

No I *completly* disagree with this. Every issue in this list conflicts with it self so we have to commit this one by one. So it will take weeks before every item on this list is done.

Btw if this gets in you have to reroll those conflicted patches once. Every time a test gets committed we have to reroll these patches. So in the end you just say, let aspilicious reroll his patches 30 times because this is more important.

I don't realy get the fuzz about waiting untill code freez, everyone knows we have lots of work to do than to clean everything so why not doing as much as possible when we can. Rerolling an importing patch maybe takes 20 minutes but in Drupal land 20 minutes is *nothing*.

AND last thing before I shut up, once this is done every test class sits in its own file so there will be less conflicts in the end. NOW chances are big test files conflict because of this 4000+ line test file madness.

Berdir’s picture

What's more important is that we need to convert all classes to PSR-0 so that we can remove the registry, we can't do that in the code cleanup phase.

Niklas Fiekas’s picture

Note that Crell has asked catch to wait with commiting PSR-0 patches at least until #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel is in (May 31). He probably had forgotten that when he committed one.

aspilicious’s picture

Added the tag webchick proposed and made a list of affected test files to that issue.
So we still can commit other test conversions. Not every file is block on that one.

Crell’s picture

I agree that we don't want to delay this migration too long, because it does (should?) make things easier after it's done.

What about scheduling them in pieces? Vis, "node module's test shifting goes in X date, so everyone beware, but you can still mess with user module this week". "OK, user module's tests get moved around this week, FYI, but you can still hack away at block module." Etc.

That at least means we don't need to chase head on 30 test-move patches at a time, just one at a time.

aspilicious’s picture

There are to many patches that depend on various subsystems, you can't postpone a single test file. Catch has pushed some tests alrdy so it's going well.

aspilicious’s picture

There are to many patches that depend on various subsystems, you can't postpone a single test file. Catch has pushed some tests alrdy so it's going well.

catch’s picture

fwiw I've committed a bunch of PSR-0 patches after aspilicious posted a list of ones that conflict with the kernel patch, there's currently no other active patches tagged with 'avoid commit conflicts', and the conflicts created by these patches do not make for difficult re-rolls for other patches - it's not like committing a big API or coding standards change all at once.

Apart from that patch, I don't see any reason to delay doing this migration in general. Once we're able to remove the registry,
#534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap becomes mostly a D7 issue, so that's one less (very, very hard to fix) critical bug against 8.x. While it's a very long way around to get rid of that bug, this is the way we've decided to do it, so PSR-0 conversion is a release blocker unless that issue gets magically out of limbo soon.

While tests for procedural code should stay in system (we moved them there because simpletest module should really just be a UI for running simpletests, ideally it wouldn't have DrupalUnitTestCase or DrupalWebTestCase in it either), it'd be great if we could add support for including test code with stuff in /Core and Component, and then move classes there - would save doing the move-around twice.

Does that need its own issue?

sun’s picture

The latter should be fairly easy to do, since we've introduced a single helper function for Simpletest to discover the PSR-0 tests, and yes, dedicated issue, please.

catch’s picture

catch’s picture

Status: Active » Postponed

And marking this postponed on that. If we can move all those tests again once, it'd be good.

catch’s picture

Status: Postponed » Active

Doh, ignore that. This only counts for stuff in lib, not procedural code, so it's not actually that many tests anyway.

catch’s picture

Issue summary: View changes

Updated issue summary.

aspilicious’s picture

Issue summary: View changes

Added upgrade tests

aspilicious’s picture

Ok we decided to leave out the .info changes for now because they are conflicting all the time!
Ow yeah!

We have to clean those in the end!

aspilicious’s picture

Theme.tests reveiled that we have also have to convert the ThemeRegistry class found in theme.inc.
We shouldn't forget that ;)

RobLoach’s picture

Title: Convert system tests to PSR-0 » Convert system tests to PSR-0 and remove system.info's files[] entry

Updating the title to reflect that :-) .

RobLoach’s picture

Issue summary: View changes

Removed cruft issues

aspilicious’s picture

Status: Active » Needs review
FileSize
257.96 KB

Here is a patch for system.test itself. It's a complicated patch due to renames and different namespaces. You should apply it to see how it's structured.

If I did something completly wrong tell me :)

Status: Needs review » Needs work

The last submitted patch, 1593058-system-tests-psr0-29.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
258.35 KB

This should be green...

aspilicious’s picture

Issue summary: View changes

Removed crufty issue

aspilicious’s picture

Rerolled!

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community
 rename core/modules/system/lib/Drupal/system/Tests/{Common => Lock}/LockFunctionalTest.php (97%)
 rename core/modules/system/lib/Drupal/system/Tests/{Common => Mail}/HtmlToTextTest.php (99%)
 rename core/modules/system/lib/Drupal/system/Tests/{Common => Mail}/MailTest.php (95%)
 rename core/modules/system/lib/Drupal/system/Tests/{Common => System}/PasswordHashingTest.php (96%)

These are already PSR-0, I do enjoy a good namespace cleansing though!

After looking through the patch and checking through the file definitions, I'm liking this patch more and more. This patch is huge, in terms of both its size and its awesomeness. Well done! RTBCed.

RobLoach’s picture

#32: 1593058-system-tests-psr0-32.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, nice work!

Leaving this open since we still have the files[] entry at the end.

RobLoach’s picture

Title: Convert system tests to PSR-0 and remove system.info's files[] entry » Remove system.info's files[] entry
Status: Active » Postponed

Setting to postponed until the other patches get in :-) .

RobLoach’s picture

Issue summary: View changes

Update info

aspilicious’s picture

Status: Postponed » Needs review
FileSize
11.08 KB

Did a grep to check if we had every instance of "files[]" and apparantly there are some sneaky instances left. This patch should fix those... Hopefully...

Berdir’s picture

The stream wrapper changes in file_test.module are already part of #1598574: Convert file.test to PSR-0.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/system_test/lib/Drupal/system_test/FileTransfer.phpundefined
@@ -0,0 +1,26 @@
+class FileTransfer {

+++ b/core/modules/update/tests/modules/update_test/lib/Drupal/update_test/FileTransfer.phpundefined
@@ -0,0 +1,36 @@
+class FileTransfer {

Maybe "MockFileTransfer" or something like that? Would also match the comment then. Otherwise this is once again an extremely generic class name, which is already used: http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21FileTransf...

+++ b/core/modules/system/tests/modules/system_test/system_test.moduleundefined
@@ -370,32 +370,13 @@ function system_test_filetransfer_info() {
-class SystemTestFileTransfer {
-  public static function factory() {
-    return new SystemTestFileTransfer;

+++ b/core/modules/update/tests/modules/update_test/lib/Drupal/update_test/FileTransfer.phpundefined
@@ -0,0 +1,36 @@
+   * Returns an UpdateTestFileTransfer object.
+   *
+   * @return
+   *   A new UpdateTestFileTransfer object.

Should use fully qualified class name, also specifiy the type for @return, the one in system is missing documentation.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
6.18 KB
aspilicious’s picture

This one is better

RobLoach’s picture

+++ b/core/modules/system/tests/modules/system_test/lib/Drupal/system_test/MockFileTransfer.phpundefined
@@ -0,0 +1,36 @@
+/**
+ * Mock FileTransfer object to test the settings form functionality.
+ */
+class MockFileTransfer {

MockFileTransfer class.

+++ b/core/modules/update/tests/modules/update_test/lib/Drupal/update_test/MockFileTransfer.phpundefined
@@ -0,0 +1,36 @@
+namespace Drupal\update_test;
+
+/**
+ * Mocks a FileTransfer object to test the settings form functionality.
+ */
+class MockFileTransfer {

MockFileTransfer class, round two :-) . Do we really have two different classes named the same in two different modules? I understand it doesn't actually cause a problem, but we could rename one of the Update module one to UpdateMockFileTransfer, or something.

aspilicious’s picture

They do exactly the same thing (just a different title). Isn't it normal they have the same name than? I don't have any preferences...

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, they're in different namespaces, so there arn't any class name conflict anyway. It's good to clean up the dependency here as well. Having update_test depend on system_test module is not the correct way around this, what is in the patch here is the correct way around it.

catch’s picture

b/core/modules/system/tests/modules/system_test/lib/Drupal/system_test/MockFileTransfer.php

Really? Can we think of somewhere else to put these?

RobLoach’s picture

The class was in the system_test.module itself before. Seems like the logical place to put it is in system_test's namespace. Have any other ideas on where it could go?

RobLoach’s picture

Issue summary: View changes

Updating the list

webchick’s picture

Status: Reviewed & tested by the community » Fixed

All the other RTBC PSR-0 patches came in, so applied and committed this patch. YAY.

Committed and pushed to 8.x. WOOHOO.

catch’s picture

Status: Fixed » Active

We've still got to actually remove the files[] entry from system.info when the last tests have landed. Re-opening for that.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
880 bytes

Cleaning up the .info file, but it shows that we still have two tests to convert?

aspilicious’s picture

Berdir’s picture

Yes, registry.test is removed in the registry removal patch and symfony.test makes no sense anymore, because when PSR-0 wouldn't work then we couldn't execute any tests in the first place, as discussed in the issue link in #50.

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

Anonymous’s picture

Issue summary: View changes

Update