Motivation

Noticed this when adding a test to another issue. There are numerous test classes in core that extend DrupalWebTestCase (WebTestBase) but are labeled as unit tests. [TestCase might be a typo, TestBase is simpletest.]

http://drupal.org/node/325974 is Drupal SimpleTest coding standards

Proposed Resolution

Simply rename all these classes.

Remaining Tasks

  • check that renamed classes are not referenced in other places.
  • see if more have creeped in since the patch was made a while ago

Follow-up Issues

Related Issues

Original summary by @xjm Feb 12 2012

I noticed this when adding a test to another issue. There are numerous test classes in core that extend DrupalWebTestCase but are labeled as unit tests. Attached patch simply renames all these classes. (I thought I saw an issue for this previously, but I guess it must have just been for a specific module.)

Files: 
CommentFileSizeAuthor
#81 multiple-web-test-classes-mislabeled-1446366-81.patch29 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,496 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#80 multiple-web-test-classes-mislabeled-1446366-79.patch29.04 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,564 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#77 multiple-web-test-classes-mislabeled-1446366-77.patch29.06 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,180 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#71 multiple-web-test-classes-mislabeled-1446366-71.patch28.77 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch multiple-web-test-classes-mislabeled-1446366-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#69 multiple-web-test-classes-mislabeled-1446366-69.patch28.45 KBbabruix
PASSED: [[SimpleTest]]: [MySQL] 58,540 pass(es).
[ View ]
#65 multiple-web-test-classes-mislabeled-1446366-65.patch20.25 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch multiple-web-test-classes-mislabeled-1446366-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#65 interdiff-63-65.txt2.28 KBYesCT
#64 multiple-web-test-classes-mislabeled-1446366-63.patch20.61 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 55,581 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#64 interdiff-62-63.txt472 bytesYesCT
#62 multiple-web-test-classes-mislabeled-1446366-62.patch21.07 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#62 interdiff-54-62.txt1.9 KBYesCT
#54 multiple-web-test-classes-mislabeled-1446366-54.patch22.5 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#48 multiple-web-test-classes-mislabeled-1446366-48.patch198.89 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#45 multiple-web-test-classes-mislabeled-1446366-45.patch175.66 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#45 interdiff.txt14.54 KBbabruix
#36 multiple-web-test-classes-mislabeled-1446366-36.patch29.67 KBbetoscopio
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch multiple-web-test-classes-mislabeled-1446366-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#36 interdiff-2.txt20.01 KBbetoscopio
#35 multiple-web-test-classes-mislabeled-1446366-35.patch22.26 KBbetoscopio
PASSED: [[SimpleTest]]: [MySQL] 48,176 pass(es).
[ View ]
#35 interdiff.txt9.77 KBbetoscopio
#27 multiple-web-test-classes-mislabeled-1446366-27.patch22.26 KBbetoscopio
PASSED: [[SimpleTest]]: [MySQL] 48,124 pass(es).
[ View ]
#27 interdiff.txt8.01 KBbetoscopio
#23 interdiff.txt612 bytesbetoscopio
#23 multiple-web-test-classes-mislabeled-1446366-23.patch22.26 KBbetoscopio
PASSED: [[SimpleTest]]: [MySQL] 48,050 pass(es).
[ View ]
#20 multiple-web-test-classes-mislabeled-1446366-20.patch22.26 KBbetoscopio
PASSED: [[SimpleTest]]: [MySQL] 47,564 pass(es).
[ View ]
#15 multiple-web-test-classes-mislabeled-1446366-15.patch173.68 KBbetoscopio
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#3 1446366-3.patch5.3 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1446366-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
not_unit_tests.patch5.29 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Active» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1446366-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Without duplicating the name of an existing class.

Status:Needs review» Reviewed & tested by the community

