Make it simple to take screenshots whilst using JavascriptTestBase

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
   @@ -84,4 +84,24 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
   +  protected function createScreenshot($filename) {
   ...
   +    $image = $driver->getScreenshot();
   +    file_put_contents($filename, $image);
   +  }
   

   IMHO we should leverage the verbose directory and generate a file automatically, write a line to the output, so we don't have to deal with defining a filename.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
   @@ -84,4 +84,24 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
   +    // Ensure the body background is white.
   +    $driver->executeScript("document.body.style.backgroundColor = 'white';");
   

   You don't say ... but why

@dawehner #3.1 yeah but we don't have that yet... and also we need to list them somehow which will make it dependent on the result printer.

Do you really think this is a problem?

Please don't hardcode background color. Thanks!

@nevergone
Can you elaborate why?

changeable background color

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -79,4 +79,25 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
+  protected function createScreenshot($filename, $background = 'white') {

Let's document the parameter.

Still would like to know why you think its needed.

Can we implement this as verbose output that can be opened like the simpletest HTML pages every time a request happens? If enabled, it would be nice to click through a test and see what was going on at every step.

I've been working with this for a contrib module. This might be a seperate issue, but we should create a method which writes screenshots to the same browser_output folder that BTB writes to. I think it would be a good idea to use the htmlOutputCounter and store them as similar file names. That way the verbose HTML output and the screenshots taken can form a linear history of the test run that is easy to view and debug.

I've already got a local testing trait that does this and organises the verbose output file into clickable links. Seems to be working nicely.

Fixed minor docs and used session methods instead of driver.

Lets do this

Patch apply locally with an up to date 8.1.x branch. relaunch test

Re-rolled

Thank you for the reroll!

Parameter documentation.

I update to last 8.1.x and tests don't fail for me. Strange thing, tests fails on something completely different from patch, and for a simpletest test although we are patching phpunit JavascriptTestBase tests.

I re-launch test in case something was wrong during tests

I update to last 8.1.x and tests don't fail for me. Strange thing, tests fails on something completely different from patch, and for a simpletest test although we are patching phpunit JavascriptTestBase tests.

Yeah no worries, we have quite some random test failures atm.

Nice work! I'm looking forward to using this function in my test actually.

I applied and tested the patch #21 on my local environment. And passed parameter $background_color doesn't work.

  protected function createScreenshot($filename, $background_color = 'white') {
    $session = $this->getSession();
    if (!empty($background)) {
      $session->executeScript("document.body.style.backgroundColor = '" . $background_color . "';");
    }

This !empty($background) was left when changing the variable name $background and should be changed to !empty($background_color), I believe. Is my understanding right?

nice @hgoto. I checked it twice and missed it...

Thank you for your reaction. The patch #28 looks good to me! But I'd like someone else to review this for RTBC.

Good spot

Should we not add a short test here to ensure that a screenshot is actually taken?

Nice. I doubt we can test the actual output of the screenshot though :P

It would be nice if this method is able to output a message in the console with the URL/path of the created file, like in the case of HTML output. That is very useful because you can click the link/path and see instantly the output.

Also I would make the filename optional, then I would default to a built filename (from test no + some variance) and I would use the same location as for HTML pages. What about that?

I think @alexpott makes a fair point. The general verbose feature needs some additional work.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -103,6 +103,29 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
+      $session->executeScript("document.body.style.backgroundColor = '" . $background_color . "';");

So I discussed this with @alexpott... this is technically a potential vector for JS injection.

E.g., suppose someone writes a UI to allow you to set the background color of your test screenshots. Drupal as a rule filters on output, not input, so that UI could be legit in passing minimally validated user input here.

I don't actually think this is a plausible exploit -- this is a test runner after all -- more concerned about the precedent of $this->executeScript($variable).

I mentioned this to @alexpott and he is thinking about it.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
   @@ -109,7 +110,10 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
   +   *   (optional) Background color name. To use the default background color set
   +   *   to NULL, however this can result in completely black screenshots if the
   +   *   theme does not have a background color. This string is escaped by
   +   *   \Zend\Escaper\Escaper::escapeJs().
       *
   

   Nice!

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
   @@ -119,7 +123,8 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
        if (!empty($background_color)) {
   -      $session->executeScript("document.body.style.backgroundColor = '" . $background_color . "';");
   +      $escaper = new Escaper();
   +      $session->executeScript("document.body.style.backgroundColor = '" . $escaper->escapeJs($background_color) . "';");
   

   I thought for a while whether we could leverage twig, turns out that would be a bit of a huzzle, as twig_escape_filter() is not necessarily easy to use.

Thanks @alexpott and @dawehner; I think this addresses the potential problem.

Technically this is an API addition to a public API (the testing framework is considered public even though tests themselves are not), but we sort of have the idea that JTB is experimentalish, and I think the value of this method for debugging 8.1.x vastly outweighs any risk of disruption from a method name collision. @alexpott is updating https://www.drupal.org/node/2716803 just in case.

@Sam152 and @claudiu.cristea, I think both your suggestions are worth followup issues.

Committed e42f6c5 and pushed to 8.2.x. Thanks!

Oh! Just doing local test but got committed.

won't it encoded (escaped) #hex color?

Similiar regex would work I think but limited to hex code and color name only, no RGBa,HSL..etc

/^(#[0-9A-F]{3}|[a-z])/

Thanks @droplet and @alexpott, I reverted. I guess this is why we should actually not leave issues at RTBC. :)

@alexpott and I agreed to remove the background color customization for now and create a followup for that, so that we can add the debugging now without getting blocked on the right way to implement it.

regular expression added

Thanks @nevergone. Per #53 and #54 let's address that in a followup.

Setting back to NR for #55.

Usually, frontender apply bg color to BODY. We could consider applying the bgColor to HTML (document.bgColor) if that worked for phantomjs bug also

Or

if( window.getComputedStyle(document.body).backgroundColor === 'rgba(0, 0, 0, 0)') {
... // set white as default
}

Ehm… isn't #55 ready? What am I missing? AFAICT #56 is an accidental mistake?

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -103,6 +104,36 @@ protected function assertJsCondition($condition, $timeout = 1000, $message = '')
+      if (!preg_match('/^(#[0-9A-F]{3}|[a-z])/', $background_color)) {

IMHO we should document why we have this regex here

Good idea.
Thank you alex!

Committed c4cb804 and pushed to 8.2.x. Thanks!

I've also backported this to 8.1.x, as explained above. This is in spite of semver and the risk of method name collisions, because this is very useful for debugging and I consider JSTB "experimental-ish". #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests is the place to discuss further whether or not that makes sense.

We still could use that followup for the background color configurability -- @nevergone and @droplet's feedback should be included in that issue.

Thanks everyone!

High fives all around!