Add functionality to store browser output to BrowserTestBase

What about the use of logging API as replacement?

#2664150: Expand BrowserTestBase with error handling support implements this in the meantime

Oh nevermind, I cannot read.

I think the following small bit would be neat for some people (see interdiff.txt).

As for now we don't have any support for CSS, because well, we link to files in a temporary directy and not some files directory form within Drupal.
Do you think there is any chance we could do that? I mean at that point we certainly have some form of installed Drupal, this is what BrowserTestBase is doing, right?

1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
   @@ -362,6 +390,12 @@ protected function setUp() {
   +      $this->verboseCounter = max(1, (int) file_get_contents($this->verboseCounterStorage));
   

   Just some brainstorming: What would we do in a future world where we might want to leverage parallel test running?

2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
   @@ -500,6 +534,12 @@ protected function drupalGet($path, array $options = array()) {
   +    $verbose = 'GET request to: ' . $url .
   +      '<hr />Ending URL: ' . $this->getSession()->getCurrentUrl();
   +    $verbose .= '<hr />' . $out;
   +
   +    $this->verbose($verbose);
   

   Do you plan to implement support for HTTP headers as well? IMHO we should print them simply all the time. They could be often helpful.

This is a test-only change it can go when it is ready.

So I'm still wondering whether we could write files to sites/default ... for browser tests at least we certainly have the actual site URL as environment variable available.

I'm confused, environment variables should be available always, as they are defined in the environment, so even outside of the PHP process.

I'm totally happy with coupling that, but we could introduce another ENV variable, if we really care.

@larowlan++

This is lovely!!!!!!!!!!!!

Tried out the test a bit more:

* It doesn't support phpstorm, sadly. You see

Notice: Trying to get property of non-object in /private/var/folders/9b/073w6tbx1tgfnhzsx7_4ps_w0000gn/T/ide-phpunit.php on line 262

Notice: Trying to get property of non-object in /private/var/folders/9b/073w6tbx1tgfnhzsx7_4ps_w0000gn/T/ide-phpunit.php on line 263
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

and no URLs appeared. Sadly I have no idea why, but it seems to be \IDE_Base_PHPUnit_TextUI_Command::handleArguments is never called.

This already is a great step forward so I would say +1 when we agree with the limitations for now and maybe iterate on them later.

Well, they kinda try to wrap existing ones, but yeah I fear we cannot deal with it.

For now this is a huge improvement, IMHO.

Shouldn't the test id be part of the verbose directory?

What is the concept of a test ID when you execute stuff via phpunit?

We discussed this issue today with @alexpott, @effulgentsia, @Cottser, and myself, but didn't come to a clear decision on its status.

So per https://www.drupal.org/core/d8-bc-policy:

Tests
The contents of automated tests provided by core modules are never considered part of the API. While the testing framework itself may be treated as an API individual test classes are always internal.

In general, I'd consider BrowserTestBase to be part of the testing framework, so that would make this an 8.2.x issue at this point since 8.1.x is frozen now and about to be in RC.

@alexpott suggested that we should consider BrowserTestBase "experimental", so I suggested it would need to be marked @internal or something if that is the case until it is considered stable. That's worth its own issue. I would suggest committing this to 8.2.x though since it is ready (aside from @alexpott's outstanding question in #30 and @dawehner's response in #31).

Er, didn't mean to actually tag it for RC target triage at this point.

Well, let's ensure that we don't adapt BrowserTestBase from here on.

I've created #2700975: Remove semver promise from testing API to discuss removing the semver promise from the entire testing API.

@dawehner, "in the minor development branch right now" is very different from "never".

I've tested this with multiple browser test base tests and it works. I've also opened the outputted html and it works create - it's nice to be able to see the output. Fantastic work.

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -367,6 +403,15 @@ protected function setUp() {
+    $verbose_directory = getenv('SIMPLETEST_VERBOSE_DIRECTORY');

This is only part that gives me pause. In the 8 dev cycle we had problems with the double meaning of this in WebTestBase. In WebTestBase / run-tests.sh verbose stands before for saving the browser output and also printing verbose result information. I don't think we should repeat that mistake here. I'm not entirely sure of the name we should use but maybe something like SIMPLETEST_BROWSER_OUTPUT_DIRECTORY?

If we want to bikeshed the name I would argue for not putting SIMPLETEST into it: BROWSERTEST_OUTPUT_DIRECTORY. This is indeed a bit more generic and could be useful for other things.

@dawehner I don't want to bikeshed. Verbose is not the right name - this caused problems whilst developing Drupal 8 and I'd like to avoid the same mistakes. Your suggestion makes sense to me.

Btw. for me this issue is not that much fundamentally different to #2664150: Expand BrowserTestBase with error handling support

Okay, I discussed this with @larowlan more and I am convinced that this is worth being an RC target, since it is strategically valuable and very low-risk. If it doesn't land before 8.1.0, my earlier concerns still stand. But if possible let's try to get it in now, since this is actually one of our priorities for 8.1.0 as well. Sorry for sidetracking the issue.

My suggestion would be:

Add BrowserTestBase inside \Drupal\Tests
Mark \Drupal\simpletest\BrowserTestBase deprecated in favour of the new one.

Also, I discovered that locally you can copy phpunit.xml.dist to phpunit.xml and add these variables using the tag. And then you don't need to remember to define them again.

Yeah people don't realize that dist files are meant to be copied :)

Okay, I discussed this with @larowlan more and I am convinced that this is worth being an RC target, since it is strategically valuable and very low-risk.

Just a process suggestion, swap around the process of #32 and #42.

+++ b/core/modules/simpletest/src/DebugBrowserTestBase.php
@@ -0,0 +1,181 @@
+  protected $verboseClassName;
...
+  protected $verboseDirectory;
...
+  protected $verboseCounterStorage;
...
+  protected $verboseCounter = 1;
...
+  protected $verboseOutput = FALSE;
...
+  protected $verboseTestId;
...
+  protected function verbose($message) {
...
+  protected function getVerboseHeaders() {

Let's also stop using the word verbose in the class - that'll also help with any confusion with run-test.sh --verbose option and and also phpunit's --verbose option.

Also @dawehner's plan is way better than DebugBrowserTestBase.

1. +++ b/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php
   @@ -0,0 +1,98 @@
   +  protected $directory;
   

   docs missing.

2. +++ b/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php
   @@ -0,0 +1,98 @@
   +    if ($htmlOutput_directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY')) {
   

   We should add this to phpunit.xml.dist

       <env name="BROWSERTEST_OUTPUT_DIRECTORY" value=""/>
       <!-- Example BROWSERTEST_OUTPUT_DIRECTORY value: /path/to/webroot/sites/default/files/simpletest/verbose -->

Thank you Lee. This is ready imo.

1. +    return '<hr />Headers: <pre>' . Html::escape(var_export(array_map(function($headers) {
   +      return implode(';', array_map('trim', $headers));
   +    }, $this->getSession()->getResponseHeaders()), TRUE)) . '</pre>';
   

   This does not work with the phantomjs driver.

   $headers here is not an array of arrays.

     /**
      * Returns headers in HTML output format.
      *
      * @return string
      *   HTML output headers.
      */
     protected function getHtmlOutputHeaders() {
       $headers = array_map(function($headers) {
         if (is_array($headers)) {
           return implode(';', array_map('trim', $headers));
         }
         else {
           return $headers;
         }
       }, $this->getSession()->getResponseHeaders());
       return '<hr />Headers: <pre>' . Html::escape(var_export($headers, TRUE)) . '</pre>';
     }
   

   The above works as expected.

2. +++ b/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php
   @@ -0,0 +1,104 @@
   +      $this->directory = rtrim($html_output_directory, '/');
   +      // Check if directory exists.
   +      if (is_dir($this->directory)) {
   +        // Remove it.
   +        $this->cleanUpLastTestRun($this->directory);
   +      }
   +      // Create the directory.
   +      if (mkdir($this->directory, 0775)) {
   +        if (is_writable($this->directory)) {
   +          chmod($this->directory, 0775);
   +        }
   +        file_put_contents($this->directory . '/.htaccess', "<IfModule mod_expires.c>\nExpiresActive Off\n</IfModule>\n");
   +      }
   

   I think we're doing too much here. Firstly deleting stuff if super dangerous. I think we should just check that it is a directory and is writeable - if not then disable browser output and tell the user the directory is not writable. Secondly we should just check for the existence of a .htaccess and if it does not exist then we should write it. Thirdly, however making a sub directory using the test id so tidying up yourself is easy.

3. JavascriptTestBase should extend from the new BrowserTestBase

In fact I'm wondering why we're bothering with a defining a directory and not just making the thing a flag since in order to run these tests you need to be able to install Drupal. We could do the same as WebTestBase and write to a directory inside "/simpletest" maybe "/simpletest/browser_output" or something. That way a user won't make the mistake I made of configuring this outside of web root. It needs to be in webroot so the CSS and JS asset links work. I also think we should output URLs instead of files at the end to make this simple.

 2469721  alex: ~/dev/sites/drupal8alt.dev/core > sudo -u _www phpunit tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
PHPUnit 4.8.6 by Sebastian Bergmann and contributors.

.

Time: 7.57 seconds, Memory: 13.75Mb

OK (1 test, 39 assertions)

HTML output was generated
http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_FunctionalJavascriptTests_Core_Form_JavascriptStatesTest-3-772664.html
 2469721  alex: ~/dev/sites/drupal8alt.dev/core > sudo -u _www phpunit modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
PHPUnit 4.8.6 by Sebastian Bergmann and contributors.

...

Time: 19.27 seconds, Memory: 13.50Mb

OK (3 tests, 8 assertions)

HTML output was generated
http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_Tests_simpletest_Functional_BrowserTestBaseTest-36-434443.html
http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_Tests_simpletest_Functional_BrowserTestBaseTest-37-434443.html
http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_Tests_simpletest_Functional_BrowserTestBaseTest-38-434443.html
http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_Tests_simpletest_Functional_BrowserTestBaseTest-39-114002.html
http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_Tests_simpletest_Functional_BrowserTestBaseTest-40-114002.html
 2469721  alex: ~/dev/sites/drupal8alt.dev/core > phpunit /Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/ComposerIntegrationTest.php
PHPUnit 4.8.6 by Sebastian Bergmann and contributors.

..

Time: 288 ms, Memory: 13.25Mb

OK (2 tests, 73 assertions)

So now you can type something like open http://drupal8alt.dev/sites/simpletest/browser_output/Drupal_Tests_simpletest_Functional_BrowserTestBaseTest-40-114002.html and you'll be able to see the output :)

Patch attached:

• changes output from files to links so CSS and JS and other assets work as expected.
• removes any cleaning up of the browser output - if you set the environment variable it is your responsibility to tidy up.
• stops outputting any information about this if you run a unit test (the old patch did)
• makes JavascriptTestBase use the new class

Updated IS for current solution.

if you set the environment variable it is your responsibility to tidy up.

Can we at least update simpletest_script_cleanup to clean this directory?

@jibran that doesn't clean up sites/default/files/simpletest/verbose so I don't get why it should clean up this.

@alexpott so why not move it to simply change it to sites/default/files/simpletest/verbose and remove the constant altogether?

Also WebTestBase shouldn't save it's browser output in sites/default/files - that adds an unnecessary dependency on having a installed site. Adding it to sites/simpletest means that all the test files are in the same place. sites/simpletest is where all the test sites are created.

Also WebTestBase shouldn't save it's browser output in sites/default/files

Maybe change that and this to sites/simpletest/files

What is wrong with calling it what it is? And not making it use a word that means something else elsewhere.

Rerolled for @file changes.

So sites/simpletest/files/html would do it for all of you?

I don't think we should create a directory called files in the simpletest directory. Because a files directory in a sites/* directory is very different to this one.

Okay so would sites/tests work for you? Just try to figure out what kinda works. In general I agree with not using "simpletest" as much as possible now.

sites/tests sounds great to me. But sites/simpletest is OK for me too. We're gradually moving away from the concept of "Simpletest tests", but it is still the "Simpletest module" that provides our test base classes, so in that regard simpletest is not wrong.

And the sites are actually installed in sites/simpletest/TEST_ID

I'm all for changing this - but not here.

I give a damn about the concrete name.

Fine with the directory name in the patch.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

This broke the output when running tests in phpstorm for me.

Notice: Trying to get property of non-object in /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php on line 263

Call Stack:
    0.0021     450088   1. {main}() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:0
    0.0157    1301392   2. IDE_Base_PHPUnit_TextUI_Command::main() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:587
    0.0157    1301504   3. PHPUnit_TextUI_Command->run() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:299
    0.0157    1301504   4. IDE_Base_PHPUnit_TextUI_Command->handleArguments() /Users/benjy/Sites/d8/app/vendor/phpunit/phpunit/src/TextUI/Command.php:110
    0.4006    1845768   5. IDE_Base_PHPUnit_TextUI_ResultPrinter->__construct() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:310

PHP Notice:  Trying to get property of non-object in /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php on line 262
PHP Stack trace:
PHP   1. {main}() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:0
PHP   2. IDE_Base_PHPUnit_TextUI_Command::main() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:587
PHP   3. PHPUnit_TextUI_Command->run() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:299
PHP   4. IDE_Base_PHPUnit_TextUI_Command->handleArguments() /Users/benjy/Sites/d8/app/vendor/phpunit/phpunit/src/TextUI/Command.php:110
PHP   5. IDE_Base_PHPUnit_TextUI_ResultPrinter->__construct() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:310
PHP Notice:  Trying to get property of non-object in /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php on line 263
PHP Stack trace:
PHP   1. {main}() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:0
PHP   2. IDE_Base_PHPUnit_TextUI_Command::main() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:587
PHP   3. PHPUnit_TextUI_Command->run() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:299
PHP   4. IDE_Base_PHPUnit_TextUI_Command->handleArguments() /Users/benjy/Sites/d8/app/vendor/phpunit/phpunit/src/TextUI/Command.php:110
PHP   5. IDE_Base_PHPUnit_TextUI_ResultPrinter->__construct() /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php:310
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.



Time: 3.62 seconds, Memory: 4.00Mb

OK (1 test, 4 assertions)

Process finished with exit code 0

EDIT: This was running: MigrateActionConfigsTest

Re-opening. If we can fix or diagnose it in the next few hours I'll commit a follow-up, otherwise will roll back especially if there's more reports.

Bumping status so it doesn't get forgotten.

Here's the command PHPStorm executes.

/usr/local/opt/php70/bin/php /private/var/folders/dk/f9s3wd2n04s8z4bd0pflm7yr0000gn/T/ide-phpunit.php --color=always --configuration /Users/benjy/Sites/d8/app/core/phpunit.xml.dist Drupal\Tests\action\Kernel\Migrate\d6\MigrateActionConfigsTest /Users/benjy/Sites/d8/app/core/modules/action/tests/src/Kernel/Migrate/d6/MigrateActionConfigsTest.php

PHPStorm doesn't call the binary directly but generates the ide-phpunit.php file in a temporary space which handles overriding the result printer for PHPStorm. You can't have two result printers so the solution right now is to remove printerClass="\Drupal\Tests\Listeners\HtmlOutputPrinter" from your phpunit.xml.dist file.

Well, there is some code in ide-phpunit.php which tries to wrap an existing printer.

Debugged the problem a bit, first I thought the problem is somewhere in phpunit itself, but its IMHO a bug in phpstorm, see https://github.com/sebastianbergmann/phpunit/issues/2141 / https://youtrack.jetbrains.com/issue/WI-24808

One thing I stumbled upon is the concept of listeners. Sadly implementing what we implemented isn't that easy within a listener as the print classes on phpunit are not in its own component, so writing stuff out would not be nice and pretty.

This is a PHPStorm bug.

    /**
     * @param PHPUnit_Util_Printer $printer
     */
    function __construct($printer)
    {
        parent::__construct(null);
        if (!is_null($printer)) {
            $this->out = $printer->out;
            $this->outTarget = $printer->outTarget;
        }
    }

At this point $printer has a value of \Drupal\Tests\Listeners\HtmlOutputPrinter so I guess something has changed in PHPUnit since this code was written. Or maybe the latest version of PHPunit do it differently.

Regardless - fixing this bug would seem to not be our responsibility. I'll look on phpstorm's bugtracker to see if someone has reported it. Also, at least for me, running BrowserTestBase tests inside PHPStorm is never going to work because my webserver is running as a different user.

Personally I think we should not revert this patch.

xpost with @dawehner. So yeah there is an existing bug report on PHPStorm and @dawehner has created a patch for them.

I don;t think that running BrowserTestBase tests inside PHPStorm should be our testing target anyway. PHPunit from the command line / run-tests.sh is. Therefore moving this issue back to fixed and resetting the issue metadata.

Additionally whilst PHPStorm reports the error it does not actually stop the test from running and any assertions that fail are correctly displayed.

Also, at least for me, running BrowserTestBase tests inside PHPStorm is never going to work because my webserver is running as a different user

The output is broken for unit and kernel tests as well though.

Let's compromise and do #2706113: Disable Drupal\Tests\Listeners\HtmlOutputPrinter by default because PHPStorm is broken and then maybe we can come out with a solution that doesn't break PHPStorm and have it on all the time. But this at least gives people the choice.

#2702661: Make it simple to take screenshots whilst using JavascriptTestBase is resolvable?

@nevergone yeah that is actually different to this issue - that can take screenshots of the current browser - so you can see exactly what has happened even with javascript.