Set printerClass in phpunit.xml.dist

I tried it out and sadly it doesn't work as in doesn't actually show any HTML links in phpstorm, but it does not longer fail to run the tests. Nice find @jibran!

I'd love this to happen. Massive DX improvement.

👏

Spent some time figure out the test failure. Here is my findings:

The failing test was introduced by #2809117: Integrate PHPUnit verbose output in Simpletest UI to confirm that the HTML format report generated when run tests via simpletest UI. Failing test prove(?) that use of HtmlOutputPrinter instead of SimpletestUiPrinter.

The test(\Drupal\simpletest\Tests\UiPhpUnitOutputTest) explicitly try to run sample test (in core/modules/simpletest/tests/fixtures/simpletest_phpunit_browsertest.php) using simpletest_phpunit_run_command() which explicitly sets --printer=SimpletestUiPrinter not HtmlOutputPrinter. I also tried to print the command getting exec()locally to confirm this:

/www/drupal8/vendor/phpunit/phpunit/phpunit --log-junit '/www/drupal8/docroot/sites/simpletest/29432055/files/phpunit_junit.xml' --printer 'Drupal\Tests\Listeners\SimpletestUiPrinter' '/www/drupal8/docroot/core/modules/simpletest/tests/fixtures/simpletest_phpunit_browsertest.php'

In theory, this change shouldn't cause the failure.

I have tried few scenarios:

1. Drupal\simpletest\Tests\UiPhpUnitOutputTest locally always fails (with & without this issue change). I am running it using below command:

php scripts/run-tests.sh  --verbose --url http://d8 --class "Drupal\simpletest\Tests\UiPhpUnitOutputTest

2. Thought it was Drupal CI issue and tried to run with non-functional change in phpunit.xml.dist (#1970534-164: Patch testing issue) and all green.

3. Made sure this is still an issue in current HEAD #1970534-162: Patch testing issue

Empirically (I could not found any reference proving that) I've found that printerClass in configuration file wins over --printer command line parameter.

Comment in phpunit.xml.dist suggesting to change printerClass HtmlOutputPrinter was added in #2760905: The documentation should be more explicit about PHPUnit requesting the webserver user to perform all functional tests, before SimpletestUiPrinter even existed (introduced in #2809117: Integrate PHPUnit verbose output in Simpletest UI).

Since SimpletestUiPrinter is direct successor of HtmlOutputPrinter, and only makes output formatting more readable, maybe it will make sense to set printer class to SimpletestUiPrinter? (patch attached)

Patch against latest HEAD

awesome. not sure how, but it's back to green :) as mentioned in #10, change shouldn't case any test fail. Let's get it in!

Will it make sense to remove explicit setting of printerClass from --printer CLI parameter (simpletest.module around line 337):

  $command = [
    $phpunit_bin,
    '--log-junit',
    escapeshellarg($phpunit_file),
    '--printer',
    escapeshellarg(SimpletestUiPrinter::class),
  ];

?

We already have test that checks that SimpletestUIPrinter is used

I'm sure the last testbot failure has nothing to do with phpunit.xml.dist change, but that's a good opportunity to check if removal of `--printer` option would work

Back to 'Needs review' because of aborted CI job

Looks like - - printer CLI option is not needed anymore

Looks good.

+++ b/core/phpunit.xml.dist
@@ -8,15 +8,8 @@
- - -printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" to use it (note there
...
+         printerClass="\Drupal\Tests\Listeners\SimpletestUiPrinter">

This isn't the same thing - so I think we should add some documentation here telling people they have the option of overriding this in their phpunit.xml to use the HtmlOutputPrinter

Added comment in phpunit.xml.dist

Fix wording

...and move comment closer to printerClass line (as it was in previous TODO comment)

Patch at #25 looks good to me.

@valthebald I'm not sure I agree with using SimpletestUiPrinter over HtmlOutputPrinter. The expected output from running PHPUnit is not links marked up in HTML and adding HTML does not improve readability from the CLI - I think it makes readability worse.

See attached screenshots.

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -334,8 +334,6 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
   -    '--printer',
   -    escapeshellarg(SimpletestUiPrinter::class),
   

   we should leave this as it was. run-tests.sh manages stripping the html as appropriate.

2. +++ b/core/phpunit.xml.dist
   @@ -8,14 +8,21 @@
   +         printerClass="\Drupal\Tests\Listeners\SimpletestUiPrinter">
   

   This should be HtmlOutputPrinter and not SimpletestUiPrinter

Discussed in slack with @valthebald. Unfortunately in PHPUnit 4.8 once the printer class is set in the phpunit.xml.dist it can't be overridden from the command line. So setting class to SimpletestUiPrinter::class in simpletest_phpunit_run_command() won't work. However this is fixed in PHPUnit 6.5 so we will be able to do once PHPUnit 4 is dropped from core. That might happen if we decide to test PHP 7.0 and PHP 7.1 using PHPUnit 6 as well and once we drop support for PHP 5.5 and 5.6.

For more about all of that see #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing and its related issues.

Woah … my head is spinning with those competing requirements … 😱 🤯

Also of note, https://youtrack.jetbrains.com/issue/WI-24808 is no longer a blocker either. And the postponed issue is also closed. So... let's keep things moving.

We'll still be use PHPUnit 4 until we drop PHP 5.5. That's not happened yet.

However this is fixed in PHPUnit 6.5

Just a minor point, and maybe you actually meant it that way, but I think it was fixed in 5.0 already.

See https://github.com/sebastianbergmann/phpunit/blob/49f1c93ee37d10ffba6ce2... or https://github.com/sebastianbergmann/phpunit/commit/b191e125cb130427b396...

@tstoeckler good spot.

But we're not going to use PhpUnit 5 :)

Rare case where "postponed" status has clear "until" date - March 2019 :)

