Decouple run-tests.sh from the simpletest module

All changes looks good for me, but found the following one:

+++ b/core/lib/Drupal/Core/Test/RunTests/RunTestsResultsBuilder.php
@@ -0,0 +1,173 @@
+        $group_summary['#' . $assertion->status] ++;
+        $form['result']['summary']['#' . $assertion->status] ++;

Should be there a space before unary operator statement?

Great!
+1 for rtbc

The code changes look reasonable: run-tests.sh is changed minimally, and some logic is moved out of a form into a @internal helper class.

This is what I wanted to write: I tested this manually. For the first time ever, I can do a fresh D8 install, run run-tests.sh, without having to install the simpletest module. I must've run into that hundreds of times over the past decade, so it's wonderful to finally have that fixed! 🥳

But … the reality is that this patch doesn't seem to work for me, or I misunderstood the purpose:

wim.leers at Acquia in ~/Work/d8 on 8.8.x*
$ php core/scripts/run-tests.sh --directory modules/jsonapi_hypermedia
  ERROR: Missing Simpletest database schema. Either install Simpletest module or use the --sqlite parameter.

What am I missing?

#26 have not tried myself, but if simpletest is not installed, then its db schema is not available. This is where the test run results are stored, and that is a must have, one way or another. The 'another' in this case would be to instruct run-tests.sh to create a separate SQLite database through the --sqlite option, and results will be stored in that db. This is AFAICS what the testbot on DrupalCI is doing. If you want a sneak peek to the future, #3075608: Introduce TestRun objects and refactor TestDatabase to be testable :)

Aha, so I misunderstood the scope and intent of this patch — my bad!

Well if that is the scope of this issue, then the title should probably be something like:
'Allow run-tests.sh to run with --sqlite when Simpletest module is not present'

