Remove simpletest module from core

git rm -r core/modules/simpletest

This needs the beta must-have followup to make sure we've solved everything for it.

I think we also need to do that for existing actual sites that have it enabled?

And I think we also need to remove ""drupal/simpletest": "self.version"," from core/composer.json, that's not in the patch yet from what I see?

This should deal with most fails except the upgrade path ones, plus #7. There are probably some remaining references that need updating as well.

Thanks for review. Addressed #9 and added back a stub simpletest.info.yml, we also need the config schema as simpletest.settings.yml is tested. Also reverted composer.json as we are shipping a stub (we still include entity_reference in composer.json so I thought this was more consistent)

Though does having the stub allow the full contrib module to still be installed? How does module discovery deal with duplicate modules?

What we need is basically what we have here https://www.drupal.org/project/drupal/issues/3075490#comment-13333900
Right? Except rerolled for D9....

Duplicate modules are no problem, it will use the one from contrib, tested this with the patch in that issue

@Lendude that patch changes run-tests.sh which we are trying to avoid for the time being, but most of it is remarkably similar. That issue also hardcodes the TestDiscovery class; here we provide test discovery as a service in core.services.yml, contrib simpletest can then swap in its own test discovery service as required. Not sure about the changes in TestRunnerKernel though, will something be required here to get the contrib simpletest to boot?

Actually, that made me notice that this comment can be removed here:

    // @todo Use \Drupal\Core\Test\TestDiscovery when we no longer need BC for
    //   hook_simpletest_alter().

> We should remove simpletest.settings.yml and the associated test and config schema IMO

simpletest.settings is actually config in the filled dump; if we remove the schema then SchemaCheckTestTrait fails when called from UpdatePathTestBaseFilledTest. We could skip it but it feels like we are working around the issue of providing a new filled dump that doesn't contain simpletest? If we do decide to skip it I think we can add something like this to UpdatePathTestBaseFilledTest:

  /**
   * SimpleTest has been removed in Drupal 9.
   *
   * @todo Remove this when the database dump is replaced.
   */
  protected $configSchemaCheckerExclusions = ['simpletest.settings'];

However, I think any other test that uses drupal-8.8.0.filled.standard.php.gz and calls runUpdates() will run into the same failure.

NW for the composer.json change in #14 and comment noted in #13

Addressed #13-16.

Please add commit credit to @Lendude and @Mile23 who did a lot of the groundwork for this in #3075490: Move simpletest module to contrib.

Removed the CSS include from Stable, but didn't remove the CSS itself.

> We should also remove the composer.json entry since this is the most likely thing to conflict with the contributed simpletest module

So this is the cause of the remaining test failure; ComposerIntegrationTest::testAllModulesReplaced() wants to ensure that each core module appears in composer.json. I modified the test to skip hidden modules, which I think is OK and lets us remove other modules the same way in the future.

Do we need to keep parts of simpletest.install around, so the uninstall is clean? simpletest_uninstall() does some cleanup and I assume the presence of simpletest_schema() would drop the database tables too. Otherwise, I think users might run into "table already exists" when installing the contrib SimpleTest.

Looks really good, just some observations:

1. +++ b/core/core.services.yml
   @@ -1713,3 +1713,6 @@ services:
   +  test_discovery:
   +    class: Drupal\Core\Test\TestDiscovery
   +    arguments: ['@app.root', '@class_loader']
   
   +++ b/core/scripts/run-tests.sh
   @@ -1040,9 +1026,7 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
   +  /** @var \Drupal\Core\Test\TestDiscovery $test_discovery */
      $test_discovery = \Drupal::service('test_discovery');
   

   I think we require that all service classes have an interface, TestDiscovery doesn't have this right now.

   And then we can use the interface in the type hint.

   But don't know how hard of a rule this is really.

2. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -84,6 +85,14 @@ public function testAllModulesReplaced() {
   +        // Skip any modules marked as hidden.
   +        $info_yml = $module_path . '/' . $file_name . '/' . $file_name . '.info.yml';
   +        if (file_exists($info_yml)) {
   +          $info = Yaml::parseFile($info_yml);
   +          if (!empty($info['hidden'])) {
   +            continue;
   +          }
   +        }
            $module_names[] = $file_name;
   

   Should we add an assertNotEmpty(module_names) after this to make sure we are not accidentally skipping everything?

Thanks for review.

#23.1 I think that is out of scope. I only changed that line because it referred to a now removed class, and it was an incorrectly formatted typehint anyway:

-  /** $test_discovery \Drupal\simpletest\TestDiscovery */
+  /** @var \Drupal\Core\Test\TestDiscovery $test_discovery */

#23.2 is a good idea, will add this later if nobody else gets there first. I tried to be careful here to not skip modules where the info.yml is not named the same as the directory; this is always the case in core but it doesn't have to be from what I remember.

Addressed #22 and #23.2.

simpletest_uninstall() must skip if drupal_valid_test_ua() returns TRUE or update tests end up destroying their own environment; I am not sure what we can do here except some manual testing.

RTBC +1

+++ b/core/modules/system/system.post_update.php
@@ -267,3 +267,12 @@ function system_post_update_entity_reference_autocomplete_match_limit(&$sandbox
+/**
+ * Uninstall SimpleTest.
+ *
+ * @see https://www.drupal.org/project/drupal/issues/3110862
+ */
+function system_post_update_uninstall_simpletest() {
+  \Drupal::service('module_installer')->uninstall(['simpletest']);
+}

The update to uninstall it was already added here.

Yay! Needs a change record to explain developers how they can restore the functionality. If that is not immediately possible then we need to stay clear of committing this close before a tagged release to let enough time to figure out a contrib experience that people can try with (even if its not the final one for Drupal 9). Also needs a release notes snippet. This is a big step :)

To repeat my comment from #17 which might get missed:

Please add commit credit to @Lendude and @Mile23 who did a lot of the groundwork for this in #3075490: Move simpletest module to contrib

Just credited both :)

Just to clarify what I meant, especially if we are to put this in a tagged release before figuring out what contrib needs to ultimately/eventually do, it would be useful to document the interim (possibly ugly) steps. Something like "Simpletest is removed from core, while we figure out how to make this easily available in contrib, apply these 5 dirty steps to your projects including undoing a core patch and dancing around the room twice".

I personally think if this is to be included in a tagged core release without even a 5-step dirty workaround for now to get it back, then we would be making people chase down rabbits that they would not need to otherwise. "Here, we removed simpletest, so if you have tests that relied on it, just ignore this tagged release" is IMHO worse than "Here, we removed simpletest, so for now you need to apply these 5 steps, it will get better promise".

If figuring out the eventual solution precedes putting this into a tagged core release, then we don't need to document any workarounds IMHO. That said, I am not a release manager, so these are personal opinions :)

Attempted to summarise the state of things to the best of my knowledge in this release notes snippet, added to issue summary:

Simpletest module is removed from core in Drupal 9.

1. If your tests rely on a component of simpletest module, such as Drupal\simpletest\WebTestBase then your tests will not work anymore with Drupal core only.
2. If you ran tests from the web user interface, that is not possible in Drupal core anymore. See https://www.drupal.org/docs/8/phpunit/running-phpunit-tests for instructions on how to run tests from the command line.

Follow #3093621: [META] Plan to get a stable D8/D9 release of the SimpleTest module and contribute there to make the formerly core base classes and the web user interface available in a contributed module. If that route does not work out, simpletest will be added back to Drupal core before the first beta release, see #3110983: Verify that simpletest tests can be run in contrib, or add simpletest back to core.

+++ b/core/core.services.yml
@@ -1713,3 +1713,6 @@ services:
+  test_discovery:
+    class: Drupal\Core\Test\TestDiscovery
+    arguments: ['@app.root', '@class_loader']

