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

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

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

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. :)

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.

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

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

/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.

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.

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

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

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

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:

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.

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.

Thanks, looks great!

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...

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

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.

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.

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

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

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()) ?

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.

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

the 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.

@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, 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.

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.

I 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.

updated issue summary.
doing more checking now.

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

Which might explain why they are in tests and not lib

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

oops that should still be TestCase not TestBase

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