Split up simpletest into simpletest and testing_ui to enable easier decisions for Drupal 9

+1 - I think we should also consider calling the UI module testing_ui rather than simpletest.

So duplicate of #2866082: [Plan] Roadmap for Simpletest then.

+1 - I think we should also consider calling the UI module testing_ui rather than simpletest.

Came here to say this.

The fail says:

1) Drupal\Tests\system\Functional\Module\InstallUninstallTest::testInstallUninstall
Behat\Mink\Exception\ExpectationException: Link with label online documentation for the Testing UI module found.

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -27,94 +27,10 @@ function simpletest_help($route_name, RouteMatchInterface $route_match) {
          $output .= '<p>' . t('The Testing module provides a framework for running automated tests. It can be used to verify a working state of Drupal before and after any code changes, or as a means for developers to write and execute tests for their modules. For more information, see the <a href=":simpletest">online documentation for the Testing module</a>.', [':simpletest' => 'https://www.drupal.org/documentation/modules/simpletest']) . '</p>';
   

   The 'online documentation' link for simpletest.module goes to the Drupal 7 page.

2. +++ b/core/modules/testing_ui/testing_ui.module
   @@ -0,0 +1,102 @@
   +    case 'help.page.testing_ui':
   

   Like the test fail says, there's no link to online documentation for this module, so we'll have to make some and link to it.

Added the link to the simpletest docs for now, just to see if that gets this in the green. We can discuss if this needs its own doc page or that we need to update the simpletest docs to show that this is now two modules.

Interdiff added some stuff I didn't change, just did the docs link.

We have patches in here, so it's not only a plan anymore :)

Needed a reroll, so here we go, no other changes added.

One thing that needs to be addressed still is the splitting of the /tests/ into stuff dealing with the UI and stuff dealing with the test runner/WebTestBase.

I think its clear that the simpletest API module would be useful from drush/drupal console. If for some reason we want to keep it. Please explain in the IS what would testing_ui in itself allow you to do without simpletest API, so the reasoning behind this issue is outlined.

@dawehner: no, if we are to write the change notice for removing the API module but not the UI module, we would be writing: "We removed the simpletest API module however kept the test runner user interface because it is still useful for [FILL THIS IN]." I am looking for [FILL THIS IN] since that explains why breaking them up is useful in the first place.

> Please explain in the IS what would testing_ui in itself allow you to do without simpletest API, so the reasoning behind this issue is outlined.

1. Running stuff on the command line is complicated and scary for a lot of users. A browser-based UI is nice and simple. We want new contributors to get into the habit of running tests.

2. Functional tests where you want to check the HMTL output during a test are a pain on the CLI, because you're switching between the terminal and the browser. Back in the old days of Drupal 7 it was much nicer to see both browser test output and test log in the browser.

Similar but slightly different #2866082: [Plan] Roadmap for Simpletest suggests making the simpletest module only show the UI form and manage in-site test runs, while moving all test-runner-related code into core/tests/. That would require some refactoring, which I assume maintainers are loathe to do because it seems risky. (I assume because no one will actually say...)

This issue splits the code differently, so that we don't do any refactoring of the test-runner code, we only split it up into two modules. That makes some sense if you're not sure which code is needed for which parts of the testing system. But if you're not sure, you could just ask Mile23, who researched it all to file #2866082: [Plan] Roadmap for Simpletest.

The summary is: It's trivial to split out the UI part and the in-site batch runner, so that's a task that we can take on and get the ball rolling. Whether we do it as part of a plan to refactor run-tests.sh to be better or whether we do it in order to kill run-tests.sh is another question entirely, and one that we really should answer before we do much more work.

Running stuff on the command line is complicated and scary for a lot of users. A browser-based UI is nice and simple.

The browser-based UI is simple but it's wrong. For instance if you select a group, you're likely to not run all the tests you mean to run. #2296615: Accurately support multiple @groups per test class There are many other bugs, too. #2215035: [meta] Simpletest UI cleanups

Splitting out the UI means it's both easier to deprecate and also easier to fix and keep even if we abandon run-tests.sh.

> The browser-based UI is simple but it's wrong.

Sure, but what you mean is that it's *currently* wrong, but not inherently wrong.

I was just commenting because @Gábor Hojtsy said he hadn't been given any reasons for keeping the UI.

FWIW I'm totally in favour of the cleanup roadmap @Mile23 has presented elsewhere, I just don't feel competent enough to help that much with it.

This task makes sense in the same way that we have Views vs Views UI and Field vs Field UI. Let's move the meta discussion to #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner.

Again: The meta discussion shouldn't be about the UI because it's relatively easy to fix and also relatively easy to remove. Especially with PHPUnit, maintaining the in-browser runner isn't all that tremendous a technical feat; it only requires a decision about what to do.

The meta discussion should be: Are we transitioning away from run-tests.sh in favor of something else, and what is it? Here is that meta: #3030136: The Fate Of Run-Tests.Sh

When we have that answer, we will be able to determine why we should split simpletest into two modules.

Since it's impossible to search for 'run-tests.sh' on d.o, here's a meta: #2626986: [meta] Improvements to run-tests.sh and it has the tag 'run-tests.sh.'

Reroll of #19 and minor change to (hopefully) fix the test fail.

More test passing.

Looks good. A few minor points:

1. +++ b/core/modules/simpletest/src/Tests/MissingCheckedRequirementsTest.php
   @@ -17,7 +17,7 @@ class MissingCheckedRequirementsTest extends WebTestBase {
   diff --git a/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   
   diff --git a/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   index 0aeceaf58c..45152f9f08 100644
   
   index 0aeceaf58c..45152f9f08 100644
   --- a/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   
   --- a/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   +++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   
   +++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   +++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   @@ -23,7 +23,7 @@ class SimpleTestBrowserTest extends WebTestBase {
   
   @@ -23,7 +23,7 @@ class SimpleTestBrowserTest extends WebTestBase {
      protected function setUp() {
        parent::setUp();
        // Create and log in an admin user.
   -    $this->drupalLogin($this->drupalCreateUser(['administer unit tests']));
   +    $this->drupalLogin($this->drupalCreateUser());
      }
    
   
   +++ b/core/modules/testing_ui/testing_ui.module
   rename from core/modules/simpletest/simpletest.routing.yml
   rename to core/modules/testing_ui/testing_ui.routing.yml
   

   The permission is being moved to testing_ui module, so shouldn't it stay granted to the test user here?

2. +++ b/core/modules/testing_ui/src/Form/TestingUiTestForm.php
   @@ -13,7 +13,7 @@
   -class SimpletestTestForm extends FormBase {
   +class TestingUiTestForm extends FormBase {
   

   Nitpick: core is inconsistent on Ui / UI in class names, but UI comes out narrowly ahead.

3. +++ b/core/modules/testing_ui/src/Form/TestingUiTestForm.php
   @@ -40,7 +40,7 @@ public static function create(ContainerInterface $container) {
   +   * Constructs a new testing_uiTestForm.
   

   Case problem.

Re: #36.1: SimpleTestBrowserTest only tests the behavior of the internal browser. The permission might not have been needed before splitting the modules out, but definitely is not needed now. The permission is in the ui module which isn't a requirement of this test. We do have to have a logged in user for testUserAgentValidation() so we keep the login part.

#36.2: Consistent with other class/file names in the same folder.

#36.3: Done.

Thanks!

In that case, I'm going to say RTBC.

Let's land this early in 8.8.x - there's no need to add to the 8.7.x pressure with this issue. The alpha deadline is today.

We need to update CRs hook_update_N's etc...

Big +1 to doing this.

I think we need a change notice as well.

Added CR: https://www.drupal.org/node/3038814

We will need to update the testing documentation here: https://www.drupal.org/docs/8/core/modules/simpletest

Possible follow-up would be to move the batch test runner to testing_ui, since it's the only place that uses it. That would be simpletest_run_tests() and friends, and would affect #2748967: Trigger E_USER_DEPRECATED for BC support in simpletest_run_tests()

I updated the IS in #40 so removing the tag, CR looks great, all feedback looks addressed.

Given a new module added and modules split, needs release managers.

+++ b/core/modules/testing_ui/testing_ui.info.yml
@@ -0,0 +1,9 @@
+core: 8.x
+dependencies:
+  - simpletest

What's the simpletest dependency necessary for at this point, could it be optional? Or is that a further follow-up?

@catch it has a dependency for the test_discovery service, which should probably be moved out of simpletest at some point? Quick google didn't turn up an issue specifically for that.

If you're looking for old issues dealing with refactoring the testing system to be more maintainable and that probably need a re-roll because the poster is alone in the wilderness, then just search for "mile23 [testing component here]"

#2863055: Move TestDiscovery out of simpletest module, minimize dependencies

Note that if you look through run-tests.sh and find all the simpletest_* function calls that don't start with simpletest_script_*, those are the only dependencies between run-tests.sh and simpletest module.

Similarly, the new testing_ui module doesn't use any of that for running tests, and uses the batch functions from the simpletest module.

So if we moved the batch functions to testing_ui, and we moved the dependencies from run-tests.sh to somewhere else, we'd have successfully deprecated simpletest module.

If someone will file those issues I'll gladly do them, because, as mentioned, kind of tired of being alone in the wilderness.

error: patch failed: core/themes/stable/css/simpletest/simpletest.module.css:1
error: core/themes/stable/css/simpletest/simpletest.module.css: patch does not apply
warning: core/scripts/run-tests.sh has type 100755, expected 100644
make: *** [patch88] Error 1

Reroll.

Not sure how that happened.

fwiw I don't have a strong preference on this issue vs. #2863055: Move TestDiscovery out of simpletest module, minimize dependencies landing first especially given they're both close. Makes sense to me to land this issue in 8.8.x

A new module could probably use product manager review though, although the eventual goal is to go back down to one or zero modules.

#2863055: Move TestDiscovery out of simpletest module, minimize dependencies is tagged for 8.7.x since it's a test-only change, and this one is 8.8.x since it's disruptive. It might be easier to change TestDiscovery first but not a problem either way.

Reroll looks good, back to RTBC.

Rerolled due to a conflict in simplest.module due to deprecation of \Drupal::url().

Fixed:

• the help test to refer to the new module page that will need to be created after commit - https://www.drupal.org/docs/8/core/modules/testing_ui
• the testing page link in the help text

Also fixed the following coding standards:

FILE: ...v/sites/drupal8alt.dev/core/modules/simpletest/simpletest.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 8 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...v/sites/drupal8alt.dev/core/modules/testing_ui/testing_ui.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

+/**
+ * Installs testing_ui automatically.
+ */
+function simpletest_update_8701() {
+  \Drupal::service('module_installer')->install(['testing_ui']);
+}

If this issue is 8.8-only, shouldn't we bump the update function number?

@bojanz good point - also no need for that to be a hook_update_N()

Let's add some update test coverage.

Reroll and updated update and test coverage look good, back to RTBC

1. +++ b/core/modules/simpletest/simpletest.post_update.php
   @@ -0,0 +1,13 @@
   +  \Drupal::service('module_installer')->install(['testing_ui']);
   

   we would need the test coverage for this

2. +++ b/core/modules/testing_ui/css/testing_ui.module.css
   @@ -0,0 +1,110 @@
   +<<<<<<< HEAD:core/themes/stable/css/simpletest/simpletest.module.css
   

   These needs to be resolved.

3. +++ b/core/modules/testing_ui/src/Form/TestingUiResultsForm.php
   @@ -48,7 +48,7 @@ public static function create(ContainerInterface $container) {
   +   * Constructs a \Drupal\testing_ui\Form\testing_uiResultsForm object.
   

   Nitpick: This should be TestingUiResultsForm

4. +++ b/core/modules/testing_ui/src/Form/TestingUiResultsForm.php
   @@ -197,13 +197,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $testing_ui_test_form = testing_uiTestForm::create(\Drupal::getContainer());
   

   Nitpick: The form class name should be updated.

Setting it back to N/W for point 2.

Patch addresses #60.

Re #60.1 - test coverage was added in #58 - it only needs one line :)
All the other points from #60 are addressed by this patch.

So there's a bit more work to do here. I've tested the --browser option on run-tests.sh - the attached patch fixes it but it looks very unthemed if testing_ui is not installed - I think that's okay (EDIT: I'm wrong - we linked to the uninstalled testing UI's css and js and it looks great - this is what we do in HEAD too if simpletest is not installed).

More serious is what to do about

• simpletest_run_tests()
• _simpletest_batch_operation()
• _simpletest_batch_finished()

In reality these are all testing UI functions and now simpletest_run_tests() depends on the testing UI library (this was broken in previous patches) - but moving simpletest_run_tests() is against our BC policy. I would argue strongly that no one is calling simpletest_run_tests() and http://grep.xnddx.ru/search?text=simpletest_run_tests&filename= would back me up but it is strictly against how BC policy to make this kind of break. But perhaps because this is testing functionality and not involved in runtime of a site we can make an exception here. In a way this applies to the theme and library changes the patch already is making. Anyhow because of this I've moved them in the attached patch and added the "needs release manager review" tag.

I've also hardened \Drupal\KernelTests\Core\Asset\AttachedAssetsTest::testAlter to not produce false positives as it was in previous patches.

I've also added the "Needs manual testing" tag because whilst there is test coverage of much of the UI it is by no means complete. This patch also fixes #60.4 which if @naveenvalecha had not caught would have resulted in a broken re-submission of failed tests through the UI.

Whilst reviewing #61 I noticed some more inconsistencies.

Oops missed one thing.

Another thing we need to think about is the config simpletest.settings.

Atm it looks something like this

clear_results: true
httpauth:
  method: 1
  password: ''
  username: ''
verbose: true

• clear_results is used by both run-tests.sh and the Testing UI via simpletest_clean_environment()
• httpauth is only supported by deprecated Simpletest tests - ie things that extend \Drupal\simpletest\TestBase - I think this makes the config essentially useless. We should search the issue queue and see if anyone has struggled with this whilst using PHPUnit based functional tests. Perhaps we can deprecated this configuration too.
• verbose is only used by the UI and really belongs in a testing_ui configuration. I think it is also only supported by deprecated Simpletest tests and therefore probably should be deprecated and therefore not worth moving. Note this setting is often confused with verbose error level logging and the --verbose option in run-tests.sh

Is there something stopping us deprecating simpletest_run_tests() and having it call testing_ui_run_tests()?

@catch dependencies - simpletest is a dependency of testing_ui and not the other way around. We could do that with a moduleExists check I guess because if you were using simpletest then testing_ui will be installed. However that's not how most people use it - they install simpletest for testing and then uninstall it. Or install it as part of some build process. So perhaps we need to install testing_ui in simpletest_hook_modules_installed() when simpletest is installed.

hmm what about emptying it except for a trigger_error() and deprecating it, then on the 0.001% chance that's someone's calling it they'll immediately know what happened.

@catch yep we can do that. I've triggered a deprecation when Testing UI is installed and we have a replacement and error when its not because we can't do much else. Added tests for this.

#65 is part of why #2866082: [Plan] Roadmap for Simpletest emphasizes moving functions out of modules altogether. If we do things like, for instance, #2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate then we don't create a module-to-module dependency problem. In the case of simpletest_run_tests() if we must have a batch runner then we should turn it into some classes that live in core/lib/Drupal/Core/Tests. Then simpletest module can depend on it for function-calling BC, and testing_ui can also depend on it to run the tests for the user.

@mile23 but the batch system in Drupal is really part of the FormAPI and running tests from forms is what the UI is for. So I think putting them in the new Testing UI module and not as part of Core makes sense. Another reason this is correct is this is where the asset libraries are added - you do not want to be doing this in Core.

Needed a reroll, so here we go.

Also went through this again with the mindset 'If I just delete the testing_ui dir, what breaks?' and the main thing that stood out was this:

+++ b/core/scripts/run-tests.sh
@@ -17,9 +17,9 @@
 use Drupal\simpletest\TestDiscovery;
+use Drupal\testing_ui\Form\TestingUiResultsForm;
 use PHPUnit\Framework\TestCase;

@@ -1567,7 +1567,7 @@ function simpletest_script_open_browser() {
     'system/admin',
-    'simpletest/drupal.simpletest',
+    'testing_ui/drupal.testing_ui',
   ]);

+++ b/core/scripts/run-tests.sh
@@ -1559,7 +1559,7 @@ function simpletest_script_open_browser() {
   $form = [];
-  SimpletestResultsForm::addResultForm($form, $results);
+  TestingUiResultsForm::addResultForm($form, $results);

run-tests.sh now has a dependency on two modules, is there no way to avoid that? Maybe make it optional? Give a warning when using the browser option and testing_ui is disabled?

OK, check this out.

run-tests.sh says this so it can build the results form for --browser:

  // Get the results form.
  $form = [];
  TestingUiResultsForm::addResultForm($form, $results);

That method is static specifically so that run-tests.sh can run it without having to use the whole form builder API. run-tests.sh can just call it and then render it without building a form state, but the UI can also call it and render it inside a form context.

Now, if we look within TestingUiResultsForm::addResultForm(), it says this:

    foreach ($test_results as $group => $assertions) {
      // Create group details with summary information.
      $info = TestDiscovery::getTestInfo($group);

So wherever it is that this method lives, we have a hard dependency on simpletest module, because that's where TestDiscovery lives.

But what if it didn't? What if it lived in core/lib/Drupal/Core/Test/? If it were there, then it would be autoloadable from all the places that need it, but not create a dependency on the simpletest module.

As it turns out, that work has been done, except for a reroll I'll do now: #2863055: Move TestDiscovery out of simpletest module, minimize dependencies

So that's one area where this web of dependencies is now reduced, because we refactored the code to be in a *third* place, instead of the two modules.

Now take this line of reasoning and apply it to all dependency problems we see here, such as #65 and #70. We move those things to a *third* place, namely core/lib/Drupal/Core/Test/. This way we can shim BC for simpletest module as needed, while also allowing testing_ui to remain dependency-free.

Postponing here on #2863055: Move TestDiscovery out of simpletest module, minimize dependencies which has some consensus already.

Later, we can factor out the static method TestingUiResultsForm::addResultForm() into a helper class in core/lib/Drupal/Core/Test/, which will be used by both the testing_ui form, and also run-tests.sh.

Currently, it seems as though the plan is to turn the simpletest module into a contrib module, with work happening in this meta: #3057420: [meta] How to deprecate Simpletest with minimal disruption

This has the effect of moving the test runner parts of the module into core, leaving the testing UI part and the WebTestBase framework as a contrib module.

We ended up moving simpletest to contrib, so marking this one as outdated.