Problem/Motivation

The new JavascriptTestBase is quite hard to debug. We should add a method that allows developers to take screenshots so they can easily see the current state.

Proposed resolution

This is similar to #2469721: Add functionality to store browser output to BrowserTestBase but that stores the HTML generated by Drupal whereas a screenshot would show you the current state whilst the test interacts with the browser. Therefore making it simpler to see any interactions occurring.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.2 KB
9.65 KB
14.93 KB

Here's a patch at it's simplest. It took me a while working out how to do this and not just see an image with a black background...

Without setting the background

With setting the background

dawehner’s picture

  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

alexpott’s picture

@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.
Re #3.2 see screenshots - but yes comment can be improved...

dawehner’s picture

@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?

nevergone’s picture

Please don't hardcode background color. Thanks!

dawehner’s picture

@nevergone
Can you elaborate why?

nevergone’s picture

changeable background color

dawehner’s picture

+++ 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.

Sam152’s picture

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.

alexpott’s picture

@Sam152 maybe but it is tricky as some javascript animations take time.

Sam152’s picture

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.

jibran’s picture

Fixed minor docs and used session methods instead of driver.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Lets do this

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: make_it_simple_to_take-2702661-13.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

The last submitted patch, 13: make_it_simple_to_take-2702661-13.patch, failed testing.

GoZ’s picture

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

GoZ’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.68 KB

Re-rolled

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the reroll!

nevergone’s picture

Parameter documentation.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: make_it_simple_to_take-2702661-21.patch, failed testing.

GoZ’s picture

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

GoZ’s picture

Status: Needs work » Reviewed & tested by the community
dawehner’s picture

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.

hgoto’s picture

Status: Reviewed & tested by the community » Needs work

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?

alexpott’s picture

@hgoto you are right... nice spot

GoZ’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
771 bytes

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

hgoto’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good spot

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: make_it_simple_to_take-2702661-28.patch, failed testing.

The last submitted patch, 28: make_it_simple_to_take-2702661-28.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Drupal git hiccup.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: make_it_simple_to_take-2702661-28.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
webchick’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: make_it_simple_to_take-2702661-28.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
764 bytes
2.1 KB

Sure why not. Leaving at RTBC because this is very simple.

dawehner’s picture

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

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review

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?

alexpott’s picture

@claudiu.cristea imo that is all followup - atm this is a simple debugging tool - plugging in to \Drupal\Tests\Listeners\HtmlOutputPrinter - which is where we'd have to do - this can be done later.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

+++ 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.

alexpott’s picture

@xjm nice catch.

We can use Zend's escaper for this. I think this is worth doing because people learn by looking at code - so whilst I don't think there is a security issue per se - I think we should set a good example of how to add variables to Javascript in code.

Psy Shell v0.7.0 (PHP 7.0.7 — cli) by Justin Hileman
>>> $e = new \Zend\Escaper\Escaper();
=> Zend\Escaper\Escaper {#12653}
>>> $e->escapeJs("alert('blah');");
=> "alert\x28\x27blah\x27\x29\x3B"
>>> $e->escapeJs("white");
=> "white"
dawehner’s picture

  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.

  • xjm committed e42f6c5 on 8.2.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, hgoto, xjm...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • xjm committed af4ce92 on 8.1.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, hgoto, xjm...
droplet’s picture

Status: Fixed » Needs work

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])/
alexpott’s picture

>>> $e = new \Zend\Escaper\Escaper();
=> Zend\Escaper\Escaper {#12504}
>>> $e->escapeJs("white");
=> "white"
>>> $e->escapeJs("#CCFFEE");
=> "\x23CCFFEE"

ho hum...

  • xjm committed 9986e5e on 8.1.x
    Revert "Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner,...

  • xjm committed 7bce285 on 8.2.x
    Revert "Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner,...
xjm’s picture

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.

xjm’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
2.03 KB

Here's a patch that does exactly that. It seemed like a nice feature to have a changeable background colour - introduced in #6 but the 99% use case is just to get a good looking screenshot which this now does. If you want different background colours you can always set the background colour outside of this method and call the screenshotting method too. This is just a convenience method to make things quick and simple for debugging - because I spent a few hours crying over black screenshots and didn't want anyone else to have to go through what I did. If we add changeable colours in the future so be it.

nevergone’s picture

Status: Needs review » Needs work
FileSize
2.68 KB
1001 bytes

regular expression added

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Needs followup

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

Setting back to NR for #55.

droplet’s picture

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
}
Wim Leers’s picture

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

dawehner’s picture

+++ 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

alexpott’s picture

@nevergone can we do the customisable background colour in a follow-up as suggested by @xjm. It's not necessary for the usual use-case and it's just preventing this issue going in since the custom regex in #56 needs testing.

Uploading #55 so the correct patch to review is last.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good idea.
Thank you alex!

  • xjm committed c4cb804 on 8.2.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, xjm, hgoto...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • xjm committed 6c0b137 on 8.1.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, xjm, hgoto...
xjm’s picture

Wim Leers’s picture

High fives all around!

  • xjm committed 7bce285 on 8.3.x
    Revert "Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner,...
  • xjm committed c4cb804 on 8.3.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, xjm, hgoto...
  • xjm committed e42f6c5 on 8.3.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, hgoto, xjm...

  • xjm committed 7bce285 on 8.3.x
    Revert "Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner,...
  • xjm committed c4cb804 on 8.3.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, xjm, hgoto...
  • xjm committed e42f6c5 on 8.3.x
    Issue #2702661 by alexpott, nevergone, GoZ, jibran, dawehner, hgoto, xjm...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.