Evaluate performance impact of limiting test discovery to *Test.php filename suffix

Framing this as performance is just the kind of insidiousness I expect. It's not about performance. It's about piss poor DX. It's not like the parent found 20 tests scattered among all of core that wouldn't be picked up. My suggestion is: namespace + annotation. Learn plugins once, reuse for tests.

Performance is a noble goal except when you are so fast you don't find the very thing you want to run.

Restoring the IS since my proposal is in a different issue already.

This seems kind of settled...

Hmm. Found a todo about this.

The developer experience problem can be solved using the following patch:
It checks for every executed test, whether the filename ends with Test.php, otherwise throws an error.

One way of fixing is to use case insensitive regex checking.

The @todo is in the docblock for Drupal\simpletest\TestDiscovery::scanDirectory().

The patch in #13 deals with PHPUnit tests only and is probably out of scope here. Tho make another issue because it's a good idea. That said, PHPUnit tests now use scanDirectory() to find test suites.

Sure, here we go: #2793701: Ensure that phpunit tests end with Test.php

#2949363: Impossible to make trigger_error in some files without test fails adds an interesting wrinkle because our test discovery system ends up loading discovered files. If a test trait has a @trigger_error(), it will trigger during test file scans, rather than test run time.

If TestDiscovery did not allow files to end with anything other than *Test.php, we wouldn't load trait files for introspection, and they could be fully deprecated. The down-side is that test files with non-standard names would be excluded, so we might get a false positive based on an ill-named test. This would not be caught by our coding standards test listener because the test file would not be discovered.

Here's a patch which does the filtering on *Test.php.

I think this code in getTestClasses() can be simplified slightly now, as every class name in this loop should end in Test already?

      catch (MissingGroupException $e) {
        // If the class name ends in Test and is not a migrate table dump.
        if (preg_match('/Test$/', $classname) && strpos($classname, 'migrate_drupal\Tests\Table') === FALSE) {
          throw $e;
        }
        // If the class is @group annotation just skip it. Most likely it is an
        // abstract class, trait or test fixture.
        continue;
      }

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -288,7 +285,7 @@ public static function scanDirectory($namespace_prefix, $path) {
       }
-      return $current->isFile() && $current->getExtension() === 'php';
+      return $current->isFile() && substr($current->getFilename(), -8) === 'Test.php';
     });

We could document a line: Ensure that files end up with Test.php to replicate the phpunit built in behaviour.

I've simplified the catch statement of getTestClasses() as requested by #26. I've adjusted the documentation to indicate the new behaviour. Group annotations, traits or test fixtures shouldn't make it to this piece of code anymore (as they'll be filtered out by the Test.php requirement). I didn't dare remove the if statement because I wasn't certain what classes would be generated for drupal_migrate (e.g. a google search helped me find a sandbox that contained a "ContentFieldTest.php" file containing the schema for a content_field_test table).

