Move TestDiscovery out of simpletest module, minimize dependencies

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -74,416 +46,38 @@ class TestDiscovery {
   +  protected function alterHook(&$test_list) {
   +    // Allow modules extending core tests to disable originals.
   +    $this->moduleHandler->alter('simpletest', $list);
      }
   

   Shouldn't you pass $test_list to the module handler, not $list?

2. +++ b/core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php
   @@ -410,6 +393,26 @@ public function providerTestGetPhpunitTestSuite() {
      }
   +  ¶
   +}
   

   trailing white space

Out of curiosity I checked for a test runtime regressions with an example:
../vendor/bin/phpunit --filter AccessManagerTest
That finishes in roughly 7 seconds before and after the patch, so I see no performance problem with this patch.

Keeping hook_simpletest_alter() in the simpletest specific TestDiscovery makes sense to me. I don't understand why we want to keep the cache in the Simpletest version and not just remove it completely. Because the Simpletest UI interacts with it and would be slow?

Just some super quick comment.

1. +++ b/core/lib/Drupal/Core/Test/TestDiscovery.php
   index 85a0696ca1..f90c74c75e 100644
   --- a/core/modules/simpletest/src/Exception/MissingGroupException.php
   
   --- a/core/modules/simpletest/src/Exception/MissingGroupException.php
   +++ b/core/modules/simpletest/src/Exception/MissingGroupException.php
   
   +++ b/core/modules/simpletest/src/Exception/MissingGroupException.php
   +++ b/core/modules/simpletest/src/Exception/MissingGroupException.php
   @@ -2,8 +2,10 @@
   
   @@ -2,8 +2,10 @@
    
    namespace Drupal\simpletest\Exception;
    
   +use Drupal\Core\Test\Exception\MissingGroupException as CoreMissingGroupException;
   +
    /**
     * Exception thrown when a simpletest class is missing an @group annotation.
     */
   -class MissingGroupException extends \LogicException {
   +class MissingGroupException extends CoreMissingGroupException {
    }
   

   Should we deprecate this exception?

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -2,27 +2,20 @@
   + *
   + * This class provides backwards compatibility for code which uses the legacy
   + * \Drupal\simpletest\TestDiscovery.
   + *
   + * @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0. Use
   + *   Drupal\Core\Test\TestDiscovery instead.
     */
   ...
   +class TestDiscovery extends CoreTestDiscovery {
   

   When we deprecate something we should call out to trigger_error() now, see https://www.drupal.org/core/deprecation

I really like how this patch allows us to move towards a more simple testing system, even on the longrun I hope we get rid of some of those aspects.
To be honest I'm wondering whether we can really get rid of caching ...

There's no use case where we need to discover tests more than once per request, other than when we test the UI form to make sure the form can discover tests and display them. And of course there's a use case where we *shouldn't* cache the list, because then the UI keeps the old discovered data even if the tests have changed.

This is a good point!

+++ b/core/modules/simpletest/simpletest.services.yml
@@ -1,4 +1,6 @@
   test_discovery:
+    # @todo Change this service so that it uses \Drupal\Core\Test\TestDiscovery
+    # in https://www.drupal.org/node/1667822
     class: Drupal\simpletest\TestDiscovery
     arguments: ['@app.root', '@class_loader', '@module_handler', '@?cache.discovery']

Why wouldn't we swap to using the non-deprecated code here?

Applying patch #24 for version 8.4.x shows following error :

error: patch failed: core/modules/simpletest/src/TestDiscovery.php:309
error: core/modules/simpletest/src/TestDiscovery.php: patch does not apply
error: patch failed: core/modules/simpletest/src/TestDiscovery.php:74
error: core/modules/simpletest/src/TestDiscovery.php: patch does not apply
error: core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php: already exists in working directory
error: core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php: No such file or directory
error: patch failed: core/tests/TestSuites/TestSuiteBase.php:1
error: core/tests/TestSuites/TestSuiteBase.php: patch does not apply

@Mohit Malik, this issue has a tag "Needs reroll" which is a signal that the patch no longer applies.

Happily there are people in the community who act on this signal

1. +++ b/core/lib/Drupal/Core/Test/TestDiscovery.php
   @@ -0,0 +1,487 @@
   +    $listing->setProfileDirectories([]);
   +    $extensions = $listing->scan('module', TRUE);
   +    $extensions += $listing->scan('profile', TRUE);
   +    $extensions += $listing->scan('theme', TRUE);
   

   makes sense to add theme engines, is there ability to configure it?

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -76,429 +44,12 @@ public function __construct($root, $class_loader, ModuleHandlerInterface $module
   -   * Returns all available extensions.
   ...
   -  protected function getExtensions() {
   -    $listing = new ExtensionDiscovery($this->root);
   

   it could use core discovery (alterable) to allow other extension types, like db-drivers

I mean that simpletest means that core can bootstrap and probably already yep

Just some nits I see

1. +++ b/core/lib/Drupal/Core/Test/TestDiscovery.php
   @@ -0,0 +1,509 @@
   +   * @todo Remove this when we deprecate simpletest module.
   

   remove when we remove simpletest module I'd say, not deprecate.

2. +++ b/core/modules/simpletest/src/Exception/MissingGroupException.php
   @@ -2,8 +2,19 @@
   +@trigger_error('Deprecated in Drupal 8.6.x for removal before Drupal 9.0.0 release. Use \Drupal\Core\Test\Exception\MissingGroupException instead. https://www.drupal.org/node/2949692', E_USER_DEPRECATED);
   ...
   + * @deprecated in Drupal 8.4.x for removal before Drupal 9.0.0 release. Use
   

   8.4/6.x => 8.7.x

3. +++ b/core/tests/Drupal/Tests/Core/Test/TestDiscoveryTest.php
   @@ -433,7 +436,8 @@ public function testGetTestClassesWithSelectedTypes() {
   -      'example' => [],
   +      'example' => [
   +      ],
   

   unrelated, this is hard enough to read, lets make it as small as possible.

Other then that, I went through the move of TestDiscovery with FileMerge and it is indeed only a move of old to new with two changes:
- the removal of moduleHandler
- the addition of alterHook to provide BC in the new version

Annoying indeed that git doesn't detect this as a move as it makes the diff hard to read. *shrug*

@Mile23 thanks, looks good to me, and would remove another blocker for removing Simpletest.

error: patch failed: core/modules/simpletest/src/TestDiscovery.php:70
error: core/modules/simpletest/src/TestDiscovery.php: patch does not apply
make: *** [patch88] Error 1

Thanks, looks good, back to RTBC.

1. +++ b/core/lib/Drupal/Core/Test/TestDiscovery.php
   @@ -0,0 +1,512 @@
   +  /**
   +   * Allow subclasses to invoke module hooks on the test list.
   +   *
   +   * This method can manipulate the test list.
   +   *
   +   * @param array $test_list
   +   *   An array of tests keyed by the the group name.
   +   *
   +   * @todo Remove this when the simpletest module no longer needs to fire its
   +   *   hook.
   +   *
   +   * @see hook_simpletest_alter()
   +   * @see https://www.drupal.org/project/drupal/issues/2866082
   +   * @see https://www.drupal.org/node/2939892
   +   * @see https://www.drupal.org/node/2949692
   +   */
   +  protected function alterHook(&$test_list) {
   +    return;
   +  }
   

   I think this is really odd to include here. But I cant think of another way of doing this.

2. +++ b/core/scripts/run-tests.sh
   @@ -19,7 +19,7 @@
   -use Drupal\simpletest\TestDiscovery;
   +use Drupal\Core\Test\TestDiscovery;
   
   +++ b/core/tests/TestSuites/TestSuiteBase.php
   @@ -2,7 +2,7 @@
   -use Drupal\simpletest\TestDiscovery;
   +use Drupal\Core\Test\TestDiscovery;
   

   I'm not sure we can make these changes yet as this means the hooks aren't fired for run-tests.sh or for phpunit - and they currently are - right?

hook_simpletest_alter() is triggered in run-tests.sh for sure.

Add

/**
 * Implements hook_simpletest_alter().
 */
function system_simpletest_alter(&$groups) {
  print __FUNCTION__ . "\n";
}

and use run-tests.sh to list all tests and you see

Available test groups & classes
-------------------------------

system_simpletest_alter
#slow
 - Drupal\KernelTests\Core\Extension\ModuleConfigureRouteTest
 - Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest
 - Drupal\Tests\simpletest\Functional\SimpletestUiTest
 - Drupal\Tests\system\Functional\Module\InstallUninstallTest
 - Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest
 - Drupal\Tests\workspaces\Kernel\WorkspaceIntegrationTest
Access

As far as I can see you're right that these hook are not fired for PHP unit so yeah the change to TestSuiteBase is fine.

I think rather than adding an alterDefinitions method in the new core class we should override the whole TestDiscovery::getTestClasses() in \Drupal\simpletest\TestDiscovery that way the new core class never has any pretensions of alterability.

Feedback has been addressed, fixed a minor nit, went through this again and this seems like a good BC solution. I'm not convinced we need all the BC here (who uses the simpletest hooks? seriously?), but this seems the safe way to do it.

Also update the test...

I am rerolling.

Resolving conflicts -- I blindly copied methods from

Drupal\simpletest\TestDiscovery

to

Drupal\Tests\Core\Test\TestDiscovery

I may have missed a subtle change so I will keep this assigned to myself until the test come back green. and the test count comeback unchanged.

I think the reroll is ok.

Spoke too soon just cleaning up a coding standard nit ... I introduced. Sorry for all the noise.

The reasoning in #77 makes sense to me, so +1

Went through this again, not an easy diff to read, with all the stuff being moved around, but I didn't spot anything getting lost.

If it turns out we do need to move more stuff it could probably be done in a follow up to minimise scope here

Committed 590ca4a and pushed to 8.8.x. Thanks!