[Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits

Theres a couple things here, as mentioned in another issue elsewhere: #2607866: [meta] DrupalCI/SimpletestUI/run-tests.sh/phpunit not in harmony .

We're not at all attached to run-tests.sh. But whatever it gets replaced with has to offer the same functionality, and phpunit does not currently do that.

Ideally we fix phpunit upstream, as we're not bound to run-tests.sh.

Seems related to #2580293: Patch having test with "PHP Fatal error" is marked as PASSED

--- a/core/modules/simpletest/src/TestDiscovery.php
+++ b/core/modules/simpletest/src/TestDiscovery.php

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -98,9 +98,7 @@ public function registerTestNamespaces() {

@@ -98,9 +98,7 @@ public function registerTestNamespaces() {
       $this->testNamespaces["Drupal\\$name\\Tests\\"][] = "$base_path/src/Tests";
 
       // Add PHPUnit test namespaces.

There is another bit up there:

    $this->testNamespaces['Drupal\\Tests\\'] = [DRUPAL_ROOT . '/core/tests/Drupal/Tests'];
    $this->testNamespaces['Drupal\\KernelTests\\'] = [DRUPAL_ROOT . '/core/tests/Drupal/KernelTests'];
    $this->testNamespaces['Drupal\\FunctionalTests\\'] = [DRUPAL_ROOT . '/core/tests/Drupal/FunctionalTests'];

which could be simplified as part of that. Do you think this is worth to change?

Prior to https://www.drupal.org/node/2232861, the code was exactly as you have it in this patch, so it might be worth investigating why the decision was made to split them up into "unit" and "functional" namespaces, and later add Kernel namespace (https://www.drupal.org/node/2304461).

This was added as part of #2232861-175: Create BrowserTestBase for web-testing on top of Mink, it seemed to be just about being explicit, which well, is a good reason, but should not block us here to fix the problem itself.

bad patch in core caused race conditions. This might have been affected.

Patch still applies.

Halfway blocker for #2102651: Port file_example module to Drupal 8.

The last few CI runs have passed, so I'm setting RTBC and the testbot is welcome to shoot me down.

Hid some files, re-running #11.

So as it turns out, I can't repro the error.

$ git apply trait-discovery-issue-2605664-v2.patch 
$ git checkout -- core/modules/simpletest/src/TestDiscovery.php 
$ ./vendor/bin/phpunit -c core/ --filter TraitAccessTest
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

.

Time: 8.73 seconds, Memory: 147.25Mb

OK (1 test, 1 assertion)

Here's a test-only patch derived from #11. Maybe the testbot has a different opinion.

Uhm, of course I can't repro because I'm using a different test runner. :-)

I blame Thanksgiving tryptophan.

Setting back to RTBC.

Hiding the test-only patch.

See also: #2624926: Refactor run-tests.sh for Console component.

This is not a bug - it was a design de

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -98,9 +98,7 @@ public function registerTestNamespaces() {
          // Add PHPUnit test namespaces.
   

   No longer multiple namespaces.

2. +++ b/core/modules/simpletest/tests/src/Traits/TestTrait.php
   @@ -0,0 +1,31 @@
   + *
   + * A nothing trait, but declared in the Drupal\Tests namespace.
   + *
   + * @see Drupal\Tests\simpletest\Unit\TestTraitAccessTest
   + */
   

   This text could be written in a more helpful manner. Also this should be in the docblock for the trait not the @file documentation.

3. +++ b/core/modules/simpletest/tests/src/Traits/TestTrait.php
   @@ -0,0 +1,31 @@
   +trait TestTrait {
   +  /**
   +   * Random string for a not very interesting trait.
   +   * @var string
   +   */
   +  protected $stuff = 'stuff';
   +
   +  /**
   +   * Return a test string to a trait user.
   +   *
   +   * @return string
   +   *  Just a random sort of string.
   +   */
   +  protected function getStuff() {
   +    return $this->stuff;
   +  }
   

   The string is not random.

4. +++ b/core/modules/simpletest/tests/src/Unit/TraitAccessTest.php
   @@ -0,0 +1,28 @@
   + * Tests if a PHPUnit test can find a trait in its own namespace.
   

   "in its own namespace" does not make sense. It could already find traits if it was in the Drupal\Tests\simpletest\Unit namespace.

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -97,10 +97,8 @@ public function registerTestNamespaces() {
-      // Add PHPUnit test namespaces.
-      $this->testNamespaces["Drupal\\Tests\\$name\\Unit\\"][] = "$base_path/tests/src/Unit";
-      $this->testNamespaces["Drupal\\Tests\\$name\\Kernel\\"][] = "$base_path/tests/src/Kernel";
-      $this->testNamespaces["Drupal\\Tests\\$name\\Functional\\"][] = "$base_path/tests/src/Functional";
+      // Add PHPUnit test namespace.
+      $this->testNamespaces["Drupal\\Tests\\$name\\"][] = "$base_path/tests/src";

One reason I've not committed this yet is that it removes the strictness of putting each test type in a defined folder. That said core's structure might not work for some modules.

That part of TestDiscovery only registers namespaces. It doesn't add to or limit discovery. We need the PSR-4 for discovering any valid *class* which might support the tests we eventually end up running.

Basically the behavior you're talking about works by accident, while diminishing the ability to write tests.

We'd want to refactor TestDiscovery::isUnitTest() such that it lets us examine classnames and approve them based on their namespace.

Should that happen here or in a follow-up?

Also, I'm not really sure why we need to have this namespace limitation. It seems like the test runner could determine the type of test in a bunch of other ways that have more to do with functionality over naming convention. Using the naming convention we'd have false passes on patches whose test never gets run.

So this stuff calls each other fairly promiscuously. To get what Alex wants here, we want for the latter type of calls (simpletest_test_get_all(), TestDiscovery::findAllClassFiles()) to allow only the approved directories.

So then at one extreme we want #2624926: Refactor run-tests.sh for Console component. :-)

At the other, we change those various methods in situ (without refactoring) to be more like core/tests/bootstrap.php, which scans the file system for test classes independent of autoloading. Speaking of which: #2301873: Use ExtensionDiscovery to register test namespaces for running PHPUnit :-)

Working on the minor refactoring mentioned above.

So here's the change.

The major changes are that I've introduced isValidTestNamespace(), which necessitated changing getExtensions() to store an ExtensionDiscovery object as a class property so we don't re-scan all the time.

I also separated concerns away from registerTestNamespaces(), adding getTestNamespaces() which only does the discovery and not the registration. I did this in hopes of writing a test, but we can't really test it just yet. See below. This change is a bit out of scope, so we can kick it out.

After adding some error_log scaffolding I see this:

[23-Dec-2015 01:17:43 Europe/Berlin] filtered out: Drupal\Tests\simpletest\Traits\TestTrait

Which is exactly what we want, assuming all tests are where they should be.

I'd write tests, but that requires two other changes:

• #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing
• Inject DRUPAL_ROOT

That failure is in a test of getTestInfo(), and it looks like this when you run it the right way. :-)

1) Drupal\Tests\simpletest\Unit\TestInfoParsingTest::testTestInfoParser with data set #2 (array('Drupal\Tests\simpletest\Funct...seTest', 'simpletest', 'Tests BrowserTestBase functionality.'), 'Drupal\Tests\simpletest\Funct...seTest')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'name' => 'Drupal\Tests\simpletest\Funct...seTest'
-    'group' => 'simpletest'
+    'group' => 'PHPUnit'
     'description' => 'Tests BrowserTestBase functionality.'
 )

/Users/paul/pj2/drupal/core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php:24

Drupal\Tests\simpletest\Functional\BrowserTestBaseTest was formerly discovered and added to PSR-4 during a scan of unit tests, but its @group of simpletest was retained.

However, since the behavior of getTestInfo() is to force all PHPUnit-based tests to the PHPUnit group, the addition of strictness to isUnitTest() means that BrowserTestBaseTest is marked as PHPUnit. BrowserTestBaseTest ultimately extends from \PHPUnit_Framework_TestCase, so it is, but I'm pretty sure it's a special case that shouldn't count as a unit test.

Additionally: There are no classes present in core which extend BrowserTestBase (other than BrowserTestBaseTest), so I'm wondering why it's there. I'm pretty sure it's part of the mink initiative, but the docblock is no help.

So: Should namespaces like Drupal\Tests\simpletest\Functional\ be unit tests? If no, then we move this check from isUnitTest() to isValidTestNamespace(). If yes, we change the test.

Bumping to 8.1.x, hopefully for cherry-picking to 8.0.x, because this is a bug.

Reverted the refactoring of registerTestNamespaces() since it's really out of scope.

Added a test for isUnitTest().

\Functional\ tests are now a special case of isValidTestNamespace(). Changes related to it should be easy to implement.

TestInfoParsingTest passes locally.

...AND TraitAccessTest passes. :-)

Still seeing #2476139: Fix UI test fail in SimpleTestBrowserTest locally, and we don't have more tests because #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing

+++ b/core/modules/simpletest/tests/src/Unit/TraitAccessTest.php
@@ -0,0 +1,31 @@
+    $this->assertSame($stuff, 'stuff', "Same old stuff");

Should be $this->assertSame('stuff', $this->getStuff);

+++ b/core/modules/simpletest/tests/src/Traits/TestTrait.php
@@ -0,0 +1,36 @@
+/**
+ * @file
+ * Contains \Drupal\Tests\simpletest\TestTrait.
+ */

@file is no longer required.

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -202,12 +237,52 @@ public function getTestClasses($extension = NULL) {
   +      $namespace = explode('\\', $classname);
   +      // Check for kernel tests: Drupal\KernelTests\.
   +      if ($namespace[1] == 'KernelTests') {
   +        return TRUE;
   +      }
   

   This would also need functional and FunctionalJavascript wouldn't it? Actually I think this sounds really similar to part of what ::getPhpunitTestSuite() is doing now.

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -416,6 +491,11 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
   +   * Unit tests have the following forms:
   +   * - Drupal\Tests\SomeTest
   +   * - Drupal\Tests\module\Unit\SomeTest
   +   * - Drupal\Tests\module\Kernel\SomeTest
   +   *
   

   That documentation is bad ... this is not a unit test ... its rather a phpunit based test. I know the method name is wrong, but at least the documenation should make that clearer

3. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -430,7 +510,7 @@ public static function isUnitTest($classname) {
   -      elseif ($namespace[3] == 'Unit') {
   +      elseif (in_array($namespace[3], ['Unit', 'Kernel'])) {
            // A module unit test.
            return TRUE;
   

   Don't we need functional and functionalJavascript in there as well?

4. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -445,12 +525,15 @@ public static function isUnitTest($classname) {
   +      $this->extensionDiscovery = new ExtensionDiscovery(DRUPAL_ROOT);
   

   could we inject the drupal root as parameter?

5. +++ b/core/modules/simpletest/tests/src/Traits/TestTrait.php
   @@ -0,0 +1,36 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\Tests\simpletest\TestTrait.
   + */
   +
   
   +++ b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
   @@ -0,0 +1,43 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\Tests\simpletest\Unit\TestDiscoveryTest.
   + */
   +
   
   +++ b/core/modules/simpletest/tests/src/Unit/TraitAccessTest.php
   @@ -0,0 +1,31 @@
   +/**
   + * @file
   + * Contains Drupal\Tests\simpletest\Unit\TraitAccessTest.
   + *
   + *  Demonstration of a very simple PHPUnit test using a trait.
   + */
   

   let's get rid of that

That documentation is bad ... this is not a unit test ... its rather a phpunit based test. I know the method name is wrong, but at least the documenation should make that clearer

At the time I wrote that, there was *no* documentation mapping namespaces to test types. I'm not sure there is at this point, either.

Don't we need functional and functionalJavascript in there as well?

Shows how old the patch is.

BTW, if you're in NOLA, find Torenware. :-)

Re-roll of #56 for 8.2.x. Addressing changes when it's green. Or red. :-)

Hello: #2721355: MissingDependentModuleUnitTest has the wrong namespace

That error leads to a new failure that was previously undiscovered. Therefore: TestDiscovery should burp when it finds a mismatch.

This is mostly re-done work.

We add TestDiscovery::getTestSuite($classname), which is similar to the existing getPhpUnitTestSuite().

getTestSuite() throws TestInWrongNamespaceException if the given classname does not fit into one of the established test suite categories such as PHPUnit-Kernel or Simpletest.

I had to disable the devel module to test this, because it found Drupal\Tests\devel_generate\DevelGenerateManagerTest and threw the exception. :-)

Lots of testing in the patch. I found TestDiscoveryTest::testGetPhpunitTestSuite() hidden on TestTestDiscovery, which is in TestInfoParsingTest.php...

simpletest group tests pass locally. Let's see the testbot dash my hopes.

This is looking much better now!! Thanks a ton! Here is a couple of small feedback and nitpicks.

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -91,7 +99,7 @@ public function registerTestNamespaces() {
        }
   -    $this->testNamespaces = array();
   +    $this->testNamespaces = [];
   
   @@ -101,30 +109,26 @@ public function registerTestNamespaces() {
   -    $this->availableExtensions = array();
   +    $this->availableExtensions = [];
        foreach ($this->getExtensions() as $name => $extension) {
   

   unrelated chage

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -101,30 +109,26 @@ public function registerTestNamespaces() {
   -      // Add PHPUnit test namespaces.
   -      $this->testNamespaces["Drupal\\Tests\\$name\\Unit\\"][] = "$base_path/tests/src/Unit";
   -      $this->testNamespaces["Drupal\\Tests\\$name\\Kernel\\"][] = "$base_path/tests/src/Kernel";
   -      $this->testNamespaces["Drupal\\Tests\\$name\\Functional\\"][] = "$base_path/tests/src/Functional";
   -      $this->testNamespaces["Drupal\\Tests\\$name\\FunctionalJavascript\\"][] = "$base_path/tests/src/FunctionalJavascript";
   +      // Add PHPUnit-based test namespaces.
   +      $this->testNamespaces["Drupal\\Tests\\$name\\"][] = "$base_path/tests/src";
   

   I really like this change. Less code to maintain, which existed for no good reason

3. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -101,30 +109,26 @@ public function registerTestNamespaces() {
   -
        foreach ($this->testNamespaces as $prefix => $paths) {
          $this->classLoader->addPsr4($prefix, $paths);
        }
   -
        return $this->testNamespaces;
      }
    
   @@ -154,7 +158,6 @@ public function registerTestNamespaces() {
   
   @@ -154,7 +158,6 @@ public function registerTestNamespaces() {
      public function getTestClasses($extension = NULL, array $types = []) {
        $reader = new SimpleAnnotationReader();
        $reader->addNamespace('Drupal\\simpletest\\Annotation');
   -
        if (!isset($extension)) {
   

   nitpick: unrelated changes

4. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -437,6 +438,37 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
   +  public static function getTestSuite($classname) {
   

   Nice variable name!

5. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -437,6 +438,37 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
   +    throw new TestInWrongNamespaceException('Test class ' . $classname . ' is in the wrong place.');
   

   Can we suggest possible other places instead as part of the exception message?

6. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -473,12 +510,14 @@ public static function getPhpunitTestSuite($classname) {
   +    if (empty($this->extensionDiscovery)) {
   +      $this->extensionDiscovery = new ExtensionDiscovery($this->root);
   +      // Ensure that tests in all profiles are discovered.
   +      $this->extensionDiscovery->setProfileDirectories([]);
   +    }
   

   Is it just me that caching this here is also a premature optimization and unneeded as part of this patch?

7. +++ b/core/modules/simpletest/tests/src/Kernel/TestDiscoveryTest.php
   @@ -0,0 +1,286 @@
   +
   +  /**
   +   * @covers ::getPhpunitTestSuite
   +   * @dataProvider providerTestGetPhpunitTestSuite
   +   */
   +  public function testGetPhpunitTestSuite($classname, $expected, $suite) {
   +    $this->assertEquals($expected, TestDiscovery::getPhpunitTestSuite($classname));
   +  }
   +
   +  /**
   +   * @covers ::getTestSuite
   +   * @dataProvider providerTestGetPhpunitTestSuite
   +   */
   +  public function testGetTestSuite($classname, $type, $expected) {
   +    $this->assertEquals($expected, TestDiscovery::getTestSuite($classname));
   +  }
   +
   +  /**
   +   * @return array[]
   +   *   - Class name.
   +   *   - PHPUnit-based test suite, or FALSE.
   +   *   - Test suite.
   +   */
   +  public function providerTestGetPhpunitTestSuite() {
   +    $data = [];
   +    $data['simpletest-webtest'] = [
   +      '\Drupal\rest\Tests\NodeTest',
   +      FALSE,
   +      'Simpletest',
   +    ];
   +    $data['simpletest-kerneltest'] = [
   +      '\Drupal\hal\Tests\FileNormalizeTest',
   +      FALSE,
   +      'Simpletest',
   +    ];
   +    $data['module-unittest'] = [
   +      'Drupal\Tests\simpletest\Unit\PhpUnitAutoloaderTest',
   +      'Unit',
   +      'PHPUnit-Unit',
   +    ];
   +    $data['module-kerneltest'] = [
   +      static::class,
   +      'Kernel',
   +      'PHPUnit-Kernel',
   +    ];
   +    $data['module-functionaltest'] = [
   +      '\Drupal\Tests\simpletest\Functional\BrowserTestBaseTest',
   +      'Functional',
   +      'PHPUnit-Functional',
   +    ];
   +    $data['module-functionaljavascripttest'] = [
   +      '\Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest',
   +      'FunctionalJavascript',
   +      'PHPUnit-FunctionalJavascript'
   +    ];
   +    $data['core-unittest'] = [
   +      '\Drupal\Tests\ComposerIntegrationTest',
   +      'Unit',
   +      'PHPUnit-Unit',
   +    ];
   +    $data['core-unittest2'] = [
   +      'Drupal\Tests\Core\DrupalTest',
   +      'Unit',
   +      'PHPUnit-Unit',
   +    ];
   +    $data['core-kerneltest'] = [
   +      '\Drupal\KernelTests\KernelTestBaseTest',
   +      'Kernel',
   +      'PHPUnit-Kernel',
   +    ];
   +    $data['core-functionaltest'] = [
   +      '\Drupal\FunctionalTests\ExampleTest',
   +      'Functional',
   +      'PHPUnit-Functional',
   +    ];
   +    $data['core-functionaljavascripttest'] = [
   +      '\Drupal\FunctionalJavascriptTests\ExampleTest',
   +      'FunctionalJavascript',
   +      'PHPUnit-FunctionalJavascript',
   +    ];
   +    return $data;
   +  }
   +
   +  /**
   +   * @dataProvider provideGetTestSuiteBadSuite
   +   */
   +  public function testGetTestSuiteBadSuite($classname) {
   +    $this->setExpectedException(TestInWrongNamespaceException::class);
   +    TestDiscovery::getTestSuite($classname);
   +  }
   +
   +  public function provideGetTestSuiteBadSuite() {
   +    return [
   +      ['\Boy\Is\This\Ever\Wrong'],
   +      ['\Drupal\Tests\module\kernel\Test'],
   +      ['\Drupal\Tests\module\functional\Test'],
   +      ['\Drupal\Tests\module\functionalJavascript\Test'],
   +      ['\Drupal\Tests\module\unit\Test'],
   +      ['\Drupal\Tests\module\Unfunctional\Test'],
   +    ];
   +  }
   +
   +}
   
   +++ b/core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php
   @@ -386,31 +388,6 @@ protected function getExtensions() {
    
   -  /**
   -   * @covers ::getPhpunitTestSuite
   -   * @dataProvider providerTestGetPhpunitTestSuite
   -   */
   -  public function testGetPhpunitTestSuite($classname, $expected) {
   -    $this->assertEquals($expected, TestDiscovery::getPhpunitTestSuite($classname));
   -  }
   -
   -  public function providerTestGetPhpunitTestSuite() {
   -    $data = [];
   -    $data['simpletest-webtest'] = ['\Drupal\rest\Tests\NodeTest', FALSE];
   -    $data['simpletest-kerneltest'] = ['\Drupal\hal\Tests\FileNormalizeTest', FALSE];
   -    $data['module-unittest'] = [static::class, 'Unit'];
   -    $data['module-kerneltest'] = ['\Drupal\KernelTests\Core\Theme\TwigMarkupInterfaceTest', 'Kernel'];
   -    $data['module-functionaltest'] = ['\Drupal\Tests\simpletest\Functional\BrowserTestBaseTest', 'Functional'];
   -    $data['module-functionaljavascripttest'] = ['\Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest', 'FunctionalJavascript'];
   -    $data['core-unittest'] = ['\Drupal\Tests\ComposerIntegrationTest', 'Unit'];
   -    $data['core-unittest2'] = ['Drupal\Tests\Core\DrupalTest', 'Unit'];
   -    $data['core-kerneltest'] = ['\Drupal\KernelTests\KernelTestBaseTest', 'Kernel'];
   -    $data['core-functionaltest'] = ['\Drupal\FunctionalTests\ExampleTest', 'Functional'];
   -    $data['core-functionaljavascripttest'] = ['\Drupal\FunctionalJavascriptTests\ExampleTest', 'FunctionalJavascript'];
   -
   -    return $data;
   -  }
   

   All that can happily live on the existing unit test, can't it?

Thanks for the review.

Addressing most of the items in #68.

#68.5: Added a link to the PHPUnit test page. I couldn't find a better set of docs.

#68.6: The optimization was present in the old patch, but yah, no worries. Removed.

#68.7: You'd *think*, wouldn't you? :-) Well, the test method itself isn't on the test class, it's on the mock class so it *never runs.* Furthermore, that test class is named TestInfoParsingTest, and we're testing TestDiscovery. I pondered folding TestInfoParsingTest.php into TestDiscoveryTest.php, because that's where it really should be. But TestDiscoveryTest needs to be a kernel test, whereas TestInfoParsingTest is a unit test, so I decided to keep them separate. We really should rename it, but again: Out of scope unless someone tells me it isn't.

Oops forgot interdiff.

Double your testing, double your fun.

The test runner needs a suite in order to run the test, regardless of core or contrib.

Contrib either has to impose the rules or we have to return a suite of something like 'unknown.'

So: If it's a core test we have to impose the rules. If it's not, then we don't.

The way to do that is add another static method called fileIsACoreTest() or something similar. In this method, we have a hard-coded list of core modules to compare against. We make a magic regex to extract the module name from the file path.

getTestInfo() will call this and add a member to $info named something like $info['test_is_core'] = TRUE. Then we pass this bool to the get*Suite() methods in order to break the rules selectively.

That means we have to inject the file path into getTestInfo(). Some callers of getTestInfo() don't care about this, such as the test form builder, so it's an optional argument.

At least that's what I'd do. Unassign yourself if you'd rather I do it.

I mean, not everyone likes building a ship in a bottle. :-)

Looks pretty good except for a few things.

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -323,7 +325,7 @@ public static function scanDirectory($namespace_prefix, $path) {
   -  public static function getTestInfo($classname, $doc_comment = NULL) {
   +  public function getTestInfo($classname, $doc_comment = NULL) {
   

   The reason this is static is: http://cgit.drupalcode.org/drupal/tree/core/scripts/run-tests.sh#n1069

   So either we leave it static, or we shave the yak in run-tests.sh. Let's leave it static for now.

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -437,15 +437,68 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
   +    $base_path = DRUPAL_ROOT . "/core/";
   

   Boo DRUPAL_ROOT. :-)

I'm going to assign this to myself and have a go.

+++ b/core/scripts/run-tests.sh
@@ -1065,8 +1065,9 @@ function simpletest_script_get_test_list() {
   // we need to do so here.
   if (!$types_processed) {
-    $test_list = array_filter($test_list, function ($test_class) use ($args) {
-      $test_info = TestDiscovery::getTestInfo($test_class);
+    $test_discovery = \Drupal::service('test_discovery');
+    $test_list = array_filter($test_list, function ($test_class) use ($args, $test_discovery) {
+      $test_info = $test_discovery->getTestInfo($test_class);
       return in_array($test_info['type'], $args['types'], TRUE);
     });

Oops I didn't make it this far down the patch. :-)

I'm pretty sure we don't want to rely on the service container in run-tests.sh. But we're already relying on simpletest being enabled, so we're also relying on the services that come with it, and that can enable it.

We move the methods back to static, and change their signature with an injected bool. This bool tells us whether to apply the core rules or not.

Please read the docblock changes and see if you can come up with better wording. :-)

I also tried making a TestIdentifier object, which is really the bestest solution, but also the trickiest to implement since it means untangling a whole bunch of dependencies and tests.

Keep It Simple, Sahib. The current identifier array works well enough, me thinks. The Bestest is the Enemy of the Goodest.

Well, TestIdentifier and implementation would be much simpler. It's just that it's already complex. :-)

Changes from #104 in this patch.

Thank you, this looks perfect for me!

+++ b/core/modules/simpletest/src/Exception/TestInWrongNamespaceException.php
@@ -0,0 +1,9 @@
diff --git a/core/modules/simpletest/src/Form/SimpletestResultsForm.php b/core/modules/simpletest/src/Form/SimpletestResultsForm.php

diff --git a/core/modules/simpletest/src/Form/SimpletestResultsForm.php b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
index d2a84d6..c7cebfd 100644

index d2a84d6..c7cebfd 100644
--- a/core/modules/simpletest/src/Form/SimpletestResultsForm.php

--- a/core/modules/simpletest/src/Form/SimpletestResultsForm.php
+++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php

+++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
+++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
@@ -235,7 +235,7 @@ protected function getResults($test_id) {

@@ -235,7 +235,7 @@ protected function getResults($test_id) {
    *
    * @param array $form
    *   The form to attach the results to.
-   * @param array $test_results
+   * @param array $results
    *   The simpletest results.
    *
    * @return array

Feel free to keep it: but in general this is totally out of scope of this issue

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -323,7 +325,7 @@ public static function scanDirectory($namespace_prefix, $path) {
   -  public static function getTestInfo($classname, $doc_comment = NULL) {
   +  public static function getTestInfo($classname, $doc_comment = NULL, $is_core_test = FALSE) {
   

   Missing documentation of $is_core_test. However, why is all this $is_core_test/isCoreTest() stuff necessary to fix the issue outlined in the issue summary?

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -453,15 +519,26 @@ public static function getPhpunitTestSuite($classname) {
   +        // We allow non-standard test suites in Contrib.
   +        if (!$is_core_class) {
   +          return 'Unclassified';
   +        }
   

   Why is this necessary? Given this is not supported in HEAD introducing it here seems out of scope and deserves it's own issue.

However, why is all this $is_core_test/isCoreTest() stuff necessary to fix the issue outlined in the issue summary?

Because apparently we needed to allow for different test suite types for contrib.

Also apparently this comes as a surprise to maintainers. :-)

So remove the 'unclassified' thing, wait for someone to complain, add it back in a follow-up?

I would suggest to do this first:
#2729711: Improve test coverage for ExtensionDiscovery

EDIT: Don't bother. As @Torenware points out in #116 below, this is not related at all.

@Torenware the whole unclassified thing still gives me pause.

This solution introduces a new problem: if a contrib module is non-compliant, it cannot be present (much less enabled) under the modules/ directory, or the test system will not work at all: running phpunit, run-tests.sh will fail before running any tests at all, and the admin/configuration/tests page white screens, all without any useful information about why this occurs.

This looks like a really good reason on first glance but digging deeper...

How is devel non-compliant? http://cgit.drupalcode.org/devel/tree/tests/src

I can see how rules is non-compliant... http://cgit.drupalcode.org/rules/tree/tests/src?h=8.x-3.x - but the thing is given that rules is not shipping with it's own phpunit.xml.dist I have no idea how these tests are run - even the travis.yml does not appear to have a way of running them. And here's the funny thing the Integration folder contains tests that extend Drupal\Tests\UnitTestCase so why are they making people jump through unnecessary hoops? Imo these tests should just be in an Integration folder under the Unit folder and then everything would just work.

@Torenware: You are right. I misread the issue title, or something like it.

@torenware: Let's get the traits working here with the stricter rules that @alexpott seems to want, and then work on widening the rules for contrib in another issue.

Currently, those tests won't be discovered anyway, so we don't lose anything by not addressing it here.

So with that in mind, shall we just throw the exception for 'illegal' tests and not worry about whether a test is in core?

@torenware in my mind it is both rules and devel_generate that are breaking the core test contract - ie. the contract that says without knowing anything about the module you should be able to run a module's tests by running run-test.sh --module MODULE_NAME. That is what we should support and all modules working that way makes everyone's life simpler. This also means that Drupal.org will successfully run their tests for them.

As I've said I'm +1 on supporting traits in in tests/src but I'm -1 on supporting non-runnable tests in tests/src or having code that supports that.

devel_generate has a further issue in that sub modules should not currently have phpunit tests because the Drupal notion of being able to centrally run tests from /core is an anathema to PHPUnit and the way it discovers tests. In the PHPUnit world the devel_generate module should have a phpunit.xml.dist and it's own bootstrap.php that would allow it to discover classes any way it likes.

The rules example highlights an interesting issue related to devel_generate - it's tests are only discovered by its travis implementation because it does /vendor/bin/phpunit -c ./core/phpunit.xml.dist ./module/rules. If it did /vendor/bin/phpunit -c ./core/phpunit.xml.dist --filter rules even though all the test classes contain the word rules it does not find the ones in /Integration because core's phpunit.xml.dist is not designed to find them but just providing a directory overrides the test discovery options we've put in place.

In fact, I thought this issue introduced a new issue because if tests are run directly though phpunit then it needs to follow the same rules as core. However PHPunit’s bootstrap.php adds tests/src so all code in there is autoloadable. One massive problem we have is that supporting two test runners which use different discovery mechanisms is very very complex.

So what should we do?

I don't know how to reconcile the change we're making here with the current structure of phpunit.xml.dist. And having the different test suites available and enforced by both phpunit and run-tests.sh/simpletest is a good thing. And being able to only run the unit tests as opposed to the functional and functionalJavascript is also useful. I guess what we need to do is to work out a set of principles about how we want test discovery to work and code for that. These principles might just answer that if you want autoloaded code you have to put it in a valid test sub directory.

Here's what I think the principles should be:

1. PHPUnit's test runner, run-tests.sh and the Simpletest UI should all work in as similar a way as possible.
2. It has to be possible to only run tests of a certain type. Lumping unit, functional and functional javascript tests together is a bad idea because they have different test infrastructure requirements.
3. Users should be able to get all a module's tests to run with run-test.sh --module MODULE_NAME
4. Running with run-test.sh --directory MODULE_DIRECTORY and ./vendor/bin/phpunit -c ./core/phpunit.xml.dist MODULE_DIRECTORY should result in running all the same PHPUnit based tests. (run-tests should also run the simpletest tests) These should also be the same tests when using the --module option.
5. Running with ./vendor/bin/phpunit -c ./core/phpunit.xml.dist --filter MODULE_NAME should also run all the module's tests

Given these principles...

We can not control how PHPUnit works with --directory discovers files - it is going to find all the tests under tests/src so given how our test system works we need to make Simpletest able to autoload from tests/src and run its discovery from there. This leads us to the conclusion that all tests under tests/src should be PHPUnit runnable tests. In all the examples given in this issue they are.

In order to make the --filter argument work the same, the phpunit.xml.dist needs to scan from tests/src too. So we need to have a test suite for such tests. I guess an unclassified test suite will do. But I don’t think we should have a different rules for contrib and core - this tends to make life more difficult rather than simpler. So let’s let core’s modules also have unclassifiable tests too. This makes the patch simpler - we then have one simple rule - tests in tests/src will be discovered and run using PHPUnit. Also this makes unclassifiable tests easy to detect - you just need to do ./vendor/bin/phpunit -vvv -c ./core/phpunit.xml.dist --testsuite unclassified --tap - so if you this with the Rules project in ./modules this will run the tests in its Integration directory.

The patch attached satisfies the above two paragraphs and meets the principles laid out too. To do this it makes all code in tests/src autoloadable, which solves the trait issue that led to this issue’s creation. All the rules module’s integration tests are now discoverable by both run-tests.sh and phpunit. It also makes devel_generate's test discoverable at least by run-tests.sh. Unfortunately it is still not discoverable by phpunit’s runner but that needs to be solved in a different fashion and by a different issue (which exists already). The patch also removes out-of-scope code cleanups made in the previous patches. The kernel test added by the previous patches is made a unit test because it can be - which resolves an @todo.

There is another opinion - having test code being autoloadable is a bug. The autoloader shouldn't be changed to include tests because doing so changes the system under test way more that a test should. I think we should consider moving to not having autoloadable tests in the future but that is a huge change which will need more planning - traits and base tests will need to move into a module’s src/Tests which is currently occupied by Simpletest's tests (web tests and its version of kernel tests).

@alexpott thanks so much for this comment. You have to know it's music to my ears. :-)

I opened a meta about all these issues a while back (in a more complain-y voice): #2607866: [meta] DrupalCI/SimpletestUI/run-tests.sh/phpunit not in harmony

Could we move that larger conversation there, or some other planning issue please? I totally don't mind doing work towards any of it as long as there is some structure to the conversation.

Agreed with all the principles.

I don't know how to reconcile the change we're making here with the current structure of phpunit.xml.dist

That's kind of the point of #2607866: [meta] DrupalCI/SimpletestUI/run-tests.sh/phpunit not in harmony ... They're already not reconciled before we fix this issue, and won't be reconciled afterwards, because this issue doesn't reconcile them. :-)

There is another opinion - having test code being autoloadable is a bug.

This is an interesting point, but it speaks to a hard-edged discipline that's completely missing from Drupal core. There are plenty of test quality topics we should cover before that, such as multiple @covers in unit tests and the like.

And of course moving traits and superclasses to src/Tests/ is the opposite of this issue. :-)

We could make a classmap for all files in test code directories rather than use PSR-4, but that seems ungainly.

Anyway. The patch.

1. +++ b/core/phpunit.xml.dist
   @@ -71,6 +71,33 @@
   +    <testsuite name="unclassified">
   +      <directory>./modules/*/tests/src</directory>
   

   Shouldn't core modules not ever have unclassifiable test types? We can add types in core, and in fact we have to. (Such as BrowserTestBase needing extra infrastructure.)

2. +++ b/core/phpunit.xml.dist
   @@ -71,6 +71,33 @@
   +      <exclude>../modules/*/tests/src/Unit</exclude>
   

   This leads us back to #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests but I can let it slide here.

   Ultimately, in line with Principle #1, we're going to have to make a Drupal-flavored suite loader so no point in protesting in this issue.

@Mile23 thanks for the review.

1. I think core and contrib modules having different capabilities is asking for trouble and adding the isCoreClass method unnecessary work
2. Yep #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests is the place to discuss how we deal with this.

Re the autoloader and testing...

We could make a classmap for all files in test code directories rather than use PSR-4, but that seems ungainly.

This is not necessary - a test loader should just enumerate the test directories and load each test by itself. There's no need to autoload the test.

Re reconciliation...

They're already not reconciled before we fix this issue, and won't be reconciled afterwards, because this issue doe sn't reconcile them

After this issue lands run-test.sh --directory MODULE_DIRECTORY and ./vendor/bin/phpunit -c ./core/phpunit.xml.dist MODULE_DIRECTORY will run the same set of tests. ATM for modules like Rules this is not the case. Both will autoload from tests/src - atm only PHPUnit does. That is some way towards reconciliation - which to me means running the same tests. I guess you are thinking about things like result formats - but that is not as important running the same tests. The only outlier is sub-modules which run-tests will pick up and PHPunit will not (by default --directory actually does) but that's #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests. And this is the same for core and contrib - it is just that no module in core has a sub module.

In an world in which each module ships with its own phpunit.xml.dist file, they could also ship with their own bootstrap.php file and in there make what they want, which might be for example providing the autoloading capabilities pointing to whatever files they need.

+++ b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
@@ -0,0 +1,233 @@
+namespace Drupal\Tests\simpletest\Unit {

Actually one doesn't need this indentation for namespaces, see https://3v4l.org/IQKDn

On top of that I'm wondering whether we could add some kind of documentation about good practises. Maybe #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests though is the better place for that.

@dawehner re the namespace thing... the second namespace in this instance is the global namespace so that's not possible. However, we only call drupal_valid_test_ua because we are going through a highly unlikely code path of discovering the site path. I've altered the unit test set up to create the dependencies necessary to not call the function. Doing this has the nice side effect of properly exposing the dependencies of test discovery in the test.

Wrt to documentation I think this is the type of thing that belongs in the handbook - specifically https://www.drupal.org/node/2116043

Where's the patch... here it is :)

+1 for an explicit setup!

I assume everyone on the issue implicitly understands why we're adding a new test suite to phpunit and how that solves run-tests.sh's inability to load traits because of the discussions but reading the summary and skimming comments I can't figure it out. Could we get an updated issue summary?

@neclimdul the new test suite is not just about run-tests.sh inability to load traits. It is also about the difference between ./vendor/bin/phpunit -c ./core/phpunit.xml.dist --filter MODULE_NAME and ./vendor/bin/phpunit -c ./core/phpunit.xml.dist MODULE_DIRECTORY. This should result in the same tests running. Atm for rules this does not happen because of the testsuites defined in phpunit.xml.dist

Also fair point about the IS - updated accordingly.

They're already not reconciled before we fix this issue, and won't be reconciled afterwards, because this issue doe sn't reconcile them

[..] I guess you are thinking about things like result formats - but that is not as important running the same tests.

Well, no not results formats, but that the discovery methods are different so therefore we will always be chasing this down.

For instance: #2301873: Use ExtensionDiscovery to register test namespaces for running PHPUnit

That's an issue attempting to make parity between how run-tests.sh and phpunit's bootstrap discover tests.

We should have the same discovery methods for both tools, but we don't. For instance your patch above looks an awful lot like this doesn't it? http://cgit.drupalcode.org/drupal/tree/core/tests/bootstrap.php#n70

RTBC++

Being the title into line with the current patch.

Well, no not results formats, but that the discovery methods are different so therefore we will always be chasing this down.

Until we either have no SImpletest or PHPUnit can run simpletest this will be the case. Of course we should have probably never allowed run-tests.sh and UI to run phpunit tests but we were too keen to add PHPUnit without affecting too much of how tests worked, users used them and the old DrupalQA worked. As we edge towards a Simpletest free future the test discovery stuff becomes moot. We just always use PHPUnit and ditch run-tests completely. But we will also need to solve #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests

The issue summary is still confusing for someone not already familiar with the issue.

If I understand correctly, there is a kind of tests that always lives in specific sub-namespaces (Unit, Kernel, Functional, FunctionalJavascript), but that may want to use traits or other stuff autoloaded from toplevel tests namespaces.

Discovery of such tests can happen in two ways: Via simpletest TestDiscovery, or via phpunit based on the phpunit.xml(.dist). And currently simpletest TestDiscovery only looks into the sub-namespaces, while phpunit looks into the toplevel tests namespaces.

Simpletest TestDiscovery only registers the sub-namespaces in the class loader, whereas phpunit registers the toplevel namespaces into the classloader (does it?). Also some part of the discovery depends on the class loader? Does it?

So there are two problems:
- In Simpletest, traits and possibly base or helper classes in toplevel tests namespaces cannot be autoloaded.
- Some tests are discovered by phpunit, but not found in simpletest.

Is this correct? If so, it would be great to update the issue summary accordingly. Start by briefly explaining the current situation, and why it is bad. Then what is the desirable situation. And then how we are planning to get there.

(I was going to ask this on IRC instead, but found none of the active participants online)

For the first problem, it seems that @Torenware's solution in #3 should be sufficient.
For the second problem, more is needed.
Right?

@Torenware thanks for working on this difficult issue for so long. I appreciate that you were frustrated by the lack of movement and the vagueness of my unease around the new 'unclassified' test type.

re #119

This is simply stupid. Why break the test system in this manner?

The current implementation is unacceptable. I present this one as superior to the current code.

These statements are not alright. Many people have put many hours into getting PHPUnit into core and expanding it's capabilities with Kernel, Functional and FunctionalJavascript testing. They are not stupid, inferior, or out to implement a system that is unacceptable.

Your efforts to make our testing system more robust and predictable with this issue are valuable. In the future, however, please be more considerate of the work of others as you make your own contributions. It's my role as a framework manager to ask questions about the architecture. Thanks for sticking with this difficult conversation and hopefully the next issue you work on will go quicker.

@donquixote the solution in #3 opens up the additional problem - with that patch if you have the rule project and go to the SimpletestUI it breaks because it has tests without a type. This is because TestDiscovery entangles autoloading and test discovery. This is also outlined in the proposed resolution section...

In order to support traits anywhere tests/src we need to change TestDiscovery to work the same way as PHPunit's bootstrap.php. Once TestDiscovery can autoload from tests/src we have an additional issue - tests outside the 4 testsuites will be discovered. To handle this we introduce a new test suite of unclassified.

I don't know how to make it clearer than that.

@quixote: Discovery of such tests can happen in two ways: Via simpletest TestDiscovery, or via phpunit based on the phpunit.xml(.dist). And currently simpletest TestDiscovery only looks into the sub-namespaces, while phpunit looks into the toplevel tests namespaces.

Simpletest TestDiscovery only registers the sub-namespaces in the class loader, whereas phpunit registers the toplevel namespaces into the classloader (does it?). Also some part of the discovery depends on the class loader? Does it?

Currently, if you run core's phpunit-based tests using phpunit, it will find and load traits for modules. If you do the same thing using TestDiscovery, it won't. That's because TestDiscovery currently only allows specific namespaces per module per test type, without allowing autoloading of the whole of the test namespace. So for instance if you have a Unit test and a Functional test and you define a trait for both of them to use, then TestDiscovery won't allow for autoloading that trait.

That's the problem we're trying to solve in this issue, in harmony with the principles in #123.

The solution is that we have TestDiscovery register the whole test namespace for autoloading, which creates a new problem: This allows autoloading of tests which don't fit into the categories core knows how to run.

An early solution to that problem was to throw an exception. @Torenware scrapped the exception and added a category for 'Unclassified.' @alexpott added a test suite for 'unclassified', which puts some edges on where these unclassified tests should be in the file system.

Updated issue summary with this comment. :-)

@neclimdul the new test suite is not just about run-tests.sh inability to load traits. It is also about the difference between ./vendor/bin/phpunit -c ./core/phpunit.xml.dist --filter MODULE_NAME and ./vendor/bin/phpunit -c ./core/phpunit.xml.dist MODULE_DIRECTORY. This should result in the same tests running. Atm for rules this does not happen because of the testsuites defined in phpunit.xml.dist

Looking at rules and reading back through the discussion, I think this is the expected behavior given that its defining tests outside of the defined phpunit test groups. The fact that this might work in run-test seems like a bug in run-tests.sh not the other way.

Looking at rules and reading back through the discussion, I think this is the expected behavior given that its defining tests outside of the defined phpunit test groups. The fact that this might work in run-test seems like a bug in run-tests.sh not the other way.

Looks like a good follow-up.

After reading updated issue summaries, and a little chat with @alexpott on IRC, I still think this is a two-fold issue. As I currently see it, it might be better (or have been better) for understanding if these issues were treated separately.

The first problem is that ExtensionDiscovery does not distinguish between autoload namespaces and test namespaces.
This would be quite simple to fix (as was already proposed in #3):

       // Add PHPUnit test namespaces.
       $this->testNamespaces["Drupal\\Tests\\$name\\Unit\\"][] = "$base_path/tests/src/Unit";
       $this->testNamespaces["Drupal\\Tests\\$name\\Kernel\\"][] = "$base_path/tests/src/Kernel";
       $this->testNamespaces["Drupal\\Tests\\$name\\Functional\\"][] = "$base_path/tests/src/Functional";
       $this->testNamespaces["Drupal\\Tests\\$name\\FunctionalJavascript\\"][] = "$base_path/tests/src/FunctionalJavascript";
-    }
 
-    foreach ($this->testNamespaces as $prefix => $paths) {
-      $this->classLoader->addPsr4($prefix, $paths);
+      $this->classLoader->addPsr4("Drupal\\Tests\\$name\\", "$base_path/tests/src");
     }

Imo this could be a separate patch in a separate issue, with a dedicated issue summary and git commit message that everyone will understand.
It will not fix all the problems described in this issue, but it will also not introduce regressions.

With this proposed change, in simpletest, the toplevel tests namespaces would be used for autoloading, but tests would still only be discovered in the dedicated sub-namespaces. phpunit would not be affected.

As a side detail: The autoload classmap would still be based on the sub-namespaces, whereas the PSR-4 directories would be for the toplevel tests namespaces. This just means that classes outside the tests namespaces would not benefit from classmap for autoloading performance. So what.

------------------

The current patch in #128 also fixes this problem, but it does more than this:

• It makes classes, traits and interfaces in tests namespaces outside the dedicated sub-namespaces autoloadable. So far, same as above.
• It makes test classes in toplevel tests namespaces outside the dedicated sub-namespaces discoverable by TestDiscovery.
• It assigns a new test suite name "Unclassified" to the test classes outside the regular sub-namespaces, which used to be ignored in current code, but are now found by TestDiscovery.

I have the impression that the issue summary does still not properly distinguish these two problems. The issue summary text takes the coupling of tests namespaces and autoload namespaces for granted, and does not address it as a problem.

Splitting this issue into two issues would make things clearer, imo.
If we keep it in one issue, at least the issue summary should explain the two problems separately.
Maybe I can give it a shot myself, if someone can confirm (on IRC, if you want) that I correctly understand it. I just think I am the wrong person, since I have not actively participated much.

No, because if it is a bug, "unclassified" is syncing the bug to phpunit so the follow up would be rolling back this patch.

@neclimdul: run-tests.sh can say TestDiscovery::getPhpunitTestSuite($test) and not run the test based on the answer. It could already do this but doesn't.

Edit: Oh wait, it does, indirectly through simpletest_script_get_test_list(), as long as you specify --types.

Looking at rules and reading back through the discussion, I think this is the expected behavior given that its defining tests outside of the defined phpunit test groups. The fact that this might work in run-test seems like a bug in run-tests.sh not the other way.

We're in a world of pain if the following commands run a different sets of PHPUnit test for inexplicable reasons.

• ./vendor/bin/phpunit -c ./core/phpunit.xml.dist --filter MODULE_NAME
• ./vendor/bin/phpunit -c ./core/phpunit.xml.dist MODULE_DIRECTORY
• run-test.sh --directory MODULE_DIRECTORY
• run-test.sh --module MODULE_NAME

For those that have not tested it (and I strongly you encourage to) just get the Rules project and run the commands in #144 with and without the patch.

I does not matter if a non PHPUnit test is in tests/src anywhere it'll not be detected as a PHPUnit test and nothing goes wrong. The problem is when PHPUnit tests are placed outside the currently expected tests/src sub-directories.

P.S. I owe everyone an apology for the RTBC review. I totally wasn't following this close enough to catch the scope change from when I first read this.

As far as the commands, I did run exactly the listed commands. I saw exactly what I expected though.

I think its entirely a bug that run-test.sh would test /modules/rules/tests/src/LolCoreIsSilly/mytest.php. The documentation is outdated and actually only lists the Unit namespace but it very explicit that it must be in a specific namespace.

• Must correspond with the following namespace and class:
• namespace Drupal\Tests\phpunit_example\Unit
• class Double_DisplayManagerTests extends UnitTestCase

Furthermore core's implementation and phpunit.xml.dist have a clearly defined sets of namespaces and "Integration" is not one of them. I don't think a bug in simpletest's runner implementation that contradicts documentation defines a new feature.

err... ok d.o...

This said, why could we not set the type/suite of the test via annotation, rather than use the directory structure?

This would never work with phpunit's runner as far as I know which I think is a no-go. IMHO. I would be very sad to see my phpstorm phpunit tests go away anyways.

So digging deeper I found some interesting things.

First, rules tests didn't run in simpletest with #128 and immediately blew up with standards violations (missing @group). Which made me realize they never where able to run under simpletest, phpunit or anything else. I'm sorry for my misleading, confusing and down right wrong comments. I clearly still didn't understand all the discussion again in this issue. gomen-nasai, mea culpa, I'm very sorry.

It seems if rules is testing these it must be using a custom phpunit.xml somewhere not in the repo which is totally ok though problematic for testbot and clearly confusing other users. Does anyone know where this namespace came from and how rules runs its tests?

Second, /core/tests/bootstrap.php where we setup phpunit's class loading(this is our design not phpunits), does exactly what donquixote suggested and registers the top level namespace into the autoloader. So aligning discovery seems as simple as broadening simpletest's autoloading, though the suggested change alone removes the core namespace registrations so it is only slightly more complicated. It would end up looking something like this.

Additionally I attached a testlog roughly matching alex's commands in #144. As you can see the directory commands roughly align and the the module commands roughly align. I didn't investigate the differing failures in depth but they seem related to php7 incompatibilities not test discovery.

There seem to be some strong feelings about detecting tests outside of the previous test suites. While I personally disagree, I think we can all agree they are at least outside the scope of syncing the class loading. Maybe we can open a feature follow up to discuss improving the usability around placing phpunit tests outside the expected test suites?

Does anyone know where this namespace came from and how rules runs its tests?

Best I can tell from a quick archaeological dig, prior to adding BrowserTestBase in beta 10 (april 2015), we didn't have separate test suites and all of the test directory was treated as unit tests. In this case Integration would have been treated as part of the unit test suite and "worked." So maybe rules just came up with its own thing really early on that doesn't fit into Drupal 8's current testing patterns and this part of the issue is just be a case of Rules falling behind and missing a changelog and misleading other developers.

#137, #148

@donquixote the solution in #3 opens up the additional problem - with that patch if you have the rule project and go to the SimpletestUI it breaks because it has tests without a type.

I don't think so.
The snippets in #141 and in #3 only change the autoload namespaces, not the discovery namespaces.
I do not see any patch following this approach. Just the code snippets in the comments. But I think it should be easy to imagine.

This is because TestDiscovery entangles autoloading and test discovery. This is also outlined in the proposed resolution section...

Well this is currently so, but it doesn't have to be. The snippet in #141 / #3 would break this entanglement.

My concern with the current approach is that we are doing two things at the same time: A quite straighforward change that is almost a bug fix (making the namespaces autoloadable), and a possibly controversial behavior change.

The issue summary suggests that doing these two things at once is technically necessary. But I do not think this is correct.

@donquixote: So we don't tell the truth about registered namespaces as a return value from registerTestNamespaces(). Then findAllClassFiles() does its thing to discover tests based on the namespaces we specified.

We keep testNamespaces around so we don't have to re-do the scan, and as a side-effect we never re-register the actual namespaces with the autoloader because the test namespaces are cached in a property.

Something like that?

That would solve the problem of traits in larger namespaces. It would however turn TestDiscovery into even more of an oral history than an engineering practice. :-)

To solve in this approach in an explicit manner, we'd want to keep the method for gathering the namespaces as it does now, for the purpose of discovery, without registering the namespaces.

The caller would be responsible for performing the registration, where we'd explicitly skip the various silo'd paths for modules and just register the top level. We'd provide a method for this on TestDiscovery.

This means we'd have to change findAllClassFiles() to do this.

That would solve the problem of finding traits in test namespaces without changing behavior, in a way that wouldn't make me cringe.

@torenware No worries that actual clear it up. It looks like its not the beta change I found at fault. Rules uses drupal-ti and looking into drupal-ti's "phpunit-core" runner, it seems to have nothing to do with the way drupal core run's tests... It does exactly matches alex's command line though. https://github.com/LionsAd/drupal_ti/blob/master/runners/phpunit-core/sc...

I think its better we focus on wrapping up the autoloading fix and discuss the #2738041: Improve DX around phpunit tests outside of core's test suites. DX issue around tests outside of our test suites separately.

@mile23 ::registerTestNamespaces() make no assertions about the returned namespaces connection to the registered autoloading namespaces. It doesn't even discuss autoloading in the documentation, its just an undocumented artifact. While it isn't how I would have written it, I don't see a reason to refactor it when the fix is simple.

If I where to refactor it, I'd converge the code in tests/bootstrap.php with what ever logic is here so it is literally running the same code instead of mirroring the same logic.

a possibly controversial behavior change.

How is actually running tests provided by a module controversial. It way more controversial to not run them.

@neclimdul re the rules @group problem - yes one (just one) test is missing the @group and that's precisely because they can't test them using run-tests.sh so they've never noticed. Because DrupalCI is not running the tests. Which is the problem we're trying to solve.

But all that said, autoloading of anything in tests/src is actually wrong. Can these projects autoload their tests?

• Silex
• Symfony's http foundation - this one is fun it uses composer.json to specifically exclude tests from the autoloader
• Doctrine
• Guzzle
• Laravel

Nope, not a single one. Changing the autoloader of the system-under-test is architecturally wrong and something we should stop. So perhaps we should change tack and remove autoloading of anything in tests/src in both run-tests.sh and bootstrap.php and make discovery work through file listing like it does for PHPUnit.

Separately, test discovery needs to be transparent and not able to miss tests. All PHPUnit test under tests/src should be run by run-tests.sh/DrupalCI. Doing anything different is very confusing and not helpful to developers who have testing working locally but then don't on DrupalCI. The other way to go is to stop testing contrib with core's phpunit.xml.dist and encourage projects to ship with their own - then every one is free to come up with their own solutions. But I would argue that that approach is really unfriendly to contrib module developers.

Of course we can't change tack and stop autoloading tests at this point in the cycle because that would break a lot of tests. :(

So we're left between the proverbial rock and a hard place - we have to compromise. And I suggest that the compromise of #128 is the way to go, because, PHPUnit and Simpletest will then both autoload from the same point AND all the variants of the commands in #144 run the expected tests.

As don has pointed out repeatedly, none of this has anything to do with autoloading traits from a different namespace other then the fact that we where lazy and through the logic into one method.

But all that said, autoloading of anything in tests/src is actually wrong. Can these projects autoload their tests?

Silex
Symfony's http foundation - this one is fun it uses composer.json to specifically exclude tests from the autoloader
Doctrine
Guzzle
Laravel

Everyone of those projects has exactly one test suite. Additionally, none of them provide a framework for running tests for sub projects. IE, you don't just drop https://github.com/FriendsOfSymfony/FOSRestBundle into your symfony project and run symfony's test suite. You run FOSRestBundle's phpunit.xml.dist and that runs tests.

The problem I want to discuss in the other issues is we've built an opinionated framework for running tests for modules. We have (mostly) clearly laid out the rules for how that should be setup. At least from within phpunit.xml.dist. Another (opinionated) test runner is running tests for Drupal projects with a different set of opinions that are contradictory to this and its causing problems.
Do we cede our opinion and match that test runner?
Do we take a middle ground and gum up our test runner with some unmaintainable gibberish?
Do we try and find other ways of providing feedback to the user that we're not going to run their tests?
Do we investigate if we can handle something better in drupalci? like letting people provide a phpunit.xml?
Do we investigate if maybe drupal-ti wants to align its test runner to what core is doing? Do they even know their core runner is causing problems?

There is no special runner for drupal-ti... they are just using phpunit, our phpunit.xml.dist and providing a directory path to the module. Nothing special is occurring. Providing a path completely overrides the testsuite stuff in our xml. This is how phpunit works and has nothing to do with drupal-ti.

Also we're not ceding anything - we support three test runners in core - phpunit, run-tests.sh and Simpletest UI. This is about making them work (with any of their existing options) in as similar a way as possible.

Do we take a middle ground and gum up our test runner with some unmaintainable gibberish?

@neclimdul you know better than to be offensive about efforts to improve Drupal. No one wants to gum up anything and maintaining the three runners is already hard.

Just asking:
Could it be possible that someone (a contrib module) intentionally does not want their tests to be found by Drupal simpletest TestDiscovery, or by Drupal phpunit?
Such a module would put the tests that are meant to be found by Drupal into one of the dedicated sub-namespaces.
Other tests, which are not meant for Drupal, would go somewhere outside of these namespaces.

Such a module could already exist, right?
By changing the behavior of the test discovery, this hiding would no longer work.
Is this what we intend to do?

EDIT: I just notice that #152 is exactly what I've been proposing all along.
+1 :)

I just notice that #152 is exactly what I've been proposing all along.
This patch eliminates all my concerns.
+1 :)

Could it be possible that someone (a contrib module) intentionally does not want their tests to be found by Drupal simpletest TestDiscovery, or by Drupal phpunit?

Firstly let's find a module that wants to do that - hopefully a well-used and important module to the Drupal ecosystem - because we certainly have a module that would like it's integration tests run by DrupalCI - that's Rules. Secondly, going back to principles... having tests that are hidden from test runners (that can be run by core's runners) sounds like... untested tests. Thirdly if you want to opt out of Drupal's test infra just don't put your tests in tests/src just stick them in tests/special_flower_test or whatever. You're going to have to write your own runner or use a different runner so using a completely different directory for that (way more special and rare use-case) is totally acceptable.

To me #152 is the worst of all worlds - we continue to have more autoloadable in tests without being able to run Rules tests. So we encourage people to place stuff outside the currently supported directories without supporting tests there - unless you use phpunit and point it at the directory.

@neclimdul and @donquixote - why don't you want Rules integration tests to run? Why do you want developers to have to learn the arcane structure beneath src/tests? We've got evidence that developers are already making mistakes. Why do we want developers to have stuff that looks tested but is not on DrupalCI? Untested tests are worse than anything, because it is silent and you just might never know until stuff breaks on a real site.

why don't you want Rules integration tests to run?

I just think that the two questions should be discussed separately, and then fixed with separate patches, in separate issues:
1. Which classes should be autoloadable?
2. Which classes should be discovered as tests?

So far I have not seen a convincing argument why this belongs together.
A number of comments imply a technical dependency between these two questions. But #152 shows there is none.

The current situation is like this:
In both phpunit and simpletest, tests are only discovered in the dedicated subdirectories for the test suites.
In phpunit, classes (and interfaces, and traits) in toplevel tests namespaces are autoloadable.
In simpletest, classes (and interfaces, and traits) in toplevel tests namespaces are NOT autoloadable. Only the classes (and interfaces, and traits) in the sub-namespaces are autoadable.

So the only difference between phpunit and simpletest, in this regard, is the autoloading.

There are clearly two steps (duh, this is 3 now. but 2 for D8):

1. Align the behavior of simpletest and phpunit, in the least intrusive way possible. This is what #152 does. For BC, we can only grow the autoloadable classes in simpletest. We cannot shrink the autoloadable namespaces in phpunit.
2. Decide whether we want to find tests outside the dedicated sub-namespaces. This would apply to phpunit and simpletest equally. This is a behavior change, and imo should go into a separate issue. This new issue would be much nicer to read, because it would not contain the distracting talk about autoloading.
3. In Drupal 9 we may discuss to not autoload anything in tests namespaces. I currently do not have a strong opinion about this. I just see it as not possible for D8 due to BC.

@donquixote - I'm just proposing that "Which classes should be autoloadable?" the answer is none - but we can't do that cause BC. So this issue then should be about "Which classes should be discovered as tests?" because that's what is important. Hence the updates to the issue summary and title. Also #172 is ignoring the directory option in both run-tests and phpunit which currently work differently and shouldn't.

I'm just proposing that "Which classes should be autoloadable?" the answer is none - but we can't do that cause BC. So this issue then should be about "Which classes should be discovered as tests?" because that's what is important.

But the autoloading question is not over.
"None" is not an option, I agree.
But the current situation is also not an option, because it is different in phpunit and simpletest.
The only option left that does not break BC is to register the same autoload namespaces in simpletest that we already register for autoloading in phpunit. A patch as in #152 would fix this inconsistency, and level the playing field for a follow-up.

The other patches have the same effect on the autoload namespaces. But there this fix is packaged with something else, which makes it less transparent e.g. in the git history. And someone studying the commit will then find this issue, where a large portion of comments are talking about traits that fail to autoload.

The introduction of an "Unclassified" test suite deserves some more discussion, which better happens in a dedicated issue. E.g. what if we want to introduce other test suite subdirectories in the future, but these tests are already "Unclassified"?

@donquixote they just become whatever testsuite we've added. Exactly as would have already occurred when we added the FunctionalJavascript recently.

re #166that is technically incorrect but only in a misunderstanding of terminology https://github.com/LionsAd/drupal_ti/tree/master/runners. Notice there is a phpunit and a phpunit-core. My assumption is that phpunit-core tries to mimic core's behavior but it is the root of this problem because it is not.

re #167I'm sorry for my phrasing. I do feel that the proposed solution is a significant maintenance pain and the introduction of a defacto API that could be a long term DX problem. That is why I'm pushing so hard to discuss that part of this issue further separately.

re #171I do want them to run. I don't know that running them in that namespace is correct though. I want them to engage people who care explicitly in fixing that problem in the best possible way though. I personally feel that changing our discovery is not the solution and exposing the fact that the tests are outside of the documented namespaces could be a much better option. We have opinions about where tests are located, we don't expect them to be in "src/tests" or "testing/src" but in "tests/src/(Unit|Kernel|Functional|FunctionalJavascript)" and those tests do not meet that criteria and I think that's valid.

My only problem with this issue has been that is was accidentally an under the radar discussion of changing test discovery listed as fixing autoloading traits in simpletest like phpunit. I would like to fix the problem at hand an open up the discussion about the DX issue with tests outside the defined namespace explicitly.

So after a discussion with Alex in IRC I'm just going to step aside on this. I do not think we should be running tests outside our defined namespaces and the directory versions of our commands are behaving as expected, bypassing any sort of automated test detection. But my complaint lodged I guess we're looking at #128.

#128 RTBC as of #129 and #146, so resetting to RTBC.

I think we should revisit this after we deal with #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests, which is narrower in scope, doesn't begin to add behavior, and might help us have more clearly-defined test suite loading behavior to modify here as needed.

#TCDrupal 2016 Sprints

#2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests is fixed, so this is unblocked.

I'd suggest that this issue could be 8.2.x, but we're in beta and it might be disruptive.

Yes, #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests takes over the test discovery part for discovery using the phpunit tool. It doesn't change the behavior of run-tests.sh, however.

Doing a bit of a reset here... This is the test we want to have pass. It currently passes when run from PHPUnit, but not from run-tests.sh.

$ ./vendor/bin/phpunit -c core/ --filter TraitAccessTest
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

.

Time: 19.32 seconds, Memory: 239.25Mb

OK (1 test, 1 assertion)

$ php core/scripts/run-tests.sh --browser --class 'Drupal\Tests\simpletest\Unit\TraitAccessTest'

Fatal error: Trait 'Drupal\Tests\simpletest\Traits\TestTrait' not found in /Users/paulmitchum/projects/drupal8/core/modules/simpletest/tests/src/Unit/TraitAccessTest.php on line 20

And now we make the test pass by adding a new rule:

If you want to use a trait across different test suites and have it run in run-tests.sh, put it in $extensionname/tests/src/Traits/.

$ php core/scripts/run-tests.sh --class 'Drupal\Tests\simpletest\Unit\TraitAccessTest'

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

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

Test run started:
  Tuesday, August 16, 2016 - 21:46

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

Drupal\Tests\simpletest\Unit\TraitAccessTest                   1 passes                                      

Test run duration: 1 sec

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -119,6 +119,10 @@ public function registerTestNamespaces() {
       $this->testNamespaces["Drupal\\Tests\\$name\\Kernel\\"][] = "$base_path/tests/src/Kernel";
       $this->testNamespaces["Drupal\\Tests\\$name\\Functional\\"][] = "$base_path/tests/src/Functional";
       $this->testNamespaces["Drupal\\Tests\\$name\\FunctionalJavascript\\"][] = "$base_path/tests/src/FunctionalJavascript";
+
+      // Add discovery for traits which are shared between different test
+      // suites.
+      $this->testNamespaces["Drupal\\Tests\\$name\\Traits\\"][] = "$base_path/tests/src/Traits";

Maybe this has been answered already, but why do we have to make up yet another directory name? Can't we just do this for modules:

$this->testNamespaces["Drupal\\Tests\\$name\\"][] = "$base_path/tests/src";

And remove all the extra folders for Unit, Kernel etc.?

If a test case is in the wrong folder then we can fail the test in the setUp() method of UnitTestCase for example, right?

Then people can put their traits wherever they want and we don't have to invent a trait location Drupalism.

If a test case is in the wrong folder then we can fail the test in the setUp() method of UnitTestCase for example, right?

See: #2465029: Ensure that PHPUnit tests are in the correct namespace which makes a test listener to fail tests in the wrong place. We also have TestSuiteBase which won't find the test if it's in the wrong directory for the suite.

$this->testNamespaces["Drupal\\Tests\\$name\\"][] = "$base_path/tests/src";

See #123 which says:

@alexpott: There is another opinion - having test code being autoloadable is a bug. The autoloader shouldn't be changed to include tests because doing so changes the system under test way more that a test should. I think we should consider moving to not having autoloadable tests in the future but that is a huge change which will need more planning - traits and base tests will need to move into a module’s src/Tests which is currently occupied by Simpletest's tests (web tests and its version of kernel tests).

This is a good guiding principle though of course we're not strictly following it.

Agreed having a *\Traits\ namespace (as in #188) is not good, but it's an explicit way to deal with this issue: It's a way to not store test traits in src/, while also following the principle of limiting classes which can be autoloaded.

In an ideal world we'd change TestDiscovery to not class-load any test namespaces and use an approach similar to TestSuiteBase. In this scenario, we'd *only* set up autoloading for the traits directory.

Another example where this is causing some issues: #2818891: Fatal error: plugin csv not found

I don't follow either and I also don't understand why modifying the autoloader during testing is such a big problem. I disagree that test helpers such as traits and base classes should live with the production code. I rather have them in the explicit namespace/folder for tests.

Anyway, now that everybody is hopelessly confused I guess it makes sense to just close this issue and start again in a new one? Or shorten the issue summary so that it is understandable?

To get going again here we could also just do exactly whatever alexpott said and stop arguing. I don't care that much about the details of this, I just want to use test traits - so I promise to shut up now :)

@Torenware: if you put your test base classes and traits in src/Test of your module as suggested by alexpott does that work for you?

Re #195: #2818891-8: Fatal error: plugin csv not found and https://dispatcher.drupalci.org/job/default/236548/console. This uses 'Drupal\Tests\migrate_source_csv\CSVDataProvider' & 'tests/src/CSVDataProvider.php'.

@heddn: did you try moving the trait to src/Tests/CSVDataProvider? Looks like you put it into the tests directory which is not what we are suggesting?

So if you apply the patch in #11, you have a handy test of the current state of trait autoloading for tests. We assume that autoloading for contrib will work the same way as autoloading for a core module (in this case simpletest).

We can run this test in phpunit:

$ ./vendor/bin/phpunit -c core/ --filter TraitAccessTest
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 20.83 seconds, Memory: 88.00Mb

OK (1 test, 1 assertion)

We can't run it in run-tests.sh:

$ php ./core/scripts/run-tests.sh --class 'Drupal\Tests\simpletest\Unit\TraitAccessTest'

Fatal error: Trait 'Drupal\Tests\simpletest\Traits\TestTrait' not found in /Users/paul/pj2/drupal/core/modules/simpletest/tests/src/Unit/TraitAccessTest.php on line 22

Basically:

• The phpunit tool allows for this discovery because our bootstrap file adds the prefix at tests/src/.
• run-tests.sh does not allow for this discovery because it silos the prefixes relative to Unit/, Functional/, etc.

Which is correct?

We have test cases and base classes that need to be autoloaded for the existing code base, within extensions in core. This works under both test runners because base classes inherit from the suite base classes (such as BrowserTestBase and so forth) so they are in the same PSR-4 space either way.

Traits are supposed to work across these silo'd boundaries, allowing for DRY between different test types. bootstrap.php permissively allows this, while TestDiscovery does not. We've been using traits in core for quite a while in order to allow for BC between new testing classes and old ones. This is possible because the traits are still silo'd next to their base classes, and the simpletest-based tests can then autoload them much like the base classes.

So the work-around is that traits can exist inside one of these silos, such as \Drupal\module\Functional\YourTrait and thus be autoloadable in either tool. A Unit suite test could then use a trait from a Functional namespace, though this does confuse the value of the namespace, and only works by accident. Future changes could have different test suite namespaces autoload in different ways such as only autoloading the Functional namespace during functional tests.

A true solution would be to determine the policy for autoloading such files and modify the tools as necessary to implement that policy.

So: What's the policy?

Enforcing the silos is probably a better policy, because it means that we're in more explicit control of what classes are autoloaded when.

This suggests that the best solution here is a special Traits namespace, which is expected to be autoloaded for all test suites. It would be in module/tests/src/Traits, or \Drupal\module\Traits\YourTrait. This has the benefit of being explicit and the downside of being another DX hoop to jump through.

How do we support this?

We make a change similar to that in #188. This explicitly adds the extensions' Traits namespace to the autoloader.

Then we make bootstrap.php perform autoloading similar to that in TestDiscovery, which adds the suite silo behavior, and also sets the Traits namespace to be autoloaded. That's the change in the patch presented here.

Moved to 8.2.x so we can start using this sooner.

+++ b/core/tests/bootstrap.php
@@ -77,15 +77,28 @@ function drupal_phpunit_contrib_extension_directory_roots($root = NULL) {
+  $suite_names = ['Unit', 'Kernel', 'Functiona', 'FunctionalJavascript'];

Nitpick: "Functiona" should be "Functional".

updated patch with changes from #199.

@Yogesh Pawar
Please always post an interdiff. Thank you!

@dawehner - Thank you so much for reminding me.
please check the interdiff.

Thanks for catching my typo. :-)

+++ b/core/modules/simpletest/tests/src/Unit/TraitAccessTest.php
@@ -0,0 +1,24 @@
+class TraitAccessTest extends \PHPUnit_Framework_TestCase {

all unit tests should extend UnitTestCase.

I don't agree that this is the best solution (I would still just autoload everything in the tests directory, your explanations don't convince me). But if this is an acceptable compromise I would RTBC this for the sake of making progress.

all unit tests should extend UnitTestCase.

Do they? Sorry for my snarky comment, but at least for me this really doesn't matter. If you don't use anything from it, why should you use it.
I was also wondering the other day: Should we convert UnitTestCase into a single trait you could use?

Yes, they should extend UnitTestCase for consistency sake. The setUp() method of UnitTestCase covers some common pitfalls that might be hard to figure out otherwise once you expand your unit test.

Yes, they should extend UnitTestCase for consistency sake. The setUp() method of UnitTestCase covers some common pitfalls that might be hard to figure out otherwise once you expand your unit test.

A unit test is supposed to be isolated. If you inherit the behavior of a setUp() you don't need, then you break the isolation of that test.

If we discover that we need the behavior of setUp() later, then we can just switch base classes.

Also, UnitTestCase has a problem, in that it always calls code in \Drupal and FileCacheFactory. This means that every UnitTestCase test that doesn't set up a @covers for every test method now also inadvertently covers \Drupal and FileCacheFactory. This means that coverage metrics aren't accurate. #2626832: Figure out how to check for unintentionally covered code

In this specific case, we don't need or care about \Drupal or FileCacheFactory, and the state of those objects shouldn't have any bearing on the test. We only care about whether the trait is discovered. If the fact that we didn't clear the pseudoglobal \Drupal in setUp() breaks our test later, then that's not an indication that this test should have used UnitTestCase. It's an indication that subsequent code has broken our expectation that there's no dependency between this test's behavior and \Drupal. Then we'll learn something useful trying to fix that.

As for the rest: In #198 I'm not trying to convince anyone that a Traits namespace is the best solution. I'm showing how the Traits namespace is the best solution given what the maintainer said. If there's a better solution then yay. If there's more clarification from the maintainers about a better way, then yay. Do those instead.

The problem is that the container is globally accessed in many places via \Drupal - if you don't clear the container in setUp() you might get unexpected test passes when your unit test grows bigger. Unfortunately at this point we need to accept that \Drupal is called in far too many places and we need to cope with that - by using UnitTestCase amongst other things. Otherwise you get into weird behavior with unit tests, and you only learn something after hours of debugging.

That's why using \PHPUnit_Framework_TestCase sets a bad example in Drupal, because Drupal is not as modern as we want it to be yet.

Anyway, I should probably open a dedicated issue for that. We are wrongly using \PHPUnit_Framework_TestCase in other places as well, so this should probably not hold up this issue.

RTBC otherwise!

Opened #2824313: All unit tests should use UnitTestCase base class.