Integrate PHPUnit verbose output in Simpletest UI

Baby steps: #2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate

Could use a test or two... There's Drupal\simpletest\Tests\SimpleTestTest which might help with test-ception.

I was unable to repro the effect but I think it has more to do with my machine configuration than the patch.

Nice. Here's a basic review.

1. +++ b/core/modules/simpletest/src/Tests/UiPhpUnitOutputTest.php
   @@ -0,0 +1,43 @@
   +    $phpunit_junit_file = \Drupal::service('file_system')->realpath('public://phpunit_junit.xml');
   ...
   +    \Drupal::service('file_system')->mkdir('public://simpletest');
   

   Use $this->container instead of \Drupal

2. +++ b/core/tests/Drupal/Tests/Listeners/SimpletestUiPrinter.php
   @@ -0,0 +1,26 @@
   +/**
   + * Defines a class for providing html output links in the Simpletest UI.
   + */
   +class SimpletestUiPrinter extends HtmlOutputPrinter {
   

   Add @see for UiPhpUnitOutputTest.

I really like the idea!

+++ b/core/modules/simpletest/simpletest.module
@@ -331,6 +331,7 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
+    putenv('BROWSERTEST_OUTPUT_DIRECTORY=' . \Drupal::service('file_system')->realpath('public://simpletest'));

Are we sure we want to add this also for the case of running it via the command line?

One other issue we found out with @klausi when trying this patch : the user-customized version of the phpunit.xml is used by default, which can make or break the UI behavior. For the UI tests, it is apparently necesssary to use phpunit -c phpunit.xml.dist to force use of the default, hopefully non-customized version of the PHUnit config file, instead of relying on the standard behavior which gives precedence to the customized version, likely not to be appropriate for the UI-run tests.

@klausi asked on #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner if someone would test this patch on, for instance, a failing version of ColorSafePreviewTest and see what happens. As someone who has been affected by this bug, I was glad to oblige. :)

I tested this in 8.2.x that was updated a while back in the dev cycle, so the patch encountered some "fuzz" to apply, but I didn't have a 8.3.x environment handy, so I hope that is OK.

So, what I did was change the line in ColorSafePreviewTest::testColorPreview() that says

     $this->assertRaw('<h2>TEST COLOR PREVIEW</h2>');

to

     $this->assertRaw('<h2>TEST COLOR PREVIEW with additional text</h2>');

so that the test would fail.

The output I got was pretty much exactly like the "before" picture in the issue summary. I did not get HTML verbose output like in the "after" picture. Do I need to do something else to make that happen? I do have the verbose setting in Simpletest turned on. Or do I need to be running 8.3.x or a more updated 8.2.x?

I did not have a custom phpunit.xml file to my knowledge. Where would that be? I mean, if some script created one when I first ran a PHP Unit test, it is possible I have an old one lying around...

Typically, you get to create one by copying core/phpunit.xml.dist to core/phpunit.xml when configuring your site to run tests on the CLI, and you then edit that file to customize the Simpletest DB URI and other things.

Also volunteering to test.

My environment is 8.3.x (not the latest, from some weeks ago) and a customized phpunit.xml. The patch applied cleanly and the output I have is:

The links at the bottom are not shown as links, but the URLs work and show the correct snapshots of each step.

Test done with Klausi sunday:

• I have a custom phpunit.xml
• On 8.3.x HEAD
• with the custom phpunit.xml, patch produces no visible result
• when removing the phpunit.xml from core/, patch works as expected
• when replacing the custom phpunit.xml in core/, patch again produces no visible result

The important line causing a change seems like it could be adding the custom output plugin.

Some of the way this works might be altered by #2810083: Duplicate test results per fail/exception which helps fix a testbot issue.

+++ b/core/tests/Drupal/Tests/Listeners/SimpletestUiPrinter.php
@@ -0,0 +1,26 @@
+  public function write($buffer) {
+    $buffer = Html::escape($buffer);
+    // Turn HTML output URLs into clickable link <a> tags.
+    $url_pattern = '@(http)(s)?(://)?([^\s]+)@';
+    $buffer = preg_replace($url_pattern, '<a href="http$2://$4" target="_blank" title="$0">$0</a>', $buffer);
+    // Make the output readable in HTML by breaking up lines properly.
+    $buffer = nl2br($buffer);
+
+    print $buffer;

Conceptually this is the same as \_filter_url isn't it? I'm wondering whether moving this int its own component would be worth doing.

In general we could probably improve that more and more. This is a good start for now.

Since it is only 2 lines of code I think it is not worth having in a component. _filter_url() is way more complex and does stuff we don't care about here.

Fair

Sorry for the delay in reviewing this -- it looks great. About to test it manually but meanwhile I have one small nitpick:

+++ b/core/tests/Drupal/Tests/Listeners/SimpletestUiPrinter.php
@@ -0,0 +1,26 @@
+    $url_pattern = '@(http)(s)?(://)?([^\s]+)@';

Is the (://)? grouping really optional? Under what circumstance is a URL without it but starting with http going to be correct?

A followup for the filtering question might be worthwhile. I'm okay with hardcoding the parsing and filtering in this initial patch (and maybe always, but it is worth discussing later).

Also it's not clear to me if this was actually tested again with a custom phpunit.xml after @klausi's fixes. And a test-only patch for a failing browser test, separately and then rolled with this one, might be helpful to show how it's being reported on DrupalCI (or that it's not changing on DrupalCI if that's the case).

Setting NR to see if those suggestions make sense. Thanks! I'll keep an eye on this issue so it doesn't get stuck in a backlog again.

Here is the phpunit.xml file I use:

  <php>
    <!-- Set error reporting to E_ALL. -->
    <ini name="error_reporting" value="32767"/>
    <!-- Do not limit the amount of memory tests take to run. -->
    <ini name="memory_limit" value="-1"/>
    <!-- Example SIMPLETEST_BASE_URL value: http://localhost -->
    <env name="SIMPLETEST_BASE_URL" value="http://d8.dev"/>
    <env name="SIMPLETEST_DB" value="mysql://root@localhost/loc_d8"/>
    <!-- Example BROWSERTEST_OUTPUT_DIRECTORY value: /path/to/webroot/sites/simpletest/browser_output -->
  </php>

Without patch: https://www.drupal.org/files/issues/Screen%20Shot%202016-11-14%20at%2008...

With patch (and no custom phpunit.xml): https://www.drupal.org/files/issues/Screen%20Shot%202016-11-14%20at%2009...

With patch (and custom phpunit.xml): https://www.drupal.org/files/issues/Screen%20Shot%202016-11-14%20at%2009...

Oh I wanted to RTBC it

So we see the verbose output:

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-940.xml:
PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

F

Time: 14.76 seconds, Memory: 2.75Mb

There was 1 failure:

1) Drupal\Tests\help\Functional\ExperimentalHelpTest::testExperimentalHelp
Failed asserting that true is false.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:43
/var/www/html/core/modules/help/tests/src/Functional/ExperimentalHelpTest.php:54

FAILURES!
Tests: 1, Assertions: 9, Failures: 1.

HTML output was generated
http://localhost/checkout/sites/simpletest/browser_output/Drupal_Tests_help_Functional_ExperimentalHelpTest-1-70400662.html
http://localhost/checkout/sites/simpletest/browser_output/Drupal_Tests_help_Functional_ExperimentalHelpTest-2-70400662.html
http://localhost/checkout/sites/simpletest/browser_output/Drupal_Tests_help_Functional_ExperimentalHelpTest-3-70400662.html
http://localhost/checkout/sites/simpletest/browser_output/Drupal_Tests_help_Functional_ExperimentalHelpTest-4-70400662.html
http://localhost/checkout/sites/simpletest/browser_output/Drupal_Tests_help_Functional_ExperimentalHelpTest-5-70400662.html
http://localhost/checkout/sites/simpletest/browser_output/Drupal_Tests_help_Functional_ExperimentalHelpTest-6-70400662.html

but well, that's of course not accessible to the outer world. I think the feedback of @xjm got addressed.

Thanks @dawehner, that looks great!

The point from #31 is still not addressed I think. I think this:
$url_pattern = '@(http)(s)?(://)?([^\s]+)@';
should be this:
$url_pattern = '@(http)(s)?(://)([^\s]+)@';

Otherwise it would match httpwww.example.com.

But this is a really minor thing.

Oh also, do we need a followup?

A followup for the filtering question might be worthwhile. I'm okay with hardcoding the parsing and filtering in this initial patch (and maybe always, but it is worth discussing later).

From #32 about #28:

Conceptually this is the same as \_filter_url isn't it? I'm wondering whether moving this int its own component would be worth doing.

Otherwise it would match httpwww.example.com.

Good point, let's change that. Well ideally we would use a protocol relative URL and be done with it.

Well, I don't think its important to actually use _filter_url. Its a different usecase, as in full text. We have a limited subset of HTML here, to be honest. Its good here IMHO to not share code. At the end its just a random testing UI.

Cool, tests are still passing

Ah yes, that's much simpler.

Committed and pushed to 8.3.x. Thanks!