Add @internal to automated tests classes

I will be working on this.

Added the @internal tag to all concrete tests classes that were not designed for extending from contrib and custom modules.
Covered the test suites loaders, too.

Half of the patch work was done by Nevena Kostova.

@ndobromirov and @nevenakostova thanks for the work on this!

Looking good so far.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbacksTest.php
   @@ -8,6 +8,7 @@
     * @group Ajax
   + * @internal
   

   Nit: I am not positive about this but all the tags I see in php docblocks have spaces between different types of tags. So here that would call for a space between @group and @internal lines.

2. the test failure in #4 is just because \Drupal\FunctionalTests\GetTestMethodCallerExtendsTest is dependent on the line number of the calling line of method so adding the @internal just pushes the line down 1. Weird.
3. All the files in this patch are under /core/tests. This should cover all tests under /core.
   Individual modules also have tests.
   There are also tests in profiles /core/profiles (I just learned that 😜)
   I think that is it but probably want to look for all *Test.php files.

Added @internal to all Test classes in *Test.php files under drupal/core, except those 6 that already had them, using the uploaded Perl script (docblock_insert.txt). Changed 2888 files.

Additional changes were made to two tests that would have otherwise failed due to line number assertions:

• ./core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php
• ./tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php

I ran tests in my local environment, but many failed with or without the patch, evidently due to a problem in my local setup.

Egg on my face. Patch in #9 did not have the changes to the two GetTestMethodCaller*Test.php files. This one does.

Patch was made against a fresh pull of 8.5.x dev, no other patch. Yes, a small change to a whole lot of files!
\

Submitting revised patch with docblock_insert script run against updated pull of 8.5.x. The patch does not need to be combined with previous patches.

3038 files were changed.

Applying script

The script does not search the directory structure but operates on the files listed on the command line. I did the following to run the script:

# In drupal directory on clean, up-to-date, 8.5.x branch,
# copy dockblock_insert.txt to docblock_insert in local directory
# and change permission to executable. May need to change Perl path.

# get usage
./docblock_insert

# usage: docblock_insert [options] file . . .
#
# Insert tag at the end of each docblocks that precede statements matching
# a defined pattern in the given list of files. Each file is modified in place,
# so be sure to have a committed files to the repository (or have other backup)
# before running.
#
# OPTIONS:
# --help # display this message
# --dryrun # don't modify, just print to screen
# --statement # default is '^\s* class\s \w+Test\b'
# --tag # default is '@internal'
#
# NOTES:
# To avoid duplication, docblocks with existing tag are ignored.
# Whitespace in statement is ignored (but quote on command line).
#
# EXAMPLE:
# docblock_insert --statement '^\s* private' --tag '@internal' file1.php file2.php

# checkout branch for patch
git checkout -b patch-2912642

# find *Test.php files under core, sort and save list
find core -name \*Test.php |sort |tee to_patch

# run patch script (options here are default)
./docblock_insert --statement '^\s* class\s \w+Test\b' --tag '@internal' `cat to_patch`

# run git diff to find list of modified files
git diff |grep ^diff |awk '{print $NF}' |sed -e 's/^b.//' |sort |tee patched

# compare to_patch and patched files
diff to_patch patched

803d802
< core/modules/language/tests/language_elements_test/src/Form/LanguageConfigurationElementTest.php
911d909
< core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/Test.php
1712,1713d1709
< core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/LocalTaskTest.php
< core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
1719d1714
< core/modules/system/tests/src/Functional/Cache/ClearTest.php
2252d2246
< core/modules/views/tests/src/Kernel/ModuleTest.php

# get not patched list and see why not
diff to_patch patched |grep '^<' |awk '{print $2}' |tee not_patched

*Test.php files not modified

• core/modules/language/tests/language_elements_test/src/Form/LanguageConfigurationElementTest.php
• core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/Test.php
• core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/LocalTaskTest.php
• core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
• core/modules/system/tests/src/Functional/Cache/ClearTest.php
• core/modules/views/tests/src/Kernel/ModuleTest.php