I've also added the suggestion by dawehner from #27 with regards to the comment verbatim.

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -177,12 +177,10 @@ public function getTestClasses($extension = NULL, array $types = []) {
-        // If the class is @group annotation just skip it. Most likely it is an
-        // abstract class, trait or test fixture.
         continue;

Isn't this still helpful information?

Re #30

At the time, I reasoned that these classes would no longer make it to this part of the codebase due to the new filename restrictions.

I'm not sure that at this time I can come to the same conclusion so maybe that was too much removal indeed :/

@Kingdutch
Let's add it back, as it makes for example the patch easier to review. Otherwise this looks rocksolid.

Added back the text

Thank you @Kingdutch

+++ b/core/modules/simpletest/tests/src/Traits/TestTraitDeprecationTrait.php
@@ -0,0 +1,13 @@
+trait TestTraitDeprecationTrait {

I don't really get why this is here. If it does need to be here the comments on it need to explain it. Given that the whole point of this change is to limit to *Test.php I'm not sure why we need this. Ah I see I guess it's TestDiscoveryTest - I'm not sure once we limit to 'Test.php' this is really necessary. And if it is then the trait needs an @see to the test and the test needs an @see to this thing. Personally I'd remove both.

The only reason is the desire to prevent regression in the future. But maybe it's too zealous insurance.

Yeah, if needed you can always add the trigger_error call later.

I read this line in the issue summary

The filter is applied during discovery already. A wrongly named test file is not executed and does not throw an error, because it is not found in the first place.

But I couldn't find any evidence that this is true - maybe I missed something. If we're not already limiting to *Test.php then I think we need to deprecate support for tests that do not end in *Test.php first and then we can remove this in Drupal 9.

Yeah we support other endings :(

Drupal test run
---------------

Tests to be run:
  - \Drupal\Tests\simpletest\Unit\TestDiscoveryTests

Test run started:
  Monday, May 7, 2018 - 12:40

Test summary
------------

Drupal\Tests\simpletest\Unit\TestDiscoveryTests               26 passes

Test run duration: 1 sec

./vendor/bin/phpunit -c ./core core/modules/simpletest/tests/src/Unit/TestDiscoveryTests.php
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\simpletest\Unit\TestDiscoveryTests
..........................                                        26 / 26 (100%)

Time: 510 ms, Memory: 8.00MB

Also the argument about what PHPUnit supports is not really correct - it'll run any file you tell it to ^^

Also the argument about what PHPUnit supports is not really correct - it'll run any file you tell it to ^^

Does it run this also when we just execute the testsuite?

./vendor/bin/phpunit -c ./core --filter TestDiscoveryTests
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing
..........................                                        26 / 26 (100%)

Time: 14.79 seconds, Memory: 792.00MB

OK (26 tests, 29 assertions)

Remaining deprecation notices (1)

  1x: Drupal\taxonomy\Tests\TaxonomyTestTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait
    1x in KernelTestSuite::suite from Drupal\Tests\TestSuites

Yep.

OK, then we still have the problem of reflection against traits that trigger deprecation errors during discovery: #2949363-23: Impossible to make trigger_error in some files without test fails

Let's do this in steps. I think given that tests are meant to be run we could make an argument to deprecate this in 8.6.x and remove support in 8.7.x. This would need release manager support. But at least that would give people time to fix any tests.

The problem is if we remove support we have no way to inform people because we're just no longer scanning the files.

This is still a very good idea! Because we do this we caused https://www.drupal.org/project/drupal/issues/3026414#comment-12940455

Just seeing how much would fail if we throw a deprecation.

This time it actually checks whether a valid Test has an invalid path.

This should rename the tests which are wrongly named in Drupal core.

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -175,6 +175,11 @@ public function getTestClasses($extension = NULL, array $types = []) {
+          @trigger_error($pathname, E_USER_DEPRECATED);
+          @trigger_error('Naming a test without a Test.php suffix is deprecated in Drupal 8.7 and will be removed before Drupal 9.0.x. Use a Test.php suffix instead.', E_USER_DEPRECATED);

"...will not be allowed in Drupal 9.0.0."

Also add $pathname to the deprecation message so there's only one @trigger_error().

This is the closest change record I could find but it doesn't say to use Test.php suffix for your file: https://www.drupal.org/node/1543796

@Neslee Canil Pinto, thanks for working on this! Would be great to attach an interdiff, see https://www.drupal.org/documentation/git/interdiff

#55 did unexpected change. And did not address #52

+++ b/core/modules/node/tests/src/FunctionalJavascript/SettingSummariesContentTypeTest.php
@@ -9,7 +9,7 @@
-class TestSettingSummariesContentTypeTest extends WebDriverTestBase {
+class SettingSummariesContentTypeTest extends WebDriverTestBase {

But #55 spotted the wrong renaming from my understanding.

Orginally, it's TestSettingSummariesContentType.php. In #51 it's renamed to TestSettingSummariesContentTypeTest.php, (Test append, but The leading Test still there). #55 renamed it to SettingSummariesContentTypeTest.php by removing the leading Test.

Instead of working upon #55, this patch started from #51. Made two changes.

1. Addressed #52
2. Rename the file TestSettingSummariesContentType.php to SettingSummariesContentTypeTest.php and renamed the class as well.

A simple Change Record added https://www.drupal.org/node/3123888

Re-rolling the patch, kindly review.

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -96,6 +96,10 @@ public function getTestClasses($extension = NULL, array $types = []) {
+        if (substr($pathname, -strlen('Test.php')) !== 'Test.php') {
+          @trigger_error("$pathname, naming a test without a Test.php suffix is deprecated in Drupal 8.7 and will not be allowed in Drupal 9.0.0. Use a Test.php suffix instead.", E_USER_DEPRECATED);

I do not think the re-roll in #61 is unnecessary, changing back the target branch to 8.9.x

The target component should be PHPUnit, I guess. And #58 only deprecated/@trigger_error for simpletest tests, not all types of test files covered.