I think that would be a great first step but it would be better is you could also run it with a mysql database without the Simpletest module. I took a quick look at what that would take, and getting the tests to run is pretty easy

  if ($new && $sqlite) {
    foreach (TestDatabase::testingSchema() as $name => $table_spec) {
      try {

can be changed to

  if ($new) {
    foreach (TestDatabase::testingSchema() as $name => $table_spec) {
      try {

and the tests will run with mysql without Simpletest. The problem is clean up. With just that change, you will be stuck with simpletest tables without a way to get rid of them. Didn't immediately see a way to clean them up, the sqlite file isn't cleaned up either.

But the main thing I wanted to test is that the result printer still worked with the --browser option and simpletest disabled, and it does.

edit: wanted to make it clear that scoping this to just --sqlite seems fine to me!

@Mile23 thanks for the feedback and IS update, all clear

So:

• make sure Drupal CI still runs
• make sure tests can be run without the simpletest module installed with --sqlite option
• make sure the --browser options keeps working without the simpletest module installed

All boxes checked I think, so this looks ready.

🥳 Manually tested with --sqlite, works great :)

We're still adding CSS and JS from the simpletest module with run-tests.

I wonder if we're going about this the wrong way around. Like maybe only offer the --browser stuff if simpletest is still around. I used to use it more when we didn't output of all the verbose output as links but now I don't ever use it - \Drupal\Tests\Listeners\HtmlOutputPrinter has replaced the need. This makes me think we should deprecate the --browser option instead of maintaining all this code in core.

@Mile23 I disagree with

only has two useful structured reports

- it has one useful structured report - the --xml option and two ways that the user can see what has failed --browser and --verbose. I really think we should take the opportunity to deprecate the the --browser option here. It really only was added to make it easier to get at the HTML produced during a test and now we have the \Drupal\Tests\Listeners\HtmlOutputPrinter for that.

I think we should take the opportunity to remove functionality from run-tests.sh. In Drupal 9 I'd like to see us only use it for concurrent testing and recommend the people and most CI systems use PHPUnit directly. It'd be great for it to be doing less.

@Mile23 I'm confused - I think the reason why you can't see which function passed or failed is because DenyNodePreviewTest uses a data provider and the --browser option is not going to provide you any more information as far as I know. Data provider tests have always been badly represented in --verbose and --browser output. Which for me is yet another reason to try to move towards PHPUnit test reporting and away from our custom stuff.

For me this critical issue is choosing to move a load of code to core that once it is there will be hard to remove and I'm not sure this code belongs in core.

I've created a change record - https://www.drupal.org/node/3083549

@Mile23 Ah I see what you are saying... the issue is with the cut off output. Ie. Drupal\Tests\node\Unit\PageCache\De and not \Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest::testPrivateImageStyleDownloadPolicy(). That is a good point. I'd still argue that really we should be telling people to use the phpunit from the CLI - because as the DenyNodePreviewTest shows the output using --browser or the CLI from run-tests.sh is unsuitable because it doesn't tell you what data has been provided.

There's an issue with the @internal stuff though.

+++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
@@ -70,40 +70,7 @@ public function __construct(Connection $database, EnvironmentCleanerInterface $c
+    return RunTestsResultsBuilder::buildStatusImageMap();

We've marked this internal but we're using it in a module that we plan to remove from core. That's breaking the idea of something being internal.

Trying to strike a compromise here, what if we add a --table option which outputs something better than the current sprintf approach, and then retain but deprecate the --browser option and have it trigger an error but then fallback to --table

Then we're doing something close to what the title of the change-record suggest, 'deprecating' rather than 'removing' - albeit where 'deprecating' means 'a fallback'. We're also closer to what we'll likely do if we were to finalise #2624926: Refactor run-tests.sh for Console component. as we'd likely use the table helper there too.

Looks like so

 php ./core/scripts/run-tests.sh --url http://127.0.0.1/ --dburl mysql://drupal:drupal@127.0.0.1:3306/local --table --sqlite /tmp/test.sqlite --types PHPUnit-Unit node

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

Tests to be run:
  - Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest
  - Drupal\Tests\node\Unit\Plugin\views\field\NodeBulkFormTest

Test run started:
  Thursday, September 26, 2019 - 21:27

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

Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest           7 passes
Drupal\Tests\node\Unit\Plugin\views\field\NodeBulkFormTest     1 passes

Test run duration: 40 sec

Detailed test results
---------------------
+--------+-------+-------------------------+---------------+--------------------------------------------------------------------------------------------------------------+
| Status | Group | Filename                | Line Function |                                                                                                              |
+--------+-------+-------------------------+---------------+--------------------------------------------------------------------------------------------------------------+
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #0() |
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #1() |
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #2() |
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #3() |
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #4() |
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #5() |
| Pass   | Other | DenyNodePreviewTest.php | 58            | Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest->testPrivateImageStyleDownloadPolicy with data set #6() |
| Pass   | Other | NodeBulkFormTest.php    | 29            | Drupal\Tests\node\Unit\Plugin\views\field\NodeBulkFormTest->testConstructor()                                |
+--------+-------+-------------------------+---------------+--------------------------------------------------------------------------------------------------------------+

Looks like so when Drupal.org doesn't mangle it :)

1. I love the new --table option. The dependency it adds is Symfony console and we already use it in core and we want to make run-tests.sh a console command at some point anyway so great! Nice work @larowlan.

2. +++ b/core/scripts/run-tests.sh
   @@ -178,11 +174,9 @@
    // Display results before database is cleared.
    if ($args['browser']) {
   -  simpletest_script_open_browser();
   -}
   -else {
   -  simpletest_script_reporter_display_results();
   +  trigger_error('The --browser option is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use --table instead. See https://www.drupal.org/node/3083549\', E_USER_DEPRECATED');
    }
   +simpletest_script_reporter_display_results($args['browser'] || $args['table']);
   

   I think we could leave simpletest_script_open_browser() around for the rest on D8's life cycle and only remove in D9. Because that's when Simpletest is leaving core.

3. +++ b/core/scripts/run-tests.sh
   @@ -317,10 +311,13 @@ function simpletest_script_help() {
   +              This enforces --keep-results
   
   @@ -449,7 +447,7 @@ function simpletest_script_parse_args() {
   +  if ($args['browser'] || $args['table']) {
        $args['keep-results'] = TRUE;
      }
   

   The --table option doesn't need the keep results option.

+++ b/core/scripts/run-tests.sh
@@ -446,10 +446,6 @@ function simpletest_script_parse_args() {
-  if ($args['browser'] || $args['table']) {
-    $args['keep-results'] = TRUE;
-  }

--browser does need keep-results as far as I know. Otherwise the html links won't work.

ah I see. I think if you pass in --browser then we should call simpletest_script_open_browser() - there's no point in breaking people's expectations in D8 - and now we're telling them how to set new expectations and use --table. That's how deprecation is supposed to work.

So I've done some testing with #52 and unfortunately the only situation is works for is passing PHPUnit tests. If a PHPUnit test fails or you run Simpletests the output is not as you'd like it.

Whilst fixing this I've realised that the new --table switch is not necessary. We should fix --verbose output here and be done. The verbose output is not an API and as @Mile23 has shown there are many situations is it totally problematic for. Funnily enough though the one situation is does work for where --browser fails for is where there is a fail because the current behaviour is to output the PHPUnit output so the user can see what has happened.

Here are the test cases I ran through:

Simpletest

sudo -u _www php ./core/scripts/run-tests.sh --non-html --url http://drupal8alt.test/ --verbose --directory core/modules/simpletest/src/Tests the output with the patch looks like this.

PHPUnit success

sudo -u _www php ./core/scripts/run-tests.sh --non-html --url http://drupal8alt.test/ --verbose --types PHPUnit-Unit node the output looks like this:

PHPUnit fail

sudo -u _www php ./core/scripts/run-tests.sh --non-html --url http://drupal8alt.test/ --verbose --types PHPUnit-Unit node where I've hacked \Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest::providerPrivateImageStyleDownloadPolicy to fail some of the time - the output looks like this:

I tested the patch on TravisCI with some combinations of run-test.sh, https://travis-ci.org/mondrake/d8-unit/builds/590486099

Results are nice.

1) I get a PHP Notice: Undefined index: debug in /home/travis/drupal8/core/scripts/run-tests.sh on line 1342 error. I think that we need to add a 'debug' key to the $result_map in simpletest_script_reporter_init:

function simpletest_script_reporter_init() {
  global $args, $test_list, $results_map;

  $results_map = [
+  'debug' => 'Debug',
    'pass' => 'Pass',
    'fail' => 'Fail',
    'exception' => 'Exception',
  ];

2) when one of the lines of the detailed log is very long the entire table gets weird at reading (see the SimpleTestInstallBatchTest test in the link for example)

3) do I understand correct since #39 we are no longer moving any code from the simpletest module and that suffices for the scope here?

EDIT:

4) Also, in case of failing PHPUnit tests I was kind of expecting the failure log be part of the table, but it's appended after the table in fact - see for example the failing Entity tests in https://travis-ci.org/mondrake/d8-unit/builds/590494824

@Mile23 #37 / #55 is still not a solution though. Moving something to core and making in @internal and then planning to move it's usages to contrib is not a plan. #54 is completely fixable. The --verbose results are already compromised and #53 makes it better. Also --browser formatting is very compromised for PHPUnit tests - which by the way are the only tests in Drupal 9 core - so maintaining the feature for Drupal 9 core makes little sense.

What if, instead of moving simpletest code to RunTestsResultsBuilder, we duplicate it in run-tests.sh? That way we could decouple the two things now and do whatever refactors of the two things separately later.

I thought about #59 too.

So I'm going to repeat myself here and question the statements that @Mile23 makes about --browser being useful with --concurrency. If a test fails and you have --verbose on you see exactly where it fails regardless of --concurrency. Yes the output is not the best for successful PHPUnit tests but the important information is there ie. you know that the test has passed and if it has failed exactly where and how. Note that PHPUnit output does not even tell you what test method has passed in visual output to the CLI because this information just noise. Therefore I stand by claims that even with --concurrency the --browser is not actually useful.

Also with respect to process we are totally allowed to remove a "feature" (though I really don't regard --browser as one) from Drupal 9 and deprecate it without replacement in Drupal 8. However I contend that there is a replacement in Drupal 9 - it's called the Simpletest in contrib module which can come with its own run-tests.sh - actually it'll need to because the core will no longer run Simpletest tests because core has none.

I suggest that this issue deprecates the --browser flag and we can improve / fix the bugs in --verbose as and when we choose because they have existing even since we integrated PHPUnit.

Here's visual proof of what I'm saying... running the command sudo -u _www php ./core/scripts/run-tests.sh --color --non-html --url http://drupal8alt.test/ --verbose --concurrency 2 --types PHPUnit-Unit node where DenyNodePreviewTest as been hacked to fail.

#59, in practice. No interdiff, this takes bits of #55 and bits of #53.

--browser is deprecated as an option, as well as the functions in run-tests.sh that support it. Code is duplicated from simpletest. So we can refactor the run-test.sh --verbose output later, independently from the fate of the simpletest module.

WRT #63.3, how about duplicating the CSS too, adding maybe an entry in system.libraries? The JS is totally irrelevant here, it seems - it is needed for the tests list form, not for the test results one

#63.2 done (I do not understand why it's better, but no problem)

I have some reservations / tweaks I think we should make but as it’s the weekend and I’m on my phone I can’t add them right now. I will comment first thing tomorrow.

Thanks Mondrake! Couple of questions, feel free to push back if these are not reasonable

1. +++ b/core/scripts/run-tests.sh
   @@ -831,7 +834,10 @@ function simpletest_script_run_one_test($test_id, $test_class) {
   +    // If we aren't running a PHPUnit-based test, then we might have a
   +    // Simpletest-based one. Ensure that: 1) The simpletest framework exists,
   +    // and 2) that our test belongs to that framework.
   +    elseif (class_exists('Drupal\simpletest\TestBase') && is_subclass_of($test_class, 'Drupal\simpletest\TestBase')) {
   

   Is there a way here we can also trigger a deprecation error if the class doesn't exist - something like 'your tests rely on simpletest and this will be moved to contrib in 9.0.0 - please move to WTB or declare a test_dependency on simpletest module?'

2. +++ b/core/scripts/run-tests.sh
   @@ -1494,6 +1500,14 @@ function simpletest_script_load_messages_by_test_id($test_ids) {
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. This function
   + *   supports the --browser option in this script. Use the --verbose option
   + *   instead.
   

   should we be triggering a deprecation error here too?

3. +++ b/core/scripts/run-tests.sh
   @@ -1570,3 +1587,250 @@ function simpletest_script_open_browser() {
   +function simpletest_script_add_result_form(array &$form, array $results) {
   

   should we make these 3 anonymous functions inside simpletest_script_open_browser so no-one else can call them?

Re #69.1 this gets to something I've been pondering this weekend. If simpletest is moving to contrib then either (a) run-tests.sh in Drupal 9 needs to be able to run Simpletest tests or (b) DrupalCI needs to support different test runners per test type. The simpletest solution is to suppose that (a) to be true for now. I've tried to address this point and be able to inform the user but adding a new failure mode when simpletest_script_run_one_test() cannot run a test.

Re #69.2 I don't think so we already have an unsilenced (so users actually see it) deprecation error. Adding a silenced deprecation error here is unlikely to ever be caught because deprecation handling is set up at a lower level in the test runner than this.

Re #66 if we're going to deprecate simpletest_script_open_browser() - which I'm totally in favour of. Then we don't need to bring all the functionality into run-tests.sh. The simpler and less complex thing to do is to move the functionality into the simpletest module. This also answers #69.3

+++ b/core/scripts/run-tests.sh
@@ -1494,79 +1502,21 @@ function simpletest_script_load_messages_by_test_id($test_ids) {
+  trigger_error('In order to use the --browser option the Simpletest module must be available. See https://www.drupal.org/node/3030340.');

I think this should be referencing [#3083549] and not the WebTestBase deprecation?

@Lendude good point. Leaving at rtbc as this is only fixing text.

+++ b/core/scripts/run-tests.sh
@@ -450,6 +445,7 @@ function simpletest_script_parse_args() {
+    trigger_error('The --browser option is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use --verbose instead. See https://www.drupal.org/node/3083549', E_USER_DEPRECATED);

@@ -843,6 +845,12 @@ function simpletest_script_run_one_test($test_id, $test_class) {
+      trigger_error(sprintf('Can not run %s. If this is a WebTestBase test the simpletest module must be installed. See https://www.drupal.org/node/3030340', $test_class));

@@ -1494,79 +1502,21 @@ function simpletest_script_load_messages_by_test_id($test_ids) {
+  trigger_error('In order to use the --browser option the Simpletest module must be available. See https://www.drupal.org/node/3083549.');

Nitpick: instead of trigger_error, why not using simpletest_script_print to echo the information to CLI? trigger_error is meant to capture deprecation errors in tests - but here we would log the message in error.log (do we need that?), and the output on CLI is not very nice (see example below). run-tests.sh is not tested itself.

$ php core/scripts/run-tests.sh --verbose --color --file --php $(which php) --browser --url http://127.0.0.1:8080 core/modules/simpletest/src/Tests/BrokenSetUpTest.php

PHP Deprecated:  The --browser option is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use --verbose instead. See https://www.drupal.org/node/3083549 in /home/travis/drupal8/core/scripts/run-tests.sh on line 453

Deprecated: The --browser option is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use --verbose instead. See https://www.drupal.org/node/3083549 in /home/travis/drupal8/core/scripts/run-tests.sh on line 453

@mondrake good call. The output with simpletest_script_print_error() is not that much better - it is indented for whatever reason. But it's not duplicated and there's a tonne of precedent in the file.

This means that we should follow deprecation policy and do a silenced @trigger_error in simpletest_script_open_browser() just in case.

Again as this is only touching on the display of messages leaving at RTBC.

So on #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner I indicated I was alright with simpletest moving to contrib and core not providing a UI test runner so long as verbose output continued to be provided by the core test runner. I'd say that's an essential part of the core featureset for the test runner.

If that's being deprecated here I have strong concerns with that.

verbose output continued to be provided by the core test runner

If that's being deprecated here I have strong concerns with that.

--verbose is not deprecated. Verbose output is not being deprecated. The ability to open it in a browser window is.

Updated the issue summary to be inline with the latest patch.

Added an extra line to the issue summary to make it clear that --verbose gives you links to HTML, so the only thing we lose from --browser is it opening the actual browser in the site under test for you. But since these are equivalent for actually figuring out what went wrong, we're not losing much of anything by deprecating it. Discussed this with @xjm and we're both agreed this is good to go.

run-tests.sh also uses Drupal\simpletest\Form\SimpletestResultsForm::addResultForm(), so that it can build the results form for its --browser option. This should be turned into a lightweight form element builder somewhere within the Drupal\Core\Tests\ namespace, so that it can be used by both run-tests.sh and simpletest as contrib.

We can remove this from the IS right?

Back to RTBC. I'm arbitrarily removing the Needs product manager review tag because we're removing a CLI option that is already de-facto deprecated by another CLI option. Added it to #3075490: Move simpletest module to contrib which is the actual issue that's removing all the stuff we're preparing for here and elsewhere.

per @Lendude in #84

--verbose is not deprecated. Verbose output is not being deprecated. The ability to open it in a browser window is.

This is unclear. What use is HTML if it cannot be opened in a browser somehow? Is the output still HTML?

When I said to @Mile23 that what was important to me was that core continue to provide verbose output, and present links to said verbose output in the run_tests.sh output, he implied (but did not outright say I guess) that this functionality was being deprecated. If core retains the ability to output a list of verbose output HTML files on the file system, which can be opened in a browser with copy and paste, and if the UI test runner in SimpleTest module itself also continues to work with links available to this output, that's fine. However, if an actual major feature is being removed from the module, that's what will (always) need product manager review.

Links to the pages generated by the system under test during a test are available if printerClass="\Drupal\Tests\Listeners\HtmlOutputPrinter" is set - which is now the default! Yay #2870145: Set printerClass in phpunit.xml.dist.

Unfortunately --browser has never been able to provide links to these generated by PHPUnit tests (i.e all browser tests apart from the Simpletest module's test now in core). --browser (and run-tests.sh in general) is not all great for PHPUnit test output.

Compare what happens when you do (on HEAD):
php ./core/scripts/run-tests.sh --color --non-html --url http://drupal8alt.test/ --verbose --class '\\Drupal\\Tests\\node\\Functional\\NodeAdminTest'
php ./core/scripts/run-tests.sh --color --non-html --url http://drupal8alt.test/ --verbose --browser --class '\\Drupal\\Tests\\node\\Functional\\NodeAdminTest'

versus
./vendor/bin/phpunit -v -c ./core core/modules/node/tests/src/Functional/NodeAdminTest.php

If your phpunit.xml.dist has been properly set up you'll get browser output whilst running PHPUnit. Neither of the run-tests.sh versions are able to link the test browser output. This patch has no impact on the ability to get to the html generated by the test.

I'm putting my foot down a little bit here because core maintainers have been sleeping on this problem for four years*

This is hugely insulting, BTW, and unnecessarily combative. Core maintainers work on a lot of stuff, and a response from one core maintainer shouldn't be confused with signoff on a plan that hasn't been reviewed and signed off by relevant stakeholders.

Yes, there's a lot of technical debt in SimpleTest. We had a whole initiative to clean up that technical debt and among that initiative's requirements were to retain the features of the current workflow for PHPUnit tests instead of WTB tests.

From my perspective, these issues are major task, like any other major tasks to clean up significant technical debt. There won't be any security, data ingerity, or upgrade path implmications if we retain the features in core. There's no depdnencies EOLing their security coverage or aside from phpunit forcing thei ridiculous schedule on us. https://www.drupal.org/core/issue-priority#critical outlines when an issue or chain of issues should be critical.

TLDR, I think this suite of issues should probbaly be marked major, not critical. If it's not done in time for 8.9/9.0 that's kinda a bummber, but it it still is acceptable.

For review: drup

This is unclear. What use is HTML if it cannot be opened in a browser somehow? Is the output still HTML?

I think this was a crosspost but just to confirm:

--browser was literally opening a browser window to the page in question on the under-test simpletest site. --verbose provides a link to the stored HTML of the page.

Agreed on issue priority, this is more rightly major alpha-deadline than critical - even if it'd be a massive shame if we can't remove Simpletest for 9.x it's not actually blocking the 9.x release or causing other critical issues.

Untagging 'Needs change record update' since the change record is accurate.

Since the actual proposal here is OK, and the patch is also OK... Committed 1b0ae80 and pushed to 8.8.x. Thanks!