Turn simpletest_clean*() functions into a helper class

It looks to not apply anymore

Re-rolled #2.

I think that this solution for crashes when cleaning environment is good alternative to another one.

The only deficiency is that it is a bit heavier than the one on the link above.

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -9,6 +9,7 @@
   +use Drupal\simpletest\EnvironmentCleaner;
   
   @@ -644,68 +645,24 @@ function simpletest_generate_file($filename, $width, $lines, $type = 'binary-tex
    function simpletest_clean_environment() {
   ...
   +  $cleaner = new EnvironmentCleaner(\Drupal::root(), TestDatabase::getConnection(), \Drupal::database(), \Drupal::translation());
   +  $cleaner->cleanEnvironment();
    }
   

   It would be nice to not have this below the simpletest namespace but under the generic test bit.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -644,68 +645,24 @@ function simpletest_generate_file($filename, $width, $lines, $type = 'binary-tex
    function simpletest_clean_database() {
   ...
    function simpletest_clean_temporary_directories() {
   
   @@ -718,29 +675,8 @@ function simpletest_clean_temporary_directories() {
    function simpletest_clean_results_table($test_id = NULL) {
   

   We should throw deprecation notices and have a @deprecated tags :(

3. +++ b/core/modules/simpletest/src/EnvironmentCleaner.php
   @@ -0,0 +1,174 @@
   + * @todo: Promote to actual service status. This will require promoting
   + *   TestDatabase to a service as well.
   

   I don't think this is something we have to do. We can do dependency injection without it being a service, aka. be somehow part of a module. What about dropping this todo?

4. +++ b/core/modules/simpletest/src/EnvironmentCleaner.php
   @@ -0,0 +1,174 @@
   + * @todo: Inject config and cache services.
   

   Honestly, test code has different requirements than production code IMHO. One could make up a scope which is minimal aka. just moving code, but sure, let's create a followup for that?

Btw cmi2 also "figured out" environment for config
The file_service also depends on config and stream wrappers which is blocked on related

Needed a reroll.

No changes, just a reroll for now.

Needed another reroll, also got rid of an unused use while doing it.

Really just some nits, the rest looks really nice.

1. +++ b/core/lib/Drupal/Core/Test/EnvironmentCleaner.php
   @@ -0,0 +1,189 @@
   + * Clean up after tests.
   

   This could be a little more descriptive, 'Helper class for cleaning generated test environments.'. I don't know, something like that?

2. +++ b/core/lib/Drupal/Core/Test/EnvironmentCleaner.php
   @@ -0,0 +1,189 @@
   +   * This could be the same as $testDatabase, or it could be different.
   

   Either leave this out, or explain how/when it's the same or when it's different

3. +++ b/core/lib/Drupal/Core/Test/EnvironmentCleaner.php
   @@ -0,0 +1,189 @@
   +  /**
   +   *
   +   * @var \Symfony\Component\Console\Output\OutputInterface
   +   */
   +  protected $output;
   

   Needs a description

4. +++ b/core/lib/Drupal/Core/Test/EnvironmentCleaner.php
   @@ -0,0 +1,189 @@
   +  /**
   +   *
   +   * @return int
   +   *   The number of tables that were removed.
   +   */
   +  protected function doCleanDatabase() {
   

   Needs a description

5. +++ b/core/lib/Drupal/Core/Test/EnvironmentCleaner.php
   @@ -0,0 +1,189 @@
   +  /**
   +   *
   +   * @return int
   +   *   The count of temporary directories removed.
   +   */
   +  protected function doCleanTemporaryDirectories() {
   

   Needs a description

6. +++ b/core/modules/simpletest/simpletest.services.yml
   @@ -1,9 +1,17 @@
   -    # @todo Change this service so that it uses \Drupal\Core\Test\TestDiscovery
   -    # in https://www.drupal.org/node/1667822
   +    # Drupal\simpletest\TestDiscovery is deprecated, but we still want to use it
   +    # as the service here to support BC hooks.
   

   Unrelated change?

7. +++ b/core/modules/simpletest/simpletest.services.yml
   @@ -1,9 +1,17 @@
   +
   +  environment_cleaner_factory:
   +    class: Drupal\simpletest\EnvironmentCleanerFactory
   +    arguments: ['@service_container']
   +  environment_cleaner:
   +    class: Drupal\simpletest\EnvironmentCleanerService
   +    factory: 'environment_cleaner_factory:createCleaner'
   +
   

   Extra newlines above and below are not needed.

@Mile23++

Tested this some more. The output on the bot seems fine, but when running 'clean environment' via the Simpletest UI I get the following feedback:

That seems to work better without the patch.

Also I needed a cache rebuild for the clean up to work in the Simpletest UI because of the new service being introduced. So I guess we need a post_update to force a cache rebuild?

Looks good now.

Feedback has been addressed. So one question remains: do we need an update function to force a cache clear? Seems excessive, but lets see if others agree.

+++ b/core/lib/Drupal/Core/Test/EnvironmentCleanerInterface.php
@@ -0,0 +1,43 @@
+<?php
+
+namespace Drupal\Core\Test;
+
+interface EnvironmentCleanerInterface {
+
+  /**
+   * Removes all test-related database tables and directories.
+   *
+   * This method removes fixture files and database entries from the system
+   * under test.
+   *
+   * @param bool $clear_results
+   *   (optional) Whether to clear the test results database. Defaults to TRUE.
+   * @param bool $clear_temp_directories
+   *   (optional) Whether to clear the test site directories. Defaults to TRUE.
+   * @param bool $clear_database
+   *   (optional) Whether to clean up the fixture database. Defaults to TRUE.
+   */
+  public function cleanEnvironment($clear_results = TRUE, $clear_temp_directories = TRUE, $clear_database = TRUE);
+
+  /**
+   * Remove database entries left over in the fixture database.
+   */
+  public function cleanDatabase();
+
+  /**
+   * Finds all leftover fixture site directories and removes them.
+   */
+  public function cleanTemporaryDirectories();
+
+  /**
+   * Clears test result tables from the results database.
+   *
+   * @param $test_id
+   *   Test ID to remove results for, or NULL to remove all results.
+   *
+   * @return int
+   *   The number of results that were removed.
+   */
+  public function cleanResultsTable($test_id = NULL);
+
+}

Would it make sense to mark these methods @internal since their signatures might change later while working with #3075608: Introduce TestRun objects and refactor TestDatabase to be testable? I think these is not to be considered public API.

Committed 914d47f and pushed to 8.8.x. Thanks!