Works as expected and renames the tests to follow the coding standards (http://drupal.org/node/325974).

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x and 7.x. Thanks!

Status:Fixed» Postponed

Oops. I spoke too fast!

This patch actually conflicted with #1270608: File-based configuration API, and since that one took approximately a year to become RTBC and this one took approximately a week to become RTBC, I decided to temporarily roll this one back from 8.x. :)

I fully support the postponing of this issue.

Status:Postponed» Needs work

Most test class names should have been cleaned up by the PSR-0 conversion, but it wouldn't hurt to double-check it once more.

Issue tags:+Novice

Assigned:Unassigned» betoscopio

Patch on #3 don't apply, now I'm looking for the changes to rewrite the patch.

Status:Needs work» Needs review
Issue tags:-Novice, -needs backport to D7

#3: 1446366-3.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, 1446366-3.patch, failed testing.

Well, I've found a class mislabeled as unit test.

/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php:class LanguageBrowserDetectionUnitTest extends WebTestBase {

Also I've found some classes with names not finishing its name as *Test, I need some confirmation here to know if is wrong named.

modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php:class LocaleJavascriptTranslation extends WebTestBase {
modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.php:class LocaleFileImportStatus extends WebTestBase {
modules/system/lib/Drupal/system/Tests/Module/ModuleEnable.php:class ModuleEnable extends WebTestBase {
modules/config/lib/Drupal/config/Tests/LocaleConfigOverride.php:class LocaleConfigOverride extends WebTestBase {
modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.php:class UserAccountLinksTests extends WebTestBase {
modules/user/lib/Drupal/user/Tests/UserBlocksTests.php:class UserBlocksTests extends WebTestBase {

About subclasses names of WebTestBase this are not finishing with *Test:

/core/modules/views/lib/Drupal/views/Tests/User/RelationshipRepresentativeNode.php:class RelationshipRepresentativeNode extends UserTestBase {
/core/modules/views/lib/Drupal/views/Tests/Comment/DefaultViewRecentComments.php:class DefaultViewRecentComments extends ViewTestBase {
/core/modules/views/lib/Drupal/views/Tests/Taxonomy/RelationshipRepresentativeNode.php:class RelationshipRepresentativeNode extends TaxonomyTestBase {

Also, I've found unit tests mislabeled as functional tests (I'm not sure if post this here or open a new issue for it):

/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.php:class BreakpointMediaQueryTest extends UnitTestBase {
/core/modules/views/lib/Drupal/views/Tests/PluginTypeListTest.php:class PluginTypeListTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/ControllerResolverTest.php:class ControllerResolverTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/FirstEntryFinalMatcherTest.php:class FirstEntryFinalMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/NestedMatcherTest.php:class NestedMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php:class PathMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.php:class RouteTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/ChainMatcherTest.php:class ChainMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php:class MatcherDumperTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/HttpMethodMatcherTest.php:class HttpMethodMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php:class ColorTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.php:abstract class StorageTestBase extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/GarbageCollectionTest.php:class GarbageCollectionTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:class DrupalKernelTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Cache/NullBackendTest.php:class NullBackendTest extends UnitTestBase {
/core/modules/search/lib/Drupal/search/Tests/SearchExpressionInsertExtractTest.php:class SearchExpressionInsertExtractTest extends UnitTestBase {
/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php:class TempStoreDatabaseTest extends UnitTestBase {

Any sugestion is welcome.

/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php:class LanguageBrowserDetectionUnitTest extends WebTestBase {

Yep, let's rename this one to LanguageBrowserDetectionTest if that classname is available.

modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php:class LocaleJavascriptTranslation extends WebTestBase {
modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.php:class LocaleFileImportStatus extends WebTestBase {
modules/system/lib/Drupal/system/Tests/Module/ModuleEnable.php:class ModuleEnable extends WebTestBase {
modules/config/lib/Drupal/config/Tests/LocaleConfigOverride.php:class LocaleConfigOverride extends WebTestBase {
modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.php:class UserAccountLinksTests extends WebTestBase {
modules/user/lib/Drupal/user/Tests/UserBlocksTests.php:class UserBlocksTests extends WebTestBase {

I'd also agree on renaming these to end in Test (so long as they are not base classes; double-check to ensure each has some test methods on it and is not extended by other tests in the module).

/core/modules/views/lib/Drupal/views/Tests/User/RelationshipRepresentativeNode.php:class RelationshipRepresentativeNode extends UserTestBase {
/core/modules/views/lib/Drupal/views/Tests/Comment/DefaultViewRecentComments.php:class DefaultViewRecentComments extends ViewTestBase {
/core/modules/views/lib/Drupal/views/Tests/Taxonomy/RelationshipRepresentativeNode.php:class RelationshipRepresentativeNode extends TaxonomyTestBase {

Yep, agreed on these as well.

Also, I've found unit tests mislabeled as functional tests (I'm not sure if post this here or open a new issue for it):

I think we can probably do that in this issue as well.

Status:Needs work» Needs review
StatusFileSize
new173.68 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Well, finally here is my first patch, I have renamed almost all the classes mentioned before with some exceptions.
- All classes mentioned in #14 has been renamed.
- Some of the suggested UnitTest classes have been renamed

/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.php:class BreakpointMediaQueryTest extends UnitTestBase {
/core/modules/views/lib/Drupal/views/Tests/PluginTypeListTest.php:class PluginTypeListTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/ControllerResolverTest.php:class ControllerResolverTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/FirstEntryFinalMatcherTest.php:class FirstEntryFinalMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/NestedMatcherTest.php:class NestedMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php:class PathMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.php:class RouteTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/ChainMatcherTest.php:class ChainMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php:class MatcherDumperTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Routing/HttpMethodMatcherTest.php:class HttpMethodMatcherTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php:class ColorTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/Cache/NullBackendTest.php:class NullBackendTest extends UnitTestBase {
/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php:class TempStoreDatabaseTest extends UnitTestBase {

/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.php:abstract class StorageTestBase extends UnitTestBase {

Has subclasses so it hasn't been renamed.
These are the subclasses (not renamed):
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageTest.php:class
DatabaseStorageTest extends StorageTestBase {
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.php:class
DatabaseStorageExpirableTest extends StorageTestBase {
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/MemoryStorageTest.php:class
MemoryStorageTest extends StorageTestBase {

These other classes haven't been renamed:

/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/GarbageCollectionTest.php:class GarbageCollectionTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:class DrupalKernelTest extends UnitTestBase {
/core/modules/search/lib/Drupal/search/Tests/SearchExpressionInsertExtractTest.php:class SearchExpressionInsertExtractTest extends UnitTestBase {

I haven't renamed these ones because I have some conceptual doubts here. According to http://drupal.org/simpletest

- Functional tests are most common. They create a fresh database installation and specifically create data for the test in the database and then make assertions based on expected results.
- Unit tests work without a database installation in the backend and are useful for isolated functions that don't make assumptions about the larger system.

Some of these mentioned tests make use of the database. Please help me to understand this to rename (or not) these classes.

Status:Needs review» Needs work
Issue tags:-Novice, -needs backport to D7

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-15.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-15.patch, failed testing.

+++ b/.htaccessundefined
@@ -91,7 +91,7 @@ DirectoryIndex index.php index.html index.htm
-  # RewriteBase /drupal
+  RewriteBase /drupal8

This change needs to be reverted.

Also, add the following to your .gitconfig:

[diff]
  renames = copies

That way the patch will be much easier to review. :) Edit: see http://drupal.org/node/1542048 for details.

Title:Multiple web test classes mislabeled as unit testsMultiple web test classes mislabeled as unit tests and some unit tests not labeled as that
Status:Needs work» Needs review
StatusFileSize
new22.26 KB
PASSED: [[SimpleTest]]: [MySQL] 47,564 pass(es).
[ View ]

Thanks xjm for your help, I'm submiting a corrected version of the patch, the changes applied are the same explained in #15.

I reviewed all the changes here and they look correct to me.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/HttpMethodMatcherUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Routing\HttpMethodMMatcherTest.
+ * Definition of Drupal\system\Tests\Routing\HttpMethodMMatcherUnitTest.

Looks like there's an extra M here (both before and after the change). Let's fix that. If you could reroll with an interdiff for that change, I think this is RTBC. Great work!

Hi xjm, yes I can reroll this patch, I will submit the changes soon, but what do you mean with 'reroll with an interdiff for that change'? Could you give me some extra explanation or resource where to learn about it?.

Other name changes that could be included in this patch are:

/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageTest.php:class
DatabaseStorageTest extends StorageTestBase {
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.php:class
DatabaseStorageExpirableTest extends StorageTestBase {
/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/MemoryStorageTest.php:class
MemoryStorageTest extends StorageTestBase {
Those are subclasses of a UnitTest.

And these others classes, I'm not sure if should be renamed or not.:

/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/GarbageCollectionTest.php:class GarbageCollectionTest extends UnitTestBase {
/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php:class DrupalKernelTest extends UnitTestBase {
/core/modules/search/lib/Drupal/search/Tests/SearchExpressionInsertExtractTest.php:class SearchExpressionInsertExtractTest extends UnitTestBase {

Please tell me your opinion if we should include this too or not.
Thanks for your help.

StatusFileSize
new22.26 KB
PASSED: [[SimpleTest]]: [MySQL] 48,047 pass(es).
[ View ]
new22.26 KB
PASSED: [[SimpleTest]]: [MySQL] 48,050 pass(es).
[ View ]
new612 bytes

Ok, here is a revisited version with an interdiff extra.
For the record, I found all the necesary explanations in your excelent article about interdiffs http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-... , comments there are very helpful too.

oops my mistake, ... the last comment had the same patch twice, I forgot to delete the old version of the attached patch when I renamed it according with the comment number.

Issue tags:-Novice

Yeah, I think the tests in #23 could be renamed as well. Maybe StorageTestBase should also be StorageUnitTestBase.

Issue tags:+Novice

StatusFileSize
new8.01 KB
new22.26 KB
PASSED: [[SimpleTest]]: [MySQL] 48,124 pass(es).
[ View ]

Ok, here is the orignal patch plus the interdiff wich includes changes mentioned in #21 and #22.

Status:Needs review» Reviewed & tested by the community

Thanks, looks great!

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/FirstEntryFinalMatcherUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Routing\NestedMatcherTest.
+ * Definition of Drupal\system\Tests\Routing\NestedMatcherUnitTest.
+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Routing\UrlMatcherDumperTest.
+ * Definition of Drupal\system\Tests\Routing\UrlMatcherDumperUnitTest.
+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Routing\PartialMatcherTest.
+ * Definition of Drupal\system\Tests\Routing\PartialMatcherUnitTest.

Oops, just caught this. These are not the actual names of the classes. (Probably copypaste errors in a previous patch.)

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/HttpMethodMatcherUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Routing\HttpMethodMMatcherTest.
+ * Definition of Drupal\system\Tests\Routing\HttpMethodMMatcherUnitTest.

Also the extra M snuck back in.

Most of the test classes being touched here are undergoing major changes currently.

Can we postpone this patch for ~4 weeks, please?

I'm also not sure whether we need to explicitly label a unit test as unit test. In an ideal world, almost all tests would be unit tests. Furthermore, we now have an additional DrupalUnitTestBase, and I would object to suffixing such tests with "DrupalUnitTest" - that would be ridiculous...

@sun, Well, it's in the coding standards. I think that DrupalUnitTestBase can follow the same rule as UnitTestBase, whatever that is, and currently it's FooUnitTest.

I am on the fence about postponing the issue. I appreciate the concern about feature freeze, but it's also pretty trivial to reroll patches for a renamed test class.

Then we should consider to change and correct those coding standards.

Well, I think it's still a useful naming convention. However, it does look like it might have been introduced without specific intent:
http://drupal.org/node/325974/revisions/view/2107598/2129730

In that revision, aspilicious removes the word "Web" but not "Unit."

IIRC there's a lot more tests named with FooUnitTest than this patch changes, though. Maybe @betoscopio can clarify since he's actively been going over the class names.

The original concern about this patch were some classes labeled as unit tests (only one was found) , so the first task was find those and take the word 'Unit' out of the name of the class.
In the way of searching functional tests wrong named as unit tests, other problems with the names were found, as many tests class names that don't finish its name with *Test and some others finished with *Tests.
Also some unit tests definitions not labeled as that, and I understand that is the concern about this patch. The details of this class names not following the naming convention are detailed in #13.
I think it would be necessary apply at least some of the changes (as tests not finishing with *Test, or finishing with *Tests). The patch has been divided into many commits, so only some changes could be applied into a smaller patch.
The problems found by xjm in #29 are still present without changing the names of the classes, so we need to fix that in some point.
I will be posting soon a new version of the interdiff fixing the problems presented in #29, could still be useful for later if this patch has to be rerolled.

Status:Needs work» Needs review
StatusFileSize
new9.77 KB
new22.26 KB
PASSED: [[SimpleTest]]: [MySQL] 48,176 pass(es).
[ View ]

Ok, here is the same patch with a modified version of the interdiff fixing the problems explained in #29.
Not sure, if this patch could be accepted anytime soon, but I'm posting this because could be helpful for a later reroll.

Status:Needs review» Needs work
StatusFileSize
new20.01 KB
new29.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch multiple-web-test-classes-mislabeled-1446366-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

There is a new issue with a smaller set of changes in #1847280: Rename some functional SimpleTest WebTestBase test classes acording to code standards.
Here is a new patch that removes all the changes in to the unit tests class names with its respective interdiff showing the diferences.

Status:Needs work» Needs review

Sending to the bot.

Note the tests coding standards page is out of date and could use updating #1869794: Update tests coding standards doc and make consistant with 1354 where appropriate

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-36.patch, failed testing.

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleEnableTest.phpundefined
@@ -2,7 +2,7 @@
+ * Contains Drupal\system\Tests\Module\ModuleEnableTest.
+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTest.phpundefined
@@ -2,7 +2,7 @@
+ * Definition of Drupal\user\Tests\UserAccountLinksTest.

since this patch was made, we are using Contains instead of Definition of, and also using the backslash in \Drupal\whatevers

Enforcing the Test suffix and not labeling web tests makes sense. Not so sure about labeling all unit tests as UnitTest though. Since this patch was created, we have converted tons of tests (Most config, entity and database tests) to DrupalUnitTestBase and I've generally avoided to rename them as that would just complicate the conversion.

Also not sure why this is tagged for backport, what if someone extends one those classes (e.g. to run the same tests with a different setUp()) ?

@Berdir -- Not sure what your concern is about backporting -- the test name standards are different in D7, but there are still standards. It won't be the same patch.

Also, this issue is not the place to discuss the standard. :)

Assigned:betoscopio» Unassigned

According to the changes in core as is explained in #41, this patch should be rerolled, I can't work in this until April, so I let this patch free to assing. By that time I could start to work on this again.

Status:Needs work» Needs review
StatusFileSize
new14.54 KB
new175.66 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Rerolled patch from #36, added changes from #41.

Status:Needs review» Needs work

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-45.patch, failed testing.

Please make sure git is configured to treat moves as renames:
http://drupal.org/node/1542048

Status:Needs work» Needs review
StatusFileSize
new198.89 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Status:Needs review» Needs work

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-48.patch, failed testing.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentCommentsTest.phpundefined
@@ -2,6 +2,7 @@
+ * Contains \Drupal\views\Tests\Comment\DefaultViewRecentCommentsTest.
  * Contains \Drupal\comment\Tests\Views\DefaultViewRecentComments.

the old Contains line was probably just forgotten to be deleted.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentCommentsTest.phpundefined
@@ -10,7 +11,7 @@
use Drupal\entity\DatabaseStorageController;
use  Drupal\views\Tests\ViewTestBase;

can we fix the extra whitespace after the second use while we are here cleaning this up?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouteUnitTest.phpundefined
index 0000000..c7a85a3
--- /dev/null
--- /dev/null
+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined

why is this file named form_test.module.orig? .orig?

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+ * @file
+ * Helper module for the form API tests.

for a .module, api examples: http://drupal.org/node/1918356
shows the description starting with a third person verb..

http://drupal.org/coding-standards/docs#file
says *if* it starts with a verb it should be third person.

So maybe this is ok. I checked a few .modules in core and there is not uniform pattern. core/modules/system/tests/modules/plugin_test/plugin_test.module uses that pattern. ok.

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+}
+
+
+/**

extra newline

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+ * Implements hook_form_FORM_ID_alter() on behalf of block.module.
...
+ * Implements hook_form_FORM_ID_alter().
...
+ * Implements hook_form_FORM_ID_alter() on behalf of system.module.

these should use a pattern like:
http://drupal.org/coding-standards/docs#functions

**
* Implements hook_form_FORM_ID_alter() for node_type_form().
*/
function mymodule_form_node_type_form_alter(&$form, &$form_state) {

Check for others to fix.

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+/**
+ * Form builder for testing drupal_validate_form().
+ *
+ * Serves for testing form processing and alterations by form validation
+ * handlers, especially for the case of a validation error:
+ * - form_set_value() should be able to alter submitted values in
+ *   $form_state['values'] without affecting the form element.
+ * - #element_validate handlers should be able to alter the $element in the form
+ *   structure and the alterations should be contained in the rebuilt form.
+ * - #validate handlers should be able to alter the $form and the alterations
+ *   should be contained in the rebuilt form.
+ */
+function form_test_validate_form($form, &$form_state) {
...
+/**
+ * Form validation handler for form_test_validate_form().
+ */
+function form_test_validate_form_validate(&$form, &$form_state) {

form functions and validate functions (and submit)
should have @see lines.
http://drupal.org/coding-standards/docs#functions

Check for other similar ones to fix in this file.

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+/**
+ * Form element validation handler for 'Name' field in form_test_validate_required_form().
+ */
+function form_test_validate_required_form_element_validate($element, &$form_state) {

I think these comments still need to be under 80 chars?

Maybe split this like:

* Form element validation handler for form_test_validate_required_form().
*
* Validates 'Name' field.

wait, this code does not have to do with 'Name', why is Name mentioned, because it's one example of a required form element?

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+/**
+ * Create a header and options array. Helper function for callbacks.
+ */
+function _form_test_tableselect_get_data() {

this next section of the file switches to _function_names_starting_with_underscores()
And the verb tense on the docblocks are not third person anymore. I think they still should be looking at http://drupal.org/coding-standards/docs#functions

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+ * @param $form_state
+ *   The form_state
+ * @param $element_properties
+ *   An array of element properties for the tableselect element.
+ *
+ * @return
+ *   A form with a tableselect element and a submit button.
+ */
+function _form_test_tableselect_form_builder($form, $form_state, $element_properties) {

why document $form_state, but not $form?

hook_form, we dont document form and form_state, but if they get an @param, I think it needs to have types.

I think this is not just another _form and so here we want to @param $form and $form_state.

Also, @return can get a type. Is a form just an array:
@return array

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+ * Menu callback; Invokes a form builder function with a wrapper callback.

Menu callback:
use a colon instead of a semicolon.

+++ b/core/modules/system/tests/modules/form_test/form_test.module.origundefined
@@ -0,0 +1,2468 @@
+ * @param $element

specify type. string?

========
I didn't get through the whole patch.
I'm not sure what changes are blocking and need to be done.
Also, several of these things are repeated, so search for similar patterns to fix within the file.

Oh, maybe the .orig files were included in the patch by mistake?
There is another: core/modules/user/user.module.orig

@YesCT, #48 is far too large to be the right patch. Compare to #36 which is less than 30K. I believe you are reviewing changes that are either in moved files (deleted lines in the old file location and added lines in the new location), which are out of scope, or the result of diffing a stale branch against 8.x or vice versa. The former would be likely given the goal of this issue. So, @babruix probably needs to configure git to detect renames properly, rather than including them in the diff:

[diff]
  renames = copies

(See http://drupal.org/documentation/git/configure.) Also use the commands git mv, git add, and git rm.

Oh, maybe the .orig files were included in the patch by mistake?
There is another: core/modules/user/user.module.orig

That's the filename extension that the patch utility gives to the original copy of a file when it applies a patch that has some rejects. It was probably included by mistake. There are two things to do to avoid this:

  1. Always check which files you are staging when you add them by using git status--don't blindly git add .--and use git diff --stat to see summaries of staged changes.
  2. Use a .gitignore. See http://drupal.org/documentation/git/configure for more information.

Assigned:Unassigned» YesCT

I'm going to take a stab at this.

Status:Needs work» Needs review
StatusFileSize
new22.5 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

patch still applied.
I ended up changing .gitconfig to renames = copy (but copies might have worked also)

I did:
drush am 1446366
which deleted and then added the moved files.

@tim.plunkett hinted to stage them.
so:
git add .; git add -u .

Then the git diff 8.x looked much better.

This is also without those .orig files.

Status:Needs review» Needs work

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-54.patch, failed testing.

Status:Needs work» Needs review

I thought I would grep for old names to see if they were referenced anywhere and needed to be changed.

+++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMediaQueryUnitTest.phpundefined
@@ -1,7 +1,7 @@
- * Definition of Drupal\breakpoint\Tests\BreakpointMediaQueryTest.
+ * Contains \Drupal\breakpoint\Tests\BreakpointMediaQueryUnitTest.
@@ -15,7 +15,7 @@
-class BreakpointMediaQueryTest extends UnitTestCase {
+class BreakpointMediaQueryUnitTest extends UnitTestBase {

But

grep -R BreakpointMediaQueryTest *
core/modules/simpletest/tests/Drupal/simpletest/Tests/phpunit_error.xml:    <testsuite name="Drupal\breakpoint\Tests\BreakpointMediaQueryTest" file="/home/chx/www/system/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.php" namespace="Drupal\breakpoint\Tests" fullPackage="Drupal.breakpoint.Tests" tests="0" assertions="0" failures="0" errors="0" time="0.000000"/>

[edit: added code tags so can see that ^]

/home/chx ... doesn't seem like it should be here.

Status:Needs review» Needs work

updated issue summary.
doing more checking now.

should all tests be in a lib?
http://drupal.org/node/325974 => Drupal SimpleTest coding standards

modules/[modulename]/lib/Drupal/[modulename]/Tests/[classname].php

core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMediaQueryUnitTest.php
maybe should be in
core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointMediaQueryUnitTest.php

+++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMediaQueryUnitTest.phpundefined
@@ -1,7 +1,7 @@
- * Definition of Drupal\breakpoint\Tests\BreakpointMediaQueryTest.
+ * Contains \Drupal\breakpoint\Tests\BreakpointMediaQueryUnitTest.
@@ -15,7 +15,7 @@
-class BreakpointMediaQueryTest extends UnitTestCase {
+class BreakpointMediaQueryUnitTest extends UnitTestBase {
+++ b/core/tests/Drupal/Tests/Core/Cache/NullBackendUnitTest.phpundefined
@@ -2,7 +2,8 @@
- * Definition of Drupal\Tests\Core\Cache\NullBackendTest.
+ * Contains \Drupal\system\Tests\Cache\NullBackendUnitTest.
+ * Contains \Drupal\Tests\Core\Cache\NullBackendTest.
@@ -15,7 +16,7 @@
-class NullBackendTest extends UnitTestCase {
+class NullBackendUnitTest extends UnitTestBase {

UnitTestCase is phpunit tests, so not something this patch should be changing.

Which might explain why they are in tests and not lib

Issue summary:View changes

issue summary template like and add follow-up and related issues section.

Title:Multiple web test classes mislabeled as unit tests and some unit tests not labeled as thatMultiple web test classes mislabeled as unit tests and some unit tests not labeled. simpletest ([Web|Unit|DrupalUnit]TestBase)

trying for a better title.

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new21.07 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

this undoes the changes to phpUnitTests (UnitTestCase) noted in #60

Status:Needs review» Needs work

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-62.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new472 bytes
new20.61 KB
FAILED: [[SimpleTest]]: [MySQL] 55,581 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

+++ b/core/tests/Drupal/Tests/Core/Cache/NullBackendTest.phpundefined
@@ -15,7 +15,7 @@
-class NullBackendTest extends UnitTestCase {
+class NullBackendTest extends UnitTestBase {

oops that should still be TestCase not TestBase

StatusFileSize
new2.28 KB
new20.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch multiple-web-test-classes-mislabeled-1446366-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

A.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentCommentsTest.phpundefined
@@ -2,6 +2,7 @@
+ * Contains \Drupal\views\Tests\Comment\DefaultViewRecentCommentsTest.
  * Contains \Drupal\comment\Tests\Views\DefaultViewRecentComments.
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/Views/RelationshipRepresentativeNodeTest.phpundefined
@@ -2,6 +2,7 @@
+ * Contains \Drupal\views\Tests\Taxonomy\RelationshipRepresentativeNodeTest.
  * Contains \Drupal\taxonomy\Tests\Views\RelationshipRepresentativeNode.

I see here the confusion, because its a Views test in the comment module.

I'm going with:
* Contains \Drupal\comment\Tests\Views\DefaultViewRecentCommentsTest.

B.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentCommentsTest.phpundefined
@@ -10,7 +11,7 @@
use  Drupal\views\Tests\ViewTestBase;

extra space taken out. If it's too much scope creep, let me know.

Here, the use does not have simpletest ... so only way of knowing is to look up ViewTestBase or remember Base is simpletest and Case is PHPUnit. :P

C.

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Contains Drupal\system\Tests\KeyValueStore\DatabaseStorageExpirableTest.
+ * Contains \Drupal\system\Tests\KeyValueStore\DatabaseStorageExpirableUnitTest.
@@ -12,7 +12,7 @@
-class DatabaseStorageExpirableTest extends StorageTestBase {
+class DatabaseStorageExpirableUnitTest extends StorageUnitTestBase {
+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Contains Drupal\system\Tests\KeyValueStore\DatabaseStorageTest.
+ * Contains \Drupal\system\Tests\KeyValueStore\DatabaseStorageUnitTest.
@@ -12,7 +12,7 @@
-class DatabaseStorageTest extends StorageTestBase {
+class DatabaseStorageUnitTest extends StorageUnitTestBase {
+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/MemoryStorageUnitTest.phpundefined
@@ -2,7 +2,7 @@
- * Contains Drupal\system\Tests\KeyValueStore\MemoryStorageTest.
+ * Contains \Drupal\system\Tests\KeyValueStore\MemoryStorageUnitTest.
@@ -10,7 +10,7 @@
-class MemoryStorageTest extends StorageTestBase {
+class MemoryStorageUnitTest extends StorageUnitTestBase {

These are missing a use to use StorageUnitTestBase.

I did not add that missing use.

D.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/Views/RelationshipRepresentativeNodeTest.phpundefined
@@ -10,7 +11,7 @@
-class RelationshipRepresentativeNode extends TaxonomyTestBase {
+class RelationshipRepresentativeNodeTest extends TaxonomyTestBase {

missing a use

I did not add the use.

E.

+++ b/core/vendor/symfony-cmf/routing/Symfony/Cmf/Component/Routing/Tests/NestedMatcher/NestedMatcherUnitTest.phpundefined
@@ -1,3 +1,7 @@
+ * @file
+ * Definition of Drupal\system\Tests\Routing\NestedMatcherUnitTest.
+ */
+class NestedMatcherUnitTest extends UnitTestBase {

Symfony... we should not be fixing that here.

[edit: added A. B. ... for easier reference later]

Assigned:YesCT» Unassigned

I wont be back at this till ... later.
Unassigning while I'm away.

Issue summary:View changes

try and clarify simpletest TestBase

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, multiple-web-test-classes-mislabeled-1446366-65.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,540 pass(es).
[ View ]

Rerolled patch.

Removing myself from the author field to unfollow the issue. --xjm

Issue summary:View changes
Issue tags:+Needs reroll

StatusFileSize
new28.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch multiple-web-test-classes-mislabeled-1446366-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch re-rolled.

Issue tags:-Needs reroll

Status:Needs review» Needs work

The last submitted patch, 71: multiple-web-test-classes-mislabeled-1446366-71.patch, failed testing.

Head has advanced alot since November

first bad test SearchExpressionInsertExtractUnitTest no longer exists ... retesting

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 71: multiple-web-test-classes-mislabeled-1446366-71.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,180 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Also renamed class DrupalKernelTest.php to DrupalKernelUnitTest.

Status:Needs review» Needs work

The last submitted patch, 77: multiple-web-test-classes-mislabeled-1446366-77.patch, failed testing.

Looking at exception failure from #77

Drupal\system\Tests\Routing\ControllerResolverUnitTest

when https://drupal.org/node/2042739 landed on January 8, 2014 it rewrote the test

The container aware tests that were covered by :-

  /**
   * Confirms that a container aware controller gets returned.
   */
  function testContainerAware() {
    $container = new Container();
    $resolver = new ControllerResolver($container);
    $request = Request::create('/some/path');
    $request->attributes->set('_controller', '\Drupal\system\Tests\Routing\MockController::run');
    $controller = $resolver->getController($request);
    $this->assertTrue($controller[0] instanceof MockController, 'The correct controller object was returned.');
  }

have been completely rewritten... so the conversion of ControllerResolverTest.php to ControllerResolverUnitTests.php needs to be revisited.

Status:Needs work» Needs review
StatusFileSize
new29.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,564 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

It can not find class \Drupal\Tests\Core\Controller\MockController
Class actually exists in core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php
Renaming it to ControllerResolverUnitTest.php did not help.

StatusFileSize
new29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,496 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Fixed patch.

Status:Needs review» Needs work

The last submitted patch, 81: multiple-web-test-classes-mislabeled-1446366-81.patch, failed testing.

The last submitted patch, 80: multiple-web-test-classes-mislabeled-1446366-79.patch, failed testing.