+++ b/core/scripts/run-tests.sh
@@ -66,8 +66,6 @@
-    // @todo Use \Drupal\Core\Test\TestDiscovery when we no longer need BC for
-    //   hook_simpletest_alter().
     $groups = \Drupal::service('test_discovery')->getTestClasses($args['module']);

@@ -95,21 +93,9 @@
+  $test_classes = \Drupal::service('test_discovery')->findAllClassFiles();

@@ -1040,9 +1026,7 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
+  /** @var \Drupal\Core\Test\TestDiscovery $test_discovery */
   $test_discovery = \Drupal::service('test_discovery');

Please don't make TestDiscovery a service. Just instantiate the class in run-tests.sh.

That way:

1) We don't have a wholly unnecessary dependency on the kernel/service locator in the test runner script. run-tests.sh is the only place TestDiscovery is used.

2) We're not stuck with an unchangeable TestDiscovery class until Drupal 11.

The rest looks good. Glad to see this happening. :-)

@Mile23: does making it a service allow Simpletest in contrib to swap it out? Simpletest has/had its own extended test discovery class, so I figured by making it a service it could swap in the extended version and still use run-tests.sh? (all untested, but in theory!)

edit: I guess if all it provided before was hook_simpletest_alter() and the contrib version is not going to provide that, swappability doesn't matter?

Right, the service in the module is there for BC, and to provide the BC alter hooks.

Since we don't need to do that any more, we don't need a service. We only need to discover tests in run-tests.sh. (And we also use TestDiscovery a little in the test suite discovery for phpunit.)

It's very unlikely that contrib will add back the alter hooks, but they can if they want to by providing the same type of shim service we're removing here.

The only other contrib concern is how to inject test_discovery into the UI form. That's their problem. :-) But they can declare the service in the module and swap out for testing as needed.

@mixologic suggested this for testing what next steps should be:

1. Find a module that still has simpletest testing requirements.
2. Patch that modules drupalci.yml file such that it applies a core patch that makes all of the simpletest changes that we're proposing.
3. Figure out what the next steps are for that module.

This link provides some ideas for which modules could one pick: http://grep.xnddx.ru/search?text=WebTestBase&filename=

So #25 is passing, and has general consensus minus my complaint about adding a new service.

Examples has some remaining WTB tests, in its testing_example, though the branch isn't passing. https://www.drupal.org/node/594964/qa

I meant to set NW in the last comment. :-)

Discussed further with catch and we decided that we we could make the change to TestingKernel here and then contrib only has one change to make.

I've also fixed up #35 - great point @Mile23 we shouldn't be adding that to core.

I wonder if we should add a build test to prove simpletest testing works. Yes it means that we'll couple the contrib simpletest module to core a bit - but I think we'll want to know when we break it's basic functionality. At least it would let us plan changes that impact it.

Addressed #35 and I like the idea of making this as simple as possible for contrib so I implemented #44 as well.

(crosspost from a train with patchy wifi signal!)

The ExtensionDiscovery approach in #45 is better than mine so go with that :)

Here's proof that #45 works for contrib - #3112696-14: Drupal 9 compatibility and re-enable SimpleTests after core removal

Let's wait for green-ness.

Cool so #45 is green - so lets add a unit test to show support for adding the simpletest module info at the very least - as we have no code in core that depends on this.

+1 on all the stuff from #45 on.

The patch has changed a little since #45, so I'd RTBC here except that #3112696: Drupal 9 compatibility and re-enable SimpleTests after core removal doesn't use the patch in #49.

#3112696: Drupal 9 compatibility and re-enable SimpleTests after core removal now also green with the patch from #49. Awesome stuff!

Added a link to https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes... to the CR for people that don't have a drupalci.yml in their project yet.

All feedback has been addressed, we have green test results, we have follow ups. Ready?

Closed #2748967: Trigger E_USER_DEPRECATED for BC support in simpletest_run_tests() as a duplicate, adding credit.