Cool, so let's make that clear in the issue title for when people scan their issue tracker!

Marked #3028974: Enable printerClass in phpunit.xml.dist as duplicate

Is this pending on the opening of 8.8 or can it land in 8.7? If it is the later, this can land now. Let's make this more explicit.

8.8 unfortunately.

8.7 will have best effort PHP5 support.

@alexpott: Given that 8.7 won't be out to May, would you mind clarifying why this can't be changed before then?

Hit this again today... now that #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x) is (just!) in we can finally do this. Yay!

+++ b/core/phpunit.xml.dist
@@ -8,14 +8,21 @@
+<!--
+ Drupal ships with several PHPUnit output printers.
+ SimpletestUiPrinter (used by default) links to the html results for functional tests.
+ Set printerClass="\Drupal\Tests\Listeners\HtmlOutputPrinter" if you don't need automatic HTML links in output.
+ If you use older PHPStorm versions (prior to 2016.2.2), and use built-in PHPUnit runner,
+ you need to remove printerClass line from this file, because of a bug with custom printerClass
+ breaking the output of PHPStorm's PHPUnit runner.
+ See https://youtrack.jetbrains.com/issue/WI-24808 for details.
+ To use custom printer class only in CLI tests, remove printerClass line from this file,
+ and add these 2 lines to $commands array in simpletest_phpunit_run_command() function:
+   '- -printer',
+   escapeshellarg(SimpletestUiPrinter::class),
+ (note there should be no spaces between the hyphens).
 -->

This feels like TMI now - talking about PHPStorm 2016.2.2 in 2019...

Also this text should wrap at 80.

I agree. I also agree that HtmlOutputPrinter makes more sense than SimpletestUiPrinter by default.

Here's is a minimal patch, analogous to #2

Crediting @vijaycs85 for testing the patch and the explanatory comment in #10.
Crediting @larowlan for reviewing and asking for HtmlOutputPrinter

Committed 0b95d68 and pushed to 8.8.x. Thanks!