Only one of these (the first) already had an @internal tag. The others were missed either because a docblock was missing or improperly places or because the class name was simply "Test." As it is, the script doesn't handle that.

Manual changes

As previously noted, two files were modified by hand for tests to pass—the tests have an explicit line no. check, which needed updating:

• core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php
• core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php

@jfmacdonald thanks for this massive patch! 👏

I started to review this patch by using Regex searches to figure out if it only changes the necessary files. Here is where I got.

I am listing it in this format:
"[REGEX EXPRESSION] = [RESULT COUNT] -> what it proves.
Hopefully this will help who ever ends up reviewing this patch further.

"diff \-\-git a" = 3038 -> files Changed
"diff \-\-git a/core/.*Test\.php b/core/.*Test.php" = 3038 -> all files changed ended in "Test.php"
By directory
"diff \-\-git a/core/modules/" = 2384
"diff \-\-git a/core/tests/" = 650
"diff \-\-git a/core/profiles/" = 4
2384 + 650 + 4 = 3038 -> No changes outside these directories

"^\+ \* @internal$" = 3038 -> needed changed added to all changed files. No extra space at the end of the lines.
"\+ \*$" = 3038 -> needed spacing line. No extra space at the end of the lines.
"^\-[ ]" = 2 - total lines removed in patch. Need for change to test line numbers

diff --git a/core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php b/core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php
index 81ebb813cd..28edf00aac 100644
--- a/core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php
+++ b/core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php
@@ -8,6 +8,8 @@
  * Test for BrowserTestBase::getTestMethodCaller() in child classes.
  *
  * @group browsertestbase
