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
Beta phase evaluation
Issue category | Bug-ish task because the names/docs are incorrect https://www.drupal.org/core/issue-category |
---|---|
Issue priority | Normal/Minor(?) because not rendering the system unusable https://www.drupal.org/core/issue-priority |
Unfrozen changes | Unfrozen because it only changes automated tests. |
Disruption | Slightly disruptive, could affects the remamed test classes. |
Allowed in the beta since it only effects tests, and that is unfrozen.
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
- #1985438: Test to prevent test classes mislabeling and wrong filenames
- #1847280: Simpletest web tests misnamed as "Unit"
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.)
Comments
Comment #1
xjmComment #3
xjmWithout duplicating the name of an existing class.
Comment #4
Kjartan CreditAttribution: Kjartan commentedWorks as expected and renames the tests to follow the coding standards (http://drupal.org/node/325974).
Comment #5
webchickCommitted and pushed to 8.x and 7.x. Thanks!
Comment #6
webchickOops. 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. :)
Comment #7
xjmI fully support the postponing of this issue.
Comment #8
sunMost test class names should have been cleaned up by the PSR-0 conversion, but it wouldn't hurt to double-check it once more.
Comment #9
xjmComment #10
betoscopioPatch on #3 don't apply, now I'm looking for the changes to rewrite the patch.
Comment #11
kscheirer#3: 1446366-3.patch queued for re-testing.
Comment #13
betoscopioWell, I've found a class mislabeled as unit test.
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.
About subclasses names of WebTestBase this are not finishing with *Test:
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):
Any sugestion is welcome.
Comment #14
xjm/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.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).Yep, agreed on these as well.
I think we can probably do that in this issue as well.
Comment #15
betoscopioWell, 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
Has subclasses so it hasn't been renamed.
These are the subclasses (not renamed):
These other classes haven't been renamed:
I haven't renamed these ones because I have some conceptual doubts here. According to http://drupal.org/simpletest
Some of these mentioned tests make use of the database. Please help me to understand this to rename (or not) these classes.
Comment #17
betoscopio#15: multiple-web-test-classes-mislabeled-1446366-15.patch queued for re-testing.
Comment #19
xjmThis change needs to be reverted.
Also, add the following to your
.gitconfig
:That way the patch will be much easier to review. :) Edit: see http://drupal.org/node/1542048 for details.
Comment #20
betoscopioThanks xjm for your help, I'm submiting a corrected version of the patch, the changes applied are the same explained in #15.
Comment #21
xjmI reviewed all the changes here and they look correct to me.
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!
Comment #22
betoscopioHi 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:
Those are subclasses of a UnitTest.
And these others classes, I'm not sure if should be renamed or not.:
Please tell me your opinion if we should include this too or not.
Thanks for your help.
Comment #23
betoscopioOk, 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.
Comment #24
betoscopiooops 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.
Comment #25
xjmYeah, I think the tests in #23 could be renamed as well. Maybe
StorageTestBase
should also beStorageUnitTestBase
.Comment #26
xjmComment #27
betoscopioOk, here is the orignal patch plus the interdiff wich includes changes mentioned in #21 and #22.
Comment #28
xjmThanks, looks great!
Comment #29
xjmOops, just caught this. These are not the actual names of the classes. (Probably copypaste errors in a previous patch.)
Also the extra M snuck back in.
Comment #30
sunMost 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...
Comment #31
xjm@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.
Comment #32
sunThen we should consider to change and correct those coding standards.
Comment #33
xjmWell, 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.
Comment #34
betoscopioThe 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.
Comment #35
betoscopioOk, 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.
Comment #36
betoscopioThere is a new issue with a smaller set of changes in #1847280: Simpletest web tests misnamed as "Unit".
Here is a new patch that removes all the changes in to the unit tests class names with its respective interdiff showing the diferences.
Comment #37
xjmSending to the bot.
Comment #38
YesCT CreditAttribution: YesCT commentedNote 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
Comment #39
YesCT CreditAttribution: YesCT commented#36: multiple-web-test-classes-mislabeled-1446366-36.patch queued for re-testing.
Comment #41
YesCT CreditAttribution: YesCT commentedsince this patch was made, we are using Contains instead of Definition of, and also using the backslash in \Drupal\whatevers
Comment #42
BerdirEnforcing 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()) ?
Comment #43
xjm@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. :)
Comment #44
betoscopioAccording 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.
Comment #45
babruix CreditAttribution: babruix commentedRerolled patch from #36, added changes from #41.
Comment #47
xjmPlease make sure git is configured to treat moves as renames:
http://drupal.org/node/1542048
Comment #48
babruix CreditAttribution: babruix commentedComment #50
YesCT CreditAttribution: YesCT commentedthe old Contains line was probably just forgotten to be deleted.
can we fix the extra whitespace after the second use while we are here cleaning this up?
why is this file named form_test.module.orig? .orig?
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.
extra newline
these should use a pattern like:
http://drupal.org/coding-standards/docs#functions
Check for others to fix.
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.
I think these comments still need to be under 80 chars?
Maybe split this like:
wait, this code does not have to do with 'Name', why is Name mentioned, because it's one example of a required form element?
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
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
Menu callback:
use a colon instead of a semicolon.
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.
Comment #51
YesCT CreditAttribution: YesCT commentedOh, maybe the .orig files were included in the patch by mistake?
There is another: core/modules/user/user.module.orig
Comment #52
xjm@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:
(See http://drupal.org/documentation/git/configure.) Also use the commands
git mv
,git add
, andgit rm
.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:git status
--don't blindlygit add .
--and usegit diff --stat
to see summaries of staged changes..gitignore
. See http://drupal.org/documentation/git/configure for more information.Comment #53
YesCT CreditAttribution: YesCT commentedI'm going to take a stab at this.
Comment #54
YesCT CreditAttribution: YesCT commentedpatch 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.
Comment #56
YesCT CreditAttribution: YesCT commentedI thought I would grep for old names to see if they were referenced anywhere and needed to be changed.
But
[edit: added code tags so can see that ^]
/home/chx ... doesn't seem like it should be here.
Comment #57
YesCT CreditAttribution: YesCT commentedComment #58
YesCT CreditAttribution: YesCT commentedupdated issue summary.
doing more checking now.
Comment #59
YesCT CreditAttribution: YesCT commentedshould 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
Comment #60
YesCT CreditAttribution: YesCT commentedUnitTestCase is phpunit tests, so not something this patch should be changing.
Which might explain why they are in tests and not lib
Comment #60.0
YesCT CreditAttribution: YesCT commentedissue summary template like and add follow-up and related issues section.
Comment #61
YesCT CreditAttribution: YesCT commentedtrying for a better title.
Comment #62
YesCT CreditAttribution: YesCT commentedthis undoes the changes to phpUnitTests (UnitTestCase) noted in #60
Comment #64
YesCT CreditAttribution: YesCT commentedoops that should still be TestCase not TestBase
Comment #65
YesCT CreditAttribution: YesCT commentedA.
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.
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.
These are missing a use to use StorageUnitTestBase.
I did not add that missing use.
D.
missing a use
I did not add the use.
E.
Symfony... we should not be fixing that here.
[edit: added A. B. ... for easier reference later]
Comment #66
YesCT CreditAttribution: YesCT commentedI wont be back at this till ... later.
Unassigning while I'm away.
Comment #66.0
YesCT CreditAttribution: YesCT commentedtry and clarify simpletest TestBase
Comment #67
YesCT CreditAttribution: YesCT commented#65: multiple-web-test-classes-mislabeled-1446366-65.patch queued for re-testing.
Comment #69
babruix CreditAttribution: babruix commentedRerolled patch.
Comment #69.0
babruix CreditAttribution: babruix commentedRemoving myself from the author field to unfollow the issue. --xjm
Comment #70
heddnComment #71
babruix CreditAttribution: babruix commentedPatch re-rolled.
Comment #72
babruix CreditAttribution: babruix commentedComment #74
martin107 CreditAttribution: martin107 commentedHead has advanced alot since November
first bad test SearchExpressionInsertExtractUnitTest no longer exists ... retesting
Comment #75
martin107 CreditAttribution: martin107 commented71: multiple-web-test-classes-mislabeled-1446366-71.patch queued for re-testing.
Comment #77
babruix CreditAttribution: babruix commentedAlso renamed class
DrupalKernelTest.php
toDrupalKernelUnitTest
.Comment #79
martin107 CreditAttribution: martin107 commentedLooking 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 :-
have been completely rewritten... so the conversion of ControllerResolverTest.php to ControllerResolverUnitTests.php needs to be revisited.
Comment #80
babruix CreditAttribution: babruix commentedIt 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.
Comment #81
babruix CreditAttribution: babruix commentedFixed patch.
Comment #84
YesCT CreditAttribution: YesCT commentedwe should see if this is still a problem.
if it is, I think a new patch should be made from scratch. (not rerolled)
the patch here seems to have out of scope changes in it.
Comment #85
betoscopioReviewed and is still a problem, some classes need to be renamed.
Related issue added.
Updated sumary.
Comment #86
betoscopioChanging status to active, because this is now a meta.
Changing title, label the UnitTest as 'Unit' is no longer needed.
Comment #96
bhanuprakashnani CreditAttribution: bhanuprakashnani at Google Summer of Code commentedComment #101
quietone CreditAttribution: quietone as a volunteer commentedTriaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.
This looks like it belongs in the testing component, phpunit.
Comment #102
mondrakeBoy, is this outdated... we no longer have Simpletest and have different testsuites in PHPUnit. Last patch is on very old code. If we still find misplaced tests, it'd probably be better open a new issue on its own.