+ *
+ * @internal
  */
 class GetTestMethodCallerExtendsTest extends GetTestMethodCallerTest {
 
@@ -18,7 +20,7 @@ public function testGetTestMethodCallerChildClass() {
     $method_caller = $this->getTestMethodCaller();
     $expected = [
       'file' => __FILE__,
-      'line' => 18,
+      'line' => 20,
       'function' => __CLASS__ . '->' . __FUNCTION__ . '()',
       'class' => BrowserTestBase::class,
       'object' => $this,

diff --git a/core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php b/core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php
index f40ff20e92..0ddfcf5d7b 100644
--- a/core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php
+++ b/core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php
@@ -8,6 +8,8 @@
  * Explicit test for BrowserTestBase::getTestMethodCaller().
  *
  * @group browsertestbase
+ *
+ * @internal
  */
 class GetTestMethodCallerTest extends BrowserTestBase {
 
@@ -18,7 +20,7 @@ public function testGetTestMethodCaller() {
     $method_caller = $this->getTestMethodCaller();
     $expected = [
       'file' => __FILE__,
-      'line' => 18,
+      'line' => 20,
       'function' => __CLASS__ . '->' . __FUNCTION__ . '()',
       'class' => BrowserTestBase::class,
       'object' => $this,

At this point though I tried to apply the patch but because there have been test conversion issues committed since this it no longer applies. Such as this issue #2927766: Update ResponseGeneratorTest to use the BrowserTestBase base class instead of the deprecated RESTTestBase

If you can provide an updated patch feel free to ping me on Drupal Slack and I will review again.

I talked to @jfmacdonald about the regex and he helped me find a way to find all lines add (start with "+" but don't start with "+++" or " *"

"^\+\s*[^* +]" in the current patch this only finds the 2 expected lines mentioned in #19 which is good.

"^\-\s*[^* -]" finds the same lines but for removals

Will run these next time I review the updated patch.

Submitted revised patch "add_internal_to-2912642-21.patch" run against updated 8.5.x code base. With this, there are 3055 core/.../*Test.php files, 3051 of which have been patched. This time I've included those cases where the test class is simply "Test." I'm also uploading the revised script that handles that.

Of the four test files that were not patched, one already had the @internal tag. The other three have incorrect doc blocks. (I'll file a separate issue for those.)

"diff \-\-git a" = 3051 -> files Changed
"diff \-\-git a/core/.*Test\.php b/core/.*Test.php" = 3051 -> all files changed ended in "Test.php"
By directory
"diff \-\-git a/core/modules/" = 2396
"diff \-\-git a/core/tests/" = 651
"diff \-\-git a/core/profiles/" = 4
2396 + 651 + 4 = 3051 -> No changes outside these directories

"^\+ \* @internal$" = 3051 -> needed changed added to all changed files. No extra space at the end of the lines.
"\+ \*$" = 3051 -> needed spacing line. No extra space at the end of the lines.
"^\-[ ]" = 2 - total lines removed in patch. Need for change to test line numbers see #19

"^\+\s*[^* +]" = 2 -> all lines that start with "+"(added lines) and have zero or spaces then a character that is not " ", "+", or "*" only finds the 2 expected lines mentioned in #19 which is good.
"^\-\s*[^* -]" = 2 -> finds the same lines but for removals

Number test files total in bash "find . -name "*Test.php" | wc -l" = 3055 as @jfmacdonald mentioned in #21

Of the four test files that were not patched, one already had the @internal tag. The other three have incorrect doc blocks. (I'll file a separate issue for those.)

@jfmacdonald if you provide the names of the those file that would be great!

Otherwise I think the patch look good and is ready for RTBC!

The following core test files were not modified. The first already has @internal, the remainder have missing or improperly placed doc blocks.

• core/modules/language/tests/language_elements_test/src/Form/LanguageConfigurationElementTest.php
• core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/LocalTaskTest.php
• core/modules/system/tests/src/Functional/Cache/ClearTest.php
• core/modules/views/tests/src/Kernel/ModuleTest.php

core/modules/language/tests/language_elements_test/src/Form/LanguageConfigurationElementTest.php confirmed already has @internal

core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/LocalTaskTest.php: Has no doc block and can be updated manually in this patch.

The other 2 have doc blocks above the use statements. Let's see if we can just move those doc blocks in this patch.

If we get push back we can take it out.

I would suggest working on a branch, running the branch, do a commit, then manually updating the 3 needed files.

Looking at core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/LocalTaskTest.php from #24 this brings up that this isn't actually a test. It is for a module that supports a test.

Should include this test modules? I assume this issue doesn't handle that but I am not sure.

If we shouldn't include them this patch does include some
"\+\+\+ b/core/modules.*/tests/modules" = 28

If we should include them then that would mean we should be looking for any class under "\+\+\+ b/core/modules.*/tests/modules" not just ones that end *Test.php

Tricky 🙁

I would say we don't include them in this patch and remove the 28 files from this patch

To be clearer I think these are the folder patterns we do want:
/core/tests/Drupal/*
/core/profiles/*/tests/src/*
/core/modules/*/tests/src/*
/core/modules/*/src/Tests/*

I think the current patch with the restrictions for just those directories.

It doesn't look like using folder, file, and class name patterns are a reliable way to determine which is a test which is not. In particular, these don't seem to be tests:

core/modules/config/tests/config_test/src/Entity/ConfigTest.php
core/modules/language/tests/language_elements_test/src/Form/LanguageConfigurationElementTest.php
core/modules/language/tests/language_test/src/Entity/NoLanguageEntityTest.php
core/modules/language/tests/language_test/src/Plugin/LanguageNegotiation/LanguageNegotiationTest.php
core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFastFileStorageTest.php
core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php

while these do:

core/modules/ckeditor/tests/modules/src/Kernel/CKEditorPluginManagerTest.php
core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php

A better approach may be to follow the requirements for test auto-detection, taking care to cover both SimpleTest and PHPUnit tests: https://www.drupal.org/docs/8/phpunit/phpunit-file-structure-namespace-a....

core/modules/config/tests/config_test/src/Entity/ConfigTest.php
core/modules/language/tests/language_elements_test/src/Form/LanguageConfigurationElementTest.php
core/modules/language/tests/language_test/src/Entity/NoLanguageEntityTest.php
core/modules/language/tests/language_test/src/Plugin/LanguageNegotiation/LanguageNegotiationTest.php

These don't fit any of the patterns. They don't match /core/modules/*/tests/src/* because they don't have /tests/src/

core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFastFileStorageTest.php
core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php

Are actually tests. If you follow there inheritance up you will see they both extend \PHPUnit\Framework\TestCase
Though the class files themselves don't have public testX() methods the classes they extend do so these methods are on classes that extend them also.
For instance MTimeProtectedFileStorageTest extends MTimeProtectedFileStorageBase which has pubic test methods.

Will I think the above would work...

A better approach may be to follow the requirements for test auto-detection, taking care to cover both SimpleTest and PHPUnit tests:

I have not looked at that but it may be better.

I found a handful of files that *appear* to be tests but don't match the file patterns:

core/modules/ckeditor/tests/modules/src/Kernel/CKEditorPluginManagerTest.php
core/modules/ckeditor/tests/modules/src/Kernel/CKEditorTest.php
core/profiles/testing/modules/drupal_system_listing_compatible_test/src/Tests/SystemListingCompatibleTest.php
core/tests/fixtures/BrowserMissingDependentModuleMethodTest.php
core/tests/fixtures/BrowserMissingDependentModuleTest.php
core/tests/fixtures/KernelMissingDependentModuleMethodTest.php
core/tests/fixtures/KernelMissingDependentModuleTest.php

However, I didn't find tests in Configuration > Development > Testing for any but one: Tests/SystemListingCompatibleTest.php

I'll try scripting detecting based on the auto-test-detection criteria and see how that works. A simple check for the @group directive in the docblock finds only 3023 of 3055 core/*Test.php files (in commit 4204b0b29a73), and that includes the above 7. Also, the first two in the list above are the only *Test.php files with paths matching .*/modules/.*/modules.

Interesting those files in #30 aren't actually tested in core in seems.
It is the output of the latest core test log: https://dispatcher.drupalci.org/job/php7_mysql5.5/4463/consoleFull

I didn't find the tests there. I wonder if these files are just in the wrong folders.

.....
Actually
core/profiles/testing/modules/drupal_system_listing_compatible_test/src/Tests/SystemListingCompatibleTest.php
core/tests/fixtures/BrowserMissingDependentModuleMethodTest.php
core/tests/fixtures/BrowserMissingDependentModuleTest.php
core/tests/fixtures/KernelMissingDependentModuleMethodTest.php
core/tests/fixtures/KernelMissingDependentModuleTest.php

All seem to classes used to test the testing system. I think they could be added @internal here or in follow up. I think this issue is not for adding @internal to all test modules though they that probably need to happen. So I think these fall into the same category.

So that just leaves CkEditor tests. I will fail an issue.

Created #2929464: Tests under "core/modules/ckeditor/tests/modules/src/Kernel" are in the wrong folder and do not get tested for the CkEditorTests mentioned in #30

@jfmacdonald did you just find those classes you mentioned by chance or did you have an automated way for finding them?

@tedbow Not by chance. The ckeditor tests in the wrong folder match the (too) simple *Test.php name pattern that I started with, didn't match the refined pattern list, but had a @group directive in their docblock.

This patch only touches *Test.php files under the following directories. 3026 files:

core/tests/Drupal
core/profiles/*/tests/src
core/modules/*/tests/src
core/modules/*/src/Tests

The uploaded Perl script test_block_insert.txt checks for @group in the docblock, and they all did.

The following files required hand modification.

Change hard-coded line 18 to line 20:

core/tests/Drupal/FunctionalTests/GetTestMethodCallerExtendsTest.php
core/tests/Drupal/FunctionalTests/GetTestMethodCallerTest.php

Moved docblock from before use statements to before class statement:

core/modules/system/tests/src/Functional/Cache/ClearTest.php
core/modules/views/tests/src/Kernel/ModuleTest.php

I tested the above four in my local, and they all passed.

John

Looking except 1 thing. I might be worth just fixing this manually after you run the script.

 /**
  * Stub class for BaseFieldDefinition.
+ *
+
+ * @internal
+
  */

This is in a/core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php in extra class in the file. It is adding empty lines.

Otherwise I think it looks good.

Alright, maybe this time. :-) Corrected a bug in the Perl script. Same hand modified as previously noted, but now we have a new commit with two new files -- our ckeditor friends.

"diff \-\-git a" = 3028 -> files Changed - this seems correct because #2929464: Tests under "core/modules/ckeditor/tests/modules/src/Kernel" are in the wrong folder and do not get tested was just committed which adds 2 more tests
"diff \-\-git a/core/.*Test\.php b/core/.*Test.php" = 3028 -> all files changed ended in "Test.php"
By directory
"diff \-\-git a/core/modules/.*/tests/src" = 2160
"diff \-\-git a/core/modules/.*/src/Tests" = 217
"diff \-\-git a/core/tests/Drupal" = 648
"diff \-\-git a/core/profiles/.*/tests/src" = 3
2160 + 217 + 648 + 3 = 3028 -> Matches total files changes -> No changes outside these directories

"^\+ \* @internal$" = 3030 -> needed changed added to all changed files. No extra space at the end of the lines.
Why more than 3028? -> b/c some files have multiple classes!
Namely, core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php which has 3 classes in the file.
"\+ \*$" = 3027 -> needed spacing line. Likely no extra space need on some files. No extra space at the end of the lines.
"^\-[^-]" = 9 - total lines removed in patch.

• 2 lines in core/modules/system/tests/src/Functional/Cache/ClearTest.php to fix incorrect position of "use" statement in relation to class docblock
• 5 lines in core/modules/views/tests/src/Kernel/ModuleTest.php to fix incorrect position of class docblock in relation use statements
• 2 lines Need for change to test line numbers see #19

"^\+\s*[^* +]" = 5 -> all lines that start with "+"(added lines) and have zero or spaces then a character that is not " ", "+", or "*" 2 are expected lines mentioned in #19 which is good. the other 3 are related to removed lines mentioned above.
"^\-\s*[^* -]" = 2 -> finds the same lines but for removals

I think this looks great.

RTBC🎉

@jfmacdonald thanks for sticking through with this!

Patch failed because of DrupalCi memory problem. Retesting.

As the author of http://wimleers.com/talk/bc-burden-benefit, I say yay!

DrupalCI problem again.

Chatted with @mixologic and there are know issues right.

putting back to RTBC

#2760167: Add \Drupal\Core\Messenger\Messenger was committed and added a new test.

I think this is the only new Test.php file.

So this patch should still apply but it does miss 1 new test.

The patch here probably no longer applies to Drupal 8.7.x and needs a re-roll.

Gvert and I will be looking in to this at DrupalEurope

Re-rolled #49 patch to 8.7.x-dev.

It was green. Needs reroll.

This is VERY complex to maintain green, mainly because of the patch size...

Maybe split the patch by test groups or components?

I'm removing the novice tag from the issue as I agree with @andypost that we should split the issue and figure out the best way to split the issue into more manageable chunks.

Agreed that this is best done in smaller, scoped, scripted segments, ideally during a beta phase. Let's make this a sub-meta.

Even the previously-proposed regexes are out of date here FWIW; core's test architecture has changed a lot at this point.

Surely this is not major priority, in fact I'd say it's not worth doing at all.