Problem/Motivation

Over in #2232861: Create BrowserTestBase for web-testing on top of Mink we got step one in, BrowserTestBase.
Now on with step 2 - adding a JavaScript driver.

Proposed resolution

* Use a phantomjs mink driver
* Let people install phantomjs on their server, its a tool like mysql/php
* Use the mink driver in a JavascriptTestBase
* Add a simple test: toolbar

Remaining tasks

* Motivate the testbot team to look at #2580007: Add phantomjs2 to the testrunners
* Review this issue
* profit

User interface changes

None

API changes

New base class for JavaScript testing

CommentFileSizeAuthor
#265 2469713-265.patch2.73 KBdawehner
#233 2469713-3-233.patch16.12 KBalexpott
#233 230-233-interdiff.txt1003 bytesalexpott
#230 2469713-3-230.patch15.9 KBalexpott
#230 228-230-interdiff.txt1.25 KBalexpott
#228 2469713-3-228.patch15.15 KBalexpott
#228 222-228-interdiff.txt3.22 KBalexpott
#222 2469713-3-222.patch15.05 KBalexpott
#222 208-222-interdiff.txt3.92 KBalexpott
#208 2469713-2-208.patch14.95 KBalexpott
#208 202-208-interdiff.txt1.95 KBalexpott
#202 interdiff.txt559 bytesdawehner
#202 2469713-202.patch14.66 KBdawehner
#200 interdiff.txt797 bytesdawehner
#200 2469713-200.patch14.65 KBdawehner
#198 interdiff.txt3.62 KBpfrenssen
#198 2469713-198.patch13.87 KBpfrenssen
#195 interdiff.txt4.47 KBpfrenssen
#195 2469713-195.patch14.22 KBpfrenssen
#191 interdiff.txt2.37 KBpfrenssen
#191 2469713-191.patch14 KBpfrenssen
#189 interdiff.txt7.89 KBpfrenssen
#189 2469713-189.patch14 KBpfrenssen
#185 interdiff.txt6.6 KBpfrenssen
#185 2469713-185.patch11.78 KBpfrenssen
#177 2469713-177.patch12.24 KBdawehner
#177 interdiff.txt1.09 KBdawehner
#176 2469713-176.patch12.98 KBdawehner
#172 2469713-172.patch13.98 KBdawehner
#170 2469713-170.patch14.04 KBdawehner
#167 interdiff.txt1.78 KBdawehner
#167 2469713-167.patch13.63 KBdawehner
#162 interdiff.txt11.67 KBdawehner
#162 2469713-161.patch11.67 KBdawehner
#161 2469713-161.patch11.67 KBdawehner
#146 interdiff.txt547 bytesMixologic
#145 interdiff.txt147 bytesMixologic
#145 2469713-145.patch10.75 KBMixologic
#142 2469713-142.patch10.75 KBMixologic
#139 2469713-139.patch10.78 KBhussainweb
#126 interdiff.txt912 bytespfrenssen
#126 2469713-126.patch10.75 KBpfrenssen
#102 2469713-jtb.102.patch10.81 KBlarowlan
#102 interdiff.txt711 byteslarowlan
#92 2469713-92.patch11.23 KBelachlan
#90 2469713-90.patch11.23 KBelachlan
#88 interdiff.txt930 byteselachlan
#88 2469713-88.patch909 byteselachlan
#81 interdiff.txt1.12 KBdawehner
#81 2469713-81.patch10.76 KBdawehner
#72 2469713-72.patch12.67 KBelachlan
#72 interdiff.txt5.75 KBelachlan
#69 2469713-69.patch12.54 KBelachlan
#46 interdiff.txt3.62 KBpfrenssen
#46 2469713-46.patch11.64 KBpfrenssen
#42 2469713-42.patch9.53 KBdawehner
#40 2469713-40.patch9.53 KBdawehner
#35 interdiff.txt3.34 KBdawehner
#35 2469713-35.patch406.19 KBdawehner
#30 2469713-30.patch408.85 KBdawehner
#30 interdiff.txt1.32 KBdawehner
#27 interdiff.txt1.41 KBdawehner
#27 2469713-27.patch407.53 KBdawehner
#22 2469713-22.patch406.55 KBdawehner
#22 interdiff.txt2.06 KBdawehner
#22 2469713-22-review.patch11.08 KBdawehner
#21 2469713-20.patch3.23 MBdawehner
#21 interdiff.txt2.85 MBdawehner
#18 2469713-18.patch3.23 MBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue summary: View changes
chx’s picture

The taxonomy overview page is not your best choice: it certainly breaks but it's the backend that breaks as it tries to pager a drag-and-drop form and that is ... not working well as it can be expected. We filed a solution to this to #242324: Going to page 2 on "list terms" page doesn't display additional terms . (Corporate interests end here.)

In my personal opinion, tabledrag is a good idea but also you might want the new filter on admin/extend because it's woefully complicated as far as I know but ask nod_ whether I remember correctly.

phenaproxima’s picture

+1 for a TableDrag test case. It's such a widely-used JS feature that it would be incredibly convenient to have a general test for it in core :) I also like @chx's idea of testing the module filter, since it's a totally different kind of JS interaction.

moshe weitzman’s picture

See #237566-122: Automated JavaScript unit testing framework for a tabledrag test that could borrowed here.

grom358’s picture

Instead of another base class (JavaScriptTestBase) why not have pluggable driver provider to BrowserTestBase. I know registerSessions() is there so a subclass could register other mink sessions, but plugins would allow for using any mink driver people wanted.

Though for core we need to agree on a javascript capable driver to include into the testing infrastructure. (eg. PhantomJS?)

Wim Leers’s picture

Issue tags: +JavaScript
dawehner’s picture

https://github.com/dawehner/MinkPhantomJSDriver looks quite interesting but well, the chain of dependencies require guzzle 6, so this would be fun first to port it over.
Maybe something similar could be a way forward.

dawehner’s picture

Status: Active » Needs review
FileSize
255.51 KB
20.27 KB

While working on this issue I saw the following message: #2563821: Implement Twig_Loader_String for ourselves

Status: Needs review » Needs work

The last submitted patch, 8: 2469713-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

This time with --binary, mh, I can't upload the file:

$ du -h 2469713-10.patch
 52M    2469713-10.patch
jibran’s picture

Funny thing I set it up, phantomjs, recently on previousnext internal project for JS testing and ran into same problem with status code issue. I was planing to write a blog post on it set up and all that but procrastination won. I had an interesting conversation with @grom358 related to this. Found some chat logs.

jibran: so turns out Selenium doesn't support status code and http headers
cafuego: jibran: Do you need to test those via behat?
jibran: cafuego: I think "I should be on $url" would be ok no need to response code.
jibran: https://code.google.com/p/selenium/issues/detail?id=2047
jibran: https://code.google.com/p/selenium/issues/detail?id=141
grom358: @jibran: what do you mean by Selenium doesn't support status code and http headers? what are you trying todo
grom358: @jibran: you can't custom HTTP headers no.. that is why BrowserTestBase included patch to use cookies
jibran: oh
grom358: good reason for that.. Selenium can control an actual browser.. ie. Firefox. And so doesn't have way to do stuff at HTTP layer cause its operating through the browser UI
jibran’s picture

It is 52M because it includes all the binaries from https://github.com/previousnext/phing-phantomjs/tree/master/bin. On internal CI we run composer install before starting testing and then run phantomjs executable using phing from https://github.com/previousnext/phing-phantomjs/blob/master/build.xml

  <target name="phantomjs"
          description="Run phantomjs service.">
    <exec command="${phantomjs.bin} --webdriver=${phantomjs.port}"
          spawn="true" />
  </target>
dawehner’s picture

https://github.com/minkphp/Mink/issues/654 could be pretty interesting, given that it would allow us to not require to use selenium.
Why is that important? Selenium for example doesn't give you access to status code, while phantomjs would do.

pfrenssen’s picture

Selenium is very complicated in that regard. One way to work around the HTTP status code problem by executing HTTP requests in the browser using Javascript, as an example see http://cgit.drupalcode.org/paddle_selenium_tests/tree/pages/Kanooh/Paddl...

Having extensive experience with Selenium I would vote against it as a default solution. It is very slow, resource heavy and prone to random failures.

dawehner’s picture

Yeah its pretty crazy, so do you suggest we take part in producing a native PhantomJS driver for mink and use that afterwards?

pfrenssen’s picture

@dawehner I am very much in favour for that!

dawehner’s picture

While talking with @jaesin via tests we realized that making it swappable currently is a first good step: #2573165: Step 1.5: Make the driver swappable more easy.

dawehner’s picture

This work here is just experimental and we certainly should figure out #2573165: Step 1.5: Make the driver swappable more easy. first.

Status: Needs review » Needs work

The last submitted patch, 18: 2469713-18.patch, failed testing.

The last submitted patch, 18: 2469713-18.patch, failed testing.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.08 KB
2.06 KB
406.55 KB

This now actually works quite well. Using ";" on cookie values seems not that trivial as you think it is. Let's use ":", it at least cannot really conflict with the way how cookies are stored internally

The last submitted patch, 22: 2469713-22-review.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2469713-22.patch, failed testing.

The last submitted patch, 22: 2469713-22-review.patch, failed testing.

The last submitted patch, 22: 2469713-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
407.53 KB
1.41 KB
  • Rerolled
  • Added an example javascript test.

Status: Needs review » Needs work

The last submitted patch, 27: 2469713-27.patch, failed testing.

dawehner’s picture

Adding a related issue on the testrunner site ... for now this can't work as phantomjs isn't there. If you would check the code and run phantomjs already, you could execute javascript, click on stuff etc. all in the browser.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
408.85 KB

This now also works.

Wim Leers’s picture

+++ b/core/modules/toolbar/tests/src/Functional/ToolbarIntegrationTest.php
@@ -0,0 +1,35 @@
+class ToolbarIntegrationTest extends BrowserTestBase {

<3

Status: Needs review » Needs work

The last submitted patch, 30: 2469713-30.patch, failed testing.

dawehner’s picture

Issue tags: +rc eligible
elachlan’s picture

#2580007: Add phantomjs2 to the testrunners needs some love and a review.

dawehner’s picture

Status: Needs work » Needs review
FileSize
406.19 KB
3.34 KB

Step 1.5 worked fine, until we tested it :)

Status: Needs review » Needs work

The last submitted patch, 35: 2469713-35.patch, failed testing.

dawehner’s picture

Here is a "fun" subissue

elachlan’s picture

So now that is RTBC, are we just waiting on #2580007: Add phantomjs2 to the testrunners?

dawehner’s picture

So now that is RTBC, are we just waiting on #2580007: Add phantomjs to the testrunners?

Yeah that one would block this issue as well!

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.53 KB

Here is a reroll.
Thank you @webchick to commit step 1.75.

Status: Needs review » Needs work

The last submitted patch, 40: 2469713-40.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.53 KB
~/w/d8 (2469713) $ phpunit -c core core/modules/toolbar/tests/src/Functional/ToolbarIntegrationTest.php
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

.

Time: 9.66 seconds, Memory: 31.00Mb

OK (1 test, 6 assertions)

Totally flabbergasted at the moment.

Status: Needs review » Needs work

The last submitted patch, 42: 2469713-42.patch, failed testing.

The last submitted patch, 42: 2469713-42.patch, failed testing.

pfrenssen’s picture

  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -226,6 +234,14 @@ protected function initMink() {
    +    // Fire up the first request to the front page in order to be able to  set
    +    // a cookie later.
    +    // @fixme This is done to circumvent:
    +    // WebDriver\Exception\UnableToSetCookie: {"errorMessage":"Unable to set Cookie: no URL has been loaded yet","request":{"headers":{"Accept":"application/json;charset=UTF-8","Content-Length":"164","Content-Type":"application/json;charset=UTF-8","Host":"127.0.0.1:8910"},"httpVersion":"1.1","method":"POST","post":"{\"cookie\":{\"name\":\"SIMPLETEST_USER_AGENT\",\"value\":\"simpletest472558;1441488302;55eb5dae78eeb7.36607058;IxhRvB7dhbDAMurfBshqgUwEOpAPPKybnJMv0JgaG8Q\",\"secure\":false}}","url":"/cookie","urlParsed":{"anchor":"","query":"","file":"cookie","directory":"/","path":"/cookie","relative":"/cookie","port":"","host":"","password":"","user":"","userInfo":"","authority":"","protocol":"","source":"/cookie","queryKey":{},"chunks":["cookie"]},"urlOriginal":"/session/91aef6b0-5414-11e5-9028-67c7fcbbdc3b/cookie"}}
    +    $session = $this->getSession();
    +    $session->visit($this->baseUrl);
    

    Isn't this a bug in Mink? Why should we have to do a request before being able to set a cookie?

  2. +++ b/core/modules/simpletest/tests/src/Functional/BrowserWithJavascriptTest.php
    @@ -0,0 +1,45 @@
    +    $session->resizeWindow(400, 300);
    +    $session->wait(1000, 'false');
    

    Is it needed to wait a full second for this? Since PhantomJS is headless, shouldn't this be instant? I always think that we should avoid to wait in tests, but it is possible that this is an asynchronous action and that there is no other way.

  3. +++ b/core/modules/simpletest/tests/src/Functional/BrowserWithJavascriptTest.php
    @@ -0,0 +1,45 @@
    +    print_r($pageSize);
    

    Crufty bits.

pfrenssen’s picture

Just playing around with it a bit. It works great, this is so much fun!

dawehner’s picture

Yeah I guess 1000 milliseconds is way too much, maybe we could go with less. It would be great to see what other similar test code is using.

Isn't this a bug in Mink? Why should we have to do a request before being able to set a cookie?

Oh yeah I totally think so, but I'm not sure how to fix it yet.

Crufty bits.

We can obviously drop it.

As a follow up we could try to figure out how to make the usage of mink easier.

pfrenssen’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -27,6 +27,7 @@
+use Zumba\Mink\Driver\PhantomJSDriver;

+++ b/core/modules/simpletest/src/JavascriptTestBase.php
@@ -0,0 +1,46 @@
+    // Setup some template cache using by the phantomjs mink driver.
+    $path = '/tmp/browsertestbase-templatecache';
+    if (!file_exists($path)) {
+      mkdir($path);
+    }

This was causing me some trouble. On my system the global temporary folder is not writeable by the webserver.

Shall we move this to sites/default/files/simpletest/browsertestbase-templatecache?

benjy’s picture

If we want the system tmp folder we should use sys_get_temp_dir() although storing things in the global tmp folder might cause issues when running multiple tests at once, caching in the simpletest folder per test might be better.

dawehner’s picture

Issue summary: View changes

Working on the issue summar

elachlan’s picture

Issue summary: View changes

Test bot team has created a branch for the code and we have reviewed it.

What is the next step? Does it need to go into production for us to test this code?

Edit: They created the branch in issue #2580007: Add phantomjs2 to the testrunners.

andypost’s picture

Issue tags: +Needs reroll

Delivered #2580007: Add phantomjs2 to the testrunners
now patch needs re-roll

elachlan’s picture

Issue summary: View changes
Status: Needs work » Needs review

Queuing the latest patch for testing.

The last submitted patch, 21: 2469713-20.patch, failed testing.

The last submitted patch, 27: 2469713-27.patch, failed testing.

The last submitted patch, 30: 2469713-30.patch, failed testing.

The last submitted patch, 35: 2469713-35.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 46: 2469713-46.patch, failed testing.

elachlan’s picture

It's beautiful :')

dawehner’s picture

Mh, damnit this wasn't everything which we need ... @elachlan any idea already?

elachlan’s picture

Nope.

PhantomJS started ok and the port is correct.

We didn't patch for #48/#49, it could be permissions. I have no idea though. Its failing in testJavascript().

dawehner’s picture

@isntall is currently helping us with that. Thank you so much!

alvar0hurtad0’s picture

Issue tags: -Needs reroll

Patch #46 applies

isntall’s picture

I ran a test using my latest attempt to have phantomjs run in the background and patch 2469713-46.patch.

https://dispatcher.drupalci.org/job/staging_simpletest_job/320/

phantomjs was running in the background and there were still the 2 failures.

elachlan’s picture

The failure:

fail: [Other] Line 19 of core/modules/simpletest/tests/src/Functional/BrowserWithJavascriptTest.php:

In the function "testJavascript", the line:

$this->drupalGet('<front>');

Are we able to get more error information?

larowlan’s picture

Note that over in #2114887: Maximum nesting level when attaching comment field to User. I had to make some changes to BTB to recognize base urls, perhaps that the issue here too?

elachlan’s picture

Were the changes committed?

dawehner’s picture

Well, I think that @isntall and the fact that the phantomjs process stops to run at some point, is a clear reason for the problems we see.

elachlan’s picture

Trying what @larowlan suggested. Not sure if this is all that was needed.

elachlan’s picture

Status: Needs work » Needs review
dawehner’s picture

@elachlan
Always please provide an interdiff, this makes it easier for people to see what you changed.

elachlan’s picture

FileSize
5.75 KB
12.67 KB

Here is a new patch which takes all the changes to browser test base done in the other issue. As well as an interdiff.

dawehner’s picture

Status: Needs review » Needs work
  1. index 031b53b..ce872d4 100644
    --- a/core/includes/bootstrap.inc
    
    --- a/core/includes/bootstrap.inc
    +++ b/core/includes/bootstrap.inc
    
    +++ b/core/includes/bootstrap.inc
    @@ -618,9 +618,9 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    
    @@ -618,9 +618,9 @@ function drupal_valid_test_ua($new_prefix = NULL) {
       // string.
       $http_user_agent = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : NULL;
       $user_agent = isset($_COOKIE['SIMPLETEST_USER_AGENT']) ? $_COOKIE['SIMPLETEST_USER_AGENT'] : $http_user_agent;
    -  if (isset($user_agent) && preg_match("/^(simpletest\d+):(.+):(.+):(.+)$/", $user_agent, $matches)) {
    +  if (isset($user_agent) && preg_match("/^(simpletest\d+);(.+);(.+);(.+)$/", $user_agent, $matches)) {
         list(, $prefix, $time, $salt, $hmac) = $matches;
    -    $check_string =  $prefix . ':' . $time . ':' . $salt;
    +    $check_string =  $prefix . ';' . $time . ';' . $salt;
         // Read the hash salt prepared by drupal_generate_test_ua().
         // This function is called before settings.php is read and Drupal's error
         // handlers are set up. While Drupal's error handling may be properly
    @@ -677,8 +677,8 @@ function drupal_generate_test_ua($prefix) {
    
    @@ -677,8 +677,8 @@ function drupal_generate_test_ua($prefix) {
       }
       // Generate a moderately secure HMAC based on the database credentials.
       $salt = uniqid('', TRUE);
    -  $check_string = $prefix . ':' . time() . ':' . $salt;
    -  return $check_string . ':' . Crypt::hmacBase64($check_string, $key);
    +  $check_string = $prefix . ';' . time() . ';' . $salt;
    +  return $check_string . ';' . Crypt::hmacBase64($check_string, $key);
     }
     
    

    I'm sorry, but this change was needed due the phantomjs nicety/driver niceties.

  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -234,14 +233,6 @@ protected function initMink() {
    -
    -    // Fire up the first request to the front page in order to be able to  set
    -    // a cookie later.
    -    // @fixme This is done to circumvent:
    -    // WebDriver\Exception\UnableToSetCookie: {"errorMessage":"Unable to set Cookie: no URL has been loaded yet","request":{"headers":{"Accept":"application/json;charset=UTF-8","Content-Length":"164","Content-Type":"application/json;charset=UTF-8","Host":"127.0.0.1:8910"},"httpVersion":"1.1","method":"POST","post":"{\"cookie\":{\"name\":\"SIMPLETEST_USER_AGENT\",\"value\":\"simpletest472558;1441488302;55eb5dae78eeb7.36607058;IxhRvB7dhbDAMurfBshqgUwEOpAPPKybnJMv0JgaG8Q\",\"secure\":false}}","url":"/cookie","urlParsed":{"anchor":"","query":"","file":"cookie","directory":"/","path":"/cookie","relative":"/cookie","port":"","host":"","password":"","user":"","userInfo":"","authority":"","protocol":"","source":"/cookie","queryKey":{},"chunks":["cookie"]},"urlOriginal":"/session/91aef6b0-5414-11e5-9028-67c7fcbbdc3b/cookie"}}
    -    $session = $this->getSession();
    -    $session->visit($this->baseUrl);
    
    @@ -272,7 +263,7 @@ protected function getDefaultDriverInstance() {
            // Use ReflectionClass to instantiate class with received params.
    -      $reflector = new \ReflectionClass($this->minkDefaultDriverClass);
    

    These two changes are wrong as well, sorry

The last submitted patch, 21: 2469713-20.patch, failed testing.

The last submitted patch, 27: 2469713-27.patch, failed testing.

The last submitted patch, 30: 2469713-30.patch, failed testing.

The last submitted patch, 35: 2469713-35.patch, failed testing.

The last submitted patch, 46: 2469713-46.patch, failed testing.

The last submitted patch, 69: 2469713-69.patch, failed testing.

The last submitted patch, 72: 2469713-72.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.76 KB
1.12 KB

Rerolled the patch

dawehner’s picture

:(

23:10:09 Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest   0 passes   1 fails                            

Status: Needs review » Needs work

The last submitted patch, 81: 2469713-81.patch, failed testing.

elachlan’s picture

Does BrowserWithJavascriptTest need to extend from JavascriptTestBase?

dawehner’s picture

Does BrowserWithJavascriptTest need to extend from JavascriptTestBase?

Well yeah I think it should!
Sadly the toolbar one also failed ...

elachlan’s picture

One problem at a time, are you able to resubmit with the change?

dawehner’s picture

Not in this night, feel free to do it ...

elachlan’s picture

Status: Needs work » Needs review
FileSize
909 bytes
930 bytes

Changed BrowserWithJavascriptTest to extend from JavascriptTestBase.

Status: Needs review » Needs work

The last submitted patch, 88: 2469713-88.patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
11.23 KB

Sorry, didn't do the patch right. Interdiffs are hard :(

Status: Needs review » Needs work

The last submitted patch, 90: 2469713-90.patch, failed testing.

elachlan’s picture

FileSize
11.23 KB

I actually accidentally didn't make the change. I am really sorry.

elachlan’s picture

Status: Needs work » Needs review

The last submitted patch, 21: 2469713-20.patch, failed testing.

The last submitted patch, 27: 2469713-27.patch, failed testing.

The last submitted patch, 30: 2469713-30.patch, failed testing.

The last submitted patch, 35: 2469713-35.patch, failed testing.

The last submitted patch, 88: 2469713-88.patch, failed testing.

The last submitted patch, 46: 2469713-46.patch, failed testing.

The last submitted patch, 81: 2469713-81.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 92: 2469713-92.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
711 bytes
10.81 KB
+++ b/core/modules/simpletest/src/JavascriptTestBase.php
@@ -0,0 +1,46 @@
+    parent::setUp();
...
+    // Setup some template cache using by the phantomjs mink driver.
+    $path = '/tmp/browsertestbase-templatecache';
+    if (!file_exists($path)) {
+      mkdir($path);
+    }

parent::setUp inits mink, which looks for the templatecache, which we haven't created yet

elachlan’s picture

Output from test. We are getting somewhere.

01:11:35 PHP Fatal error:  Uncaught exception 'GuzzleHttp\Exception\ConnectException' with message 'cURL error 7: Failed to connect to 127.0.0.1 port 8510: Connection refused (see http://curl.haxx.se/libcurl/c/libcurl-errors.html)' in /var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:186
01:11:35 Stack trace:
01:11:35 #0 /var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php(150): GuzzleHttp\Handler\CurlFactory::createRejection(Object(GuzzleHttp\Handler\EasyHandle), Array)
01:11:35 #1 /var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php(103): GuzzleHttp\Handler\CurlFactory::finishError(Object(GuzzleHttp\Handler\CurlMultiHandler), Object(GuzzleHttp\Handler\EasyHandle), Object(GuzzleHttp\Handler\CurlFactory))
01:11:35 #2 /var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php(180): GuzzleHttp\Handler\CurlFactory::finish(Object(GuzzleHttp\Handler\CurlMultiHandler), Object(GuzzleHttp\Handler\EasyHandle), Object(GuzzleHttp\Handler\CurlFactory))
01:11:37 #3 /var/www/html/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php in /var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php on line 93
01:11:37 Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest   0 passes   1 fails                            
01:11:37 Drupal\language\Tests\ConfigurableLanguageTest                 3 passes                  

Status: Needs review » Needs work

The last submitted patch, 102: 2469713-jtb.102.patch, failed testing.

elachlan’s picture

The failure is caused by phantomjs being disabled in #2580007: Add phantomjs2 to the testrunners.

The Maintainers will re-enable it and we should be able to test it again.

The last submitted patch, 102: 2469713-jtb.102.patch, failed testing.

Mixologic’s picture

#2580007: Add phantomjs2 to the testrunners Is in now, added a test to see if it works.

The last submitted patch, 102: 2469713-jtb.102.patch, failed testing.

elachlan’s picture

Failed because

00:01:45 Executing execute:command
00:01:45 Executing on container instance fc84c66b:
00:01:45 daemon -- nohup phantomjs --ssl-protocol=any --ignore-ssl-errors=true /var/www/html/vendor/jcalderonzumba/gastonjs/src/Client/main.js 8510 1024 768
00:01:45 Command created as exec id d8800da0
00:01:45 /bin/bash: daemon: command not found
Mixologic’s picture

yep. for some reason daemon, despite being in the php-base container, wasnt there. we've bandaided it by putting it in the .yml as an apt get install. But the test was re-ran and should actually overwrite the results that elachlan is referring to. (third test on #102)

elachlan’s picture

Congratulations :)

00:47:28 Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest   1 passes
larowlan’s picture

oh yeah!

elachlan’s picture

But also, we need more work on this.

00:58:10 Drupal\Tests\toolbar\Functional\ToolbarIntegrationTest         0 passes   1 fails                            
00:58:11 FATAL Drupal\Tests\toolbar\Functional\ToolbarIntegrationTest: test runner returned a non-zero error code (2).
00:58:11 Drupal\Tests\toolbar\Functional\ToolbarIntegrationTest         0 passes   1 fails                            
00:58:12 Drupal\tour\Tests\TourCacheTagsTest                           24 passes          

The last submitted patch, 102: 2469713-jtb.102.patch, failed testing.

larowlan’s picture

elachlan - are you working on this? I can - drop in on irc if you want to chat

lee

larowlan’s picture

Assigned: Unassigned » larowlan
larowlan’s picture

There was 1 error:

1) Drupal\Tests\toolbar\Functional\ToolbarIntegrationTest::testToolbarToggling
Zumba\GastonJS\Exception\JavascriptError: One or more errors were raised in the Javascript code on the page.
            If you don't care about these errors, you can ignore them by
            setting js_errors: false in your Poltergeist configuration (see documentation for details).
TypeError: 'undefined' is not a function (evaluating 'Drupal.toolbar.mediaQueryChangeHandler.bind(null, model, label)')
TypeError: 'undefined' is not a function (evaluating 'Drupal.toolbar.mediaQueryChangeHandler.bind(null, model, label)')
    at http://d8.dev/core/modules/toolbar/js/toolbar.js?v=8.0.0-dev:98
    at http://d8.dev/core/assets/vendor/jquery/jquery.min.js?v=2.1.4:2
    at http://d8.dev/core/assets/vendor/jquery/jquery.min.js?v=2.1.4:2
    at http://d8.dev/core/modules/toolbar/js/toolbar.js?v=8.0.0-dev:140
    at http://d8.dev/core/misc/drupal.js?v=8.0.0-dev:168
    at http://d8.dev/core/misc/drupal.js?v=8.0.0-dev:178
    at http://d8.dev/core/assets/vendor/domready/ready.min.js?v=1.0.8:4
elachlan’s picture

I guess we need to catch those and display meaningful output in the results?

larowlan’s picture

argh

/vagrant/app(2469713-jtb *): phantomjs --version
1.9.0
d8 /vagrant/app(2469713-jtb *): phantomjs
phantomjs> Function.prototype.bind
undefined
phantomjs>

We need a polyfill https://github.com/ariya/phantomjs/issues/10522

dawehner’s picture

Can't we just install a more or less recent version of phantomjs?

elachlan’s picture

We can use phantomjs2. Not sure if the driver supports that though.

I really think we should use NPM to install phantomjs2.

I am suggesting this because it would better support the move in #2609560: Base DCI containers off official containers.

dawehner’s picture

We can use phantomjs2. Not sure if the driver supports that though.

I always used the version 2 of it locally, so that should be alright.

pfrenssen’s picture

I'm also always using PhantomJS 2.0. I've read in the issue queue on Github that there are "some problems" with it, but I haven't encountered them yet. Also the 2.0 release was already made in January of this year, so I'm assuming that it has been further developed in the meanwhile, even though there doesn't seem to be any release made after that. The last commit on the project was 12 days ago so it is definitely still alive.

benjy’s picture

  1. +++ b/core/modules/simpletest/tests/src/Functional/BrowserWithJavascriptTest.php
    @@ -0,0 +1,45 @@
    +    $session->resizeWindow(400, 300);
    +    $session->wait(1000, 'false');
    

    I wonder if we should provide an abstraction on JavascriptTestBase for anything that requires a wait()?

    This would allow us to be consistent with our waits per different action, across all of our tests. That way, when we figure out that we only had to wait 50ms for a resizeWindow call, we can update it once and all our tests benefit?

  2. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarIntegrationTest.php
    @@ -0,0 +1,54 @@
    +    $this->assertTrue($this->getSession()->getDriver()->isVisible('.//a[@id="toolbar-link-system-admin_content"]'), 'Toolbar tray is open by default.');
    +    $this->getSession()->getDriver()->click('.//a[@id="toolbar-item-administration"]');
    

    Just a thought, $this->getSession()->getDriver()-> is quite verbose, how about a __call on BTB that allows us to do this: $this->isVisible() and proxy any methods that exist directly to the driver?

    EDIT: We could also use @method to get PHPStorm support on __call for the driver methods.

pfrenssen’s picture

@benjy, using wait() is a bad practice and can be avoided in almost all circumstances. It is usually very clear what you are waiting on. In most cases you are waiting for an element to appear in the DOM or vanish from it, or you might be waiting for a certain javascript variable to reach a certain value. You can implement a loop (typically called a spin() in this context) that polls the required state change at regular intervals (typically every 50-100ms) and returns as soon as the desired state is reached. In order to prevent deadlocks the spin function will accept a timeout parameter and will throw an exception when this timeout is reached, terminating the test with a failure.

So let's implementing a spin() method and use it instead of waiting :)

pfrenssen’s picture

FileSize
10.75 KB
912 bytes

I wanted to implement a spin() function but wasn't able to run the tests any more with the latest patch:

$ ./vendor/bin/phpunit -c core core/modules/simpletest/tests/src/Functional/BrowserWithJavascriptTest.php 
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

E

Time: 4.2 minutes, Memory: 59.75Mb

There was 1 error:

1) Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest::testJavascript
Behat\Mink\Exception\DriverException: Template cache /tmp/browsertestbase-templatecache directory does not exist

/home/pieter/v/drupal/drupal/vendor/jcalderonzumba/mink-phantomjs-driver/src/BasePhantomJSDriver.php:55
/home/pieter/v/drupal/drupal/vendor/jcalderonzumba/mink-phantomjs-driver/src/BasePhantomJSDriver.php:36
/home/pieter/v/drupal/drupal/core/modules/simpletest/src/BrowserTestBase.php:276
/home/pieter/v/drupal/drupal/core/modules/simpletest/src/BrowserTestBase.php:231
/home/pieter/v/drupal/drupal/core/modules/simpletest/src/JavascriptTestBase.php:

The reason for this is that the PHP process isn't allowed to write to the global /tmp folder on my system. Let's instead use the temporary folder that BrowserTestBase provides. That is guaranteed to work on all systems, and its also in line with how our other test frameworks handle this situation.

dawehner’s picture

Good idea. Now we just need to fix the phantomjs binary.

elachlan’s picture

I fixed it the other week, just waiting for a review.

benjy’s picture

@benjy, using wait() is a bad practice and can be avoided in almost all circumstances.

Yeah, it certainly felt a bit like an anti-pattern, what you suggested sounds good to me!

dawehner’s picture

@elachlan Oh I was just blind for a while. Thanks a lot!

pfrenssen’s picture

+++ b/core/includes/bootstrap.inc
@@ -677,8 +677,8 @@ function drupal_generate_test_ua($prefix) {
+  $check_string = $prefix . ':' . time() . ':' . $salt;

+++ b/core/modules/simpletest/src/JavascriptTestBase.php
@@ -0,0 +1,41 @@
+    $this->minkDefaultDriverArgs = [
+      'http://127.0.0.1:8510',
+      $path,
+    ];

Shall we make the host and port number for the PhantomJS process configurable? I guess this can be useful in some cases. How shall we do that? Source it from an environment variable?

dawehner’s picture

Shall we make the host and port number for the PhantomJS process configurable? I guess this can be useful in some cases. How shall we do that? Source it from an environment variable?

We could put it into an environment variable but set it by default into our phpunit.xml.dist file. In general we should try to reduce unnecessary additional required environment variables, and just make it possible to override it.

tunic’s picture

Shall we make the host and port number for the PhantomJS process configurable? I guess this can be useful in some cases. How shall we do that? Source it from an environment variable?

Yes, this is needed for example when using a CI tool with several projects. You can configure each project to use a different PhantomI JS instance on its own port , so projects can be tested simultaneously.

Wim Leers’s picture

Related issues: +#2628744: Test coverage

Note that once we have this, the BigPipe module will able to have proper tests, i.e. it won't have to simulate the behavior of its JS.

dawehner’s picture

Assigned: larowlan » dawehner

Assigning to me while debugging the failure.

dawehner’s picture

Assigned: dawehner » Unassigned
elachlan’s picture

Status: Needs work » Needs review

Setting to Needs Review to run tests on latest patch.

Status: Needs review » Needs work

The last submitted patch, 126: 2469713-126.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
10.78 KB

Rerolling #126.

elachlan’s picture

Status: Needs review » Needs work

Test Failed:

Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest   0 passes   1 fails                            
05:47:39 FATAL Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest: test runner returned a non-zero error code (2).
05:47:40 Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest   0 passes   1 fails                            
05:47:41 Drupal\language\Tests\EntityDefaultLanguageTest               11 passes

Edit:
Side note

05:58:29 Drupal\Tests\toolbar\Functional\ToolbarIntegrationTest         1 passes 

The last submitted patch, 139: 2469713-139.patch, failed testing.

Mixologic’s picture

An errant print_r($pageSize); seemed to cause the last patch to bail/fail.

Mixologic’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 142: 2469713-142.patch, failed testing.

Mixologic’s picture

Status: Needs work » Needs review
FileSize
10.75 KB
147 bytes

I really should know better than to manually edit a patch.

Mixologic’s picture

FileSize
547 bytes

And this is actually how an interdiff is made. IIRC.

dawehner’s picture

Shall we make the host and port number for the PhantomJS process configurable? I guess this can be useful in some cases. How shall we do that? Source it from an environment variable?

Do you mind open up a follow up for now?

elachlan’s picture

00:42:13 Drupal\Tests\toolbar\Functional\ToolbarIntegrationTest         0 passes   1 fails                            
00:42:13 Drupal\tracker\Tests\Migrate\d7\MigrateTrackerSettingsTest     5 passes                                      
00:42:16 Drupal\Tests\simpletest\Functional\BrowserWithJavascriptTest   0 passes   1 fails       

Status: Needs review » Needs work

The last submitted patch, 145: 2469713-145.patch, failed testing.

Mixologic’s picture

Okay, I think I see what might have gone wrong here.

The testbots have to run N tests at a time in parallel. Im not sure how mink and phantomjs handle multiple requests to the same port, and if there is some state stored in phantom JS that has to be retrieved across multiple connections to 8510, but if I change the toolbar test to be in the 'javascript' group, and do a repeat 2, the tests fail. If I do not repeat, or run them on their own, they pass.

Are we sure that phantomJS is going to be able to handle multiple simultaneous requests and responses? Do we need to spawn a separate phantomjs process per test?

elachlan’s picture

I see, Did a quick google:
http://stackoverflow.com/questions/28822262/does-phantomjs-share-memory-...

I think we need to implement multiple tabs, with a tab per test.

Mixologic’s picture

From the same page:

So if cookies or localStorage is an issue, this is not for you.

Tests will need to do things like log users in and out and be isolated from each other. Tabs cannot work.

elachlan’s picture

So we would have to create a phantomjs instance per test?

Or do we change the middle layer to have some sort of queue system?

elachlan’s picture

Isn't mink supposed to handle that?

Mixologic’s picture

I have no idea, but before we make any further changes to the testbots, this needs to work in a local environment with concurrency dialed up. run-tests.sh has a --repeat functionality in conjunction with --concurrency that should do the trick. If you can get that working locally, let us know what you've done - if it requires spawing new phantom js processes for each test or whatever works.

elachlan’s picture

@dawehner you are better positioned to explain this.

dawehner’s picture

Assigned: Unassigned » dawehner

I'm now trying to basically start one phantomjs process per javascript test. Its the only thing which will keep us sane IMHO.

dawehner’s picture

Assigned: dawehner » Unassigned

@longwave, @Mixologic and @dawehner talked on IRC about this issue

We agreed to split up the process into several steps:

  • Allow to run just the javascript tests: #2659100: Allow run-tests.sh to run just the javascript Functional tests
  • Create a new job type on drupalci_testbot to execute those tests. They could then be executed in a serial execution order
  • Once we have that people can write tests
  • On the longrun we need to lookup into a solution how to make those tests concurrent. We talked about both using a pool or phantomjs ports and starting one
    for each test, to getting rid of phantomjs and using selenium + selenium grid etc., but its a discussion which can be done independently from it. The important thing is to get javascript tests itself running at some point.
elachlan’s picture

@dawehner, sounds good.

I guess if we create a job type with a queue, then we can add the concurrency later. This could be done similar to how we currently do it, but we just launch a certain number of daemons based on the parameters.

dawehner’s picture

I guess if we create a job type with a queue, then we can add the concurrency later. This could be done similar to how we currently do it, but we just launch a certain number of daemons based on the parameters.

Well yeah, let's play around with ideas/solutions once we have serial testing working.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.67 KB

Given that javascript tests are so fundamental different I propose to make an additional testsuite for javascript functional tests.

This will also make #2659100: Allow run-tests.sh to run just the javascript Functional tests potentially easier.

dawehner’s picture

Here is the interdiff

Wim Leers’s picture

Issue tags: -rc eligible

Yay, progress! Awesome work :)

  1. +++ b/core/includes/bootstrap.inc
    @@ -617,9 +617,9 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    -  if (isset($user_agent) && preg_match("/^(simpletest\d+);(.+);(.+);(.+)$/", $user_agent, $matches)) {
    +  if (isset($user_agent) && preg_match("/^(simpletest\d+):(.+):(.+):(.+)$/", $user_agent, $matches)) {
    

    I had to read this three times to see the difference: from semi-colons to colons.

    Why this change?

  2. +++ b/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
    @@ -0,0 +1,39 @@
    +    $session->wait(1000, 'false');
    

    ->wait()… that's a rather unfortunate precedent for our very first JS test :P

  3. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
    @@ -0,0 +1,49 @@
    +class ToolbarIntegrationTest extends JavascriptTestBase {
    

    <3 <3 <3

dawehner’s picture

I had to read this three times to see the difference: from semi-colons to colons.
Why this change?

Pure 100% fun, see https://github.com/minkphp/Mink/pull/675

Basically, PhantomJS doens't handle cookie urlencoding as other stuff. Without it, the special cookies we set will be thrown away on the browser level.

->wait()… that's a rather unfortunate precedent for our very first JS test :P

Well, browser windows don't resize instant :)

dawehner’s picture

cosmicdreams’s picture

@dawehner you might want to consider a custom wait function that doesn't wait an arbitrarily long time. http://stackoverflow.com/questions/22291945/implementing-waitfor-functio...

dawehner’s picture

Some continues work, while trying to get the tests running on the testbot

Status: Needs review » Needs work

The last submitted patch, 167: 2469713-167.patch, failed testing.

dawehner’s picture

Title: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » [PP-2] Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary
Status: Needs work » Postponed
dawehner’s picture

Just a reroll

isntall’s picture

The patch in comment #170 does not apply.

error: patch failed: core/modules/simpletest/src/BrowserTestBase.php:29
error: core/modules/simpletest/src/BrowserTestBase.php: patch does not apply
dawehner’s picture

Rerolled in a minute

kostyashupenko’s picture

Status: Postponed » Needs review

The last submitted patch, 170: 2469713-170.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 172: 2469713-172.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.98 KB

Let's do a reroll, now that --types is in.

dawehner’s picture

Fixed some autoloader issue.

isntall’s picture

DCI should now support phantomJS tests.

dawehner’s picture

Nice @isntall Thanks a ton!

benjy’s picture

+++ b/core/phpunit.xml.dist
@@ -50,6 +50,17 @@
+    <testsuite name="functional-javascript">
+      <directory>./tests/Drupal/FunctionalJavascriptTests</directory>

Why not just Javascript? If we ever had anything like JS unit tests, they'd likely end up in a folder like my_module/tests/mocha/xyz.js

dawehner’s picture

Why not just Javascript? If we ever had anything like JS unit tests, they'd likely end up in a folder like my_module/tests/mocha/xyz.js

Well, my thoughts have been, if we add mocha / karma etc. these would be different kind of tests than the functional tests we have now. Much like we separate the unit tests from the kernel and the functional tests in PHP, IMHO we should try to keep a similar kind of structure for anything javascript related. Yes, you would not run any other kind of javascripttests with phpunit but at least giving it the right name is not the worst idea.

almaudoh’s picture

Title: [PP-2] Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary

Doesn't look like anything is blocking this now...

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Status: Needs review » Needs work
Issue tags: +Needs reroll
  1. Patch doesn't apply any more.
  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -240,6 +248,13 @@ protected function initMink() {
    +    // Fire up the first request to the front page in order to be able to  set
    +    // a cookie later.
    +    // @fixme This is done to circumvent:
    +    // WebDriver\Exception\UnableToSetCookie: {"errorMessage":"Unable to set Cookie: no URL has been loaded yet","request":{"headers":{"Accept":"application/json;charset=UTF-8","Content-Length":"164","Content-Type":"application/json;charset=UTF-8","Host":"127.0.0.1:8910"},"httpVersion":"1.1","method":"POST","post":"{\"cookie\":{\"name\":\"SIMPLETEST_USER_AGENT\",\"value\":\"simpletest472558;1441488302;55eb5dae78eeb7.36607058;IxhRvB7dhbDAMurfBshqgUwEOpAPPKybnJMv0JgaG8Q\",\"secure\":false}}","url":"/cookie","urlParsed":{"anchor":"","query":"","file":"cookie","directory":"/","path":"/cookie","relative":"/cookie","port":"","host":"","password":"","user":"","userInfo":"","authority":"","protocol":"","source":"/cookie","queryKey":{},"chunks":["cookie"]},"urlOriginal":"/session/91aef6b0-5414-11e5-9028-67c7fcbbdc3b/cookie"}}
    +    $session = $this->getSession();
    +    $session->visit($this->baseUrl);
    

    I don't know a better way of doing this. Isn't this out of our control, i.e. due to a limitation in Mink or PhantomJS?

  3. +++ b/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
    @@ -0,0 +1,39 @@
    +    $session->resizeWindow(400, 300);
    +    $session->wait(1000, 'false');
    

    We should address this one second wait.

  4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
    @@ -0,0 +1,38 @@
    +/**
    + * Runs a browser test using phantomJS.
    + *
    + * Use this test baseclass if you want to test expected behaviour of some
    + * JavaScript.
    + */
    

    Nitpick: there are different capitalizations used of "javascript", "mink" and "phantomjs" throughout the patch. Let's look up the official spelling :)

  5. +++ b/core/tests/README.md
    @@ -0,0 +1,14 @@
    +## Function tests
    

    Functional tests

  6. +++ b/core/tests/README.md
    @@ -0,0 +1,14 @@
    +```
    +* Run the function tests:
    +```
    

    Run the functional tests.

Assigning to me, going to address this after lunch.

pfrenssen’s picture

Issue tags: -Needs reroll

Oh the patch is rolled against 8.0.x? Indeed testing is exempt from the rules against adding new features. Then it applies :)

pfrenssen’s picture

Addressed my remarks. I researched the workaround for visiting the front page before a cookie can be set, and it turns out it is required to do this by the W3C WebDriver specification, so it is perfectly fine to do this. I've added a comment with more information. I also updated the README so the instructions are now correct for running the entire functional test suite. I also went through the patch with my finest comb to straighten out any last wrinkles.

This looks pretty ready to me now :)

jibran’s picture

+++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
@@ -0,0 +1,52 @@
+    $this->assertTrue($this->getSession()->getDriver()->isVisible('.//a[@id="toolbar-link-system-admin_content"]'), 'Toolbar tray is open by default.');
+    $this->getSession()->getDriver()->click('.//a[@id="toolbar-item-administration"]');
+    $this->assertFalse($this->getSession()->getDriver()->isVisible('//a[@id="toolbar-link-system-admin_content"]'), 'Toolbar tray is closed after clicking the "Manage" button.');
+    $this->getSession()->getDriver()->click('.//a[@id="toolbar-item-administration"]');
+    $this->assertTrue($this->getSession()->getDriver()->isVisible('//a[@id="toolbar-link-system-admin_content"]'), 'Toolbar tray is visible again after clicking the "Manage" button a second time.');
...
+    $this->assertTrue($this->getSession()->getDriver()->isVisible('.//div[@id="toolbar-item-administration-tray" and contains(concat(" ", @class, " "), " toolbar-tray-horizontal ")]'), 'Toolbar tray is horizontally oriented by default.');
+    $this->assertEmpty($this->getSession()->getDriver()->find('.//div[@id="toolbar-item-administration-tray" and contains(concat(" ", @class, " "), " toolbar-tray-vertical ")]'), 'Toolbar tray not vertically oriented by default.');
...
+    $this->getSession()->getDriver()->click('.//div[@id="toolbar-item-administration-tray"]//button[contains(concat(" ", @class, " "), " toolbar-icon-toggle-vertical ")]');
...
+    $this->assertTrue($this->getSession()->getDriver()->isVisible('.//div[@id="toolbar-item-administration-tray" and contains(concat(" ", @class, " "), " toolbar-tray-vertical ")]'), 'After toggling the orientation the toolbar tray is now displayed vertically.');
...
+    $this->getSession()->getDriver()->click('.//div[@id="toolbar-item-administration-tray"]//button[contains(concat(" ", @class, " "), " toolbar-icon-toggle-horizontal ")]');
...
+    $this->assertTrue($this->getSession()->getDriver()->isVisible('.//div[@id="toolbar-item-administration-tray" and contains(concat(" ", @class, " "), " toolbar-tray-horizontal ")]'), 'After toggling the orientation a second time the toolbar tray is displayed horizontally again.');

Can we use CssSelector here instead?

pfrenssen’s picture

Sure! Good suggestion @jibran, since these two tests will serve as examples CssSelector will make it much friendlier for people new to JS testing.

dawehner’s picture

Thank you @pfrenssen

pfrenssen’s picture

FileSize
14 KB
7.89 KB

Converted XPath to CSS selectors. Also provided some helper methods to make writing tests much more developer friendly. I wouldn't try to put a full suite of helper methods in now, we can expand on this in followup tickets.

Looking at how the code reads now CSS selectors are indeed so much more friendly to read. I've been writing XPath expressions for so long that I sometimes forget how alien and unfriendly these are for people that are not used to them.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
pfrenssen’s picture

FileSize
14 KB
2.37 KB

Weird I just commented but my session got logged out. Apologies if this comes up double.

Made some small cleanups to the CSS selectors.

Mixologic’s picture

Not sure why, but the testrunner is *not* finding the JavaScript tests.

dawehner’s picture

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -33,4 +33,90 @@ protected function initMink() {
+  protected function assertElementPresent($css_selector, $message = '') {
...
+  protected function assertElementNotPresent($css_selector, $message = '') {
...
+  protected function click($css_selector) {
...
+  protected function selectorToXpath($css_selector) {

All of those should land in BrowserTestBase

jibran’s picture

+1 to #193.

pfrenssen’s picture

FileSize
14.22 KB
4.47 KB

Great idea, moved them to BrowserTestBase.

dawehner’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -1356,6 +1356,53 @@ protected function drupalUserIsLoggedIn(UserInterface $account) {
+  protected function selectorToXpath($css_selector) {
+    return $this->getSession()->getSelectorsHandler()->selectorToXpath('css', $css_selector);
+  }

That one is a bit weird, because this is exactly what the symfony css component is doing.

pfrenssen’s picture

@Mixologic, I checked the phpunit.xml.dist for any clue why the tests are not running on DCI but it all seems OK. Strange.

pfrenssen’s picture

FileSize
13.87 KB
3.62 KB

Addressed #196.

Mixologic’s picture

Okay, just to clarify, currently the way drupalci is handling these phantomJS tests is to run run-tests.sh a second time with just the js tests because they cannot have concurrency.

00:31:55.510 cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh  --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/jenkins_default_97850 --color --keep-results --concurrency 1 --keep-results-table --sqlite /var/www/html/results/simpletest.sqlite --types PHPUnit-FunctionalJavascript --all
00:31:55.515 Command created as exec id 3bf39031
00:31:56.623   ERROR: No valid tests were specified.

Is what the latest test shows in https://dispatcher.drupalci.org/job/default/97850/consoleFull

So, right now, none of these javascript tests are running on drupalci.

dawehner’s picture

This fixes it.

Status: Needs review » Needs work

The last submitted patch, 200: 2469713-200.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.66 KB
559 bytes

There we go, a small problem in the latest changes from @pfrenssen

pfrenssen’s picture

Great! It works!!

Sam152’s picture

Very excited for JTB. Great work all involved.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Yay!!!

kim.pepper’s picture

Long time follower of this issue. Thanks for all the great work everyone!

tstoeckler’s picture

Just to save people some clicking, here's the proof from https://dispatcher.drupalci.org/job/default/98370/console:

15:43:13 cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh  --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/jenkins_default_98370 --color --keep-results --concurrency 1 --keep-results-table --sqlite /var/www/html/results/simpletest.sqlite --types PHPUnit-FunctionalJavascript --all
15:43:13 Command created as exec id aa0f3b76
15:43:14 
15:43:14 Drupal test run
15:43:14 ---------------
15:43:14 
15:43:14 All tests will run.
15:43:14 
15:43:14 Test run started:
15:43:14   Monday, March 14, 2016 - 15:43
15:43:14 
15:43:14 Test summary
15:43:14 ------------
15:43:14 
15:43:20 Drupal\Tests\simpletest\FunctionalJavascript\BrowserWithJava   1 passes                                      
15:43:31 Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegration   1 passes                                      
15:43:31 
15:43:31 Test run duration: 16 sec
alexpott’s picture

Patch didn't apply to 8.2.x where it needs to go first - plus a couple of coding standards fixes - yes we're going to remove @file docblocks but until then core should be consistent.

As my changes are extremely minor leaving at rtbc.

dawehner’s picture

Thank you alex! You are right, I changed my templates in storm already for custom code, so I forgot to add this back for core work.

alexpott’s picture

Tests are still working on 8.2.x... https://dispatcher.drupalci.org/job/default/98893/console

12:20:34 cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh  --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/jenkins_default_98893 --color --keep-results --concurrency 1 --keep-results-table --sqlite /var/www/html/results/simpletest.sqlite --types PHPUnit-FunctionalJavascript --all
12:20:34 Command created as exec id e8066be4
12:20:35 
12:20:35 Drupal test run
12:20:35 ---------------
12:20:35 
12:20:35 All tests will run.
12:20:35 
12:20:35 Test run started:
12:20:35   Tuesday, March 15, 2016 - 12:20
12:20:35 
12:20:35 Test summary
12:20:35 ------------
12:20:35 
12:20:42 Drupal\Tests\simpletest\FunctionalJavascript\BrowserWithJava   1 passes                                      
12:20:52 Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegration   1 passes                                      
12:20:53 
12:20:53 Test run duration: 17 sec
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3436dfd and pushed to 8.1.x and 8.2.x. Thanks!

Looking forward to having a test for #states in core :)

alexpott’s picture

The postgres fail on #208 was an unrelated random fail that was cause by a patch that has now been reverted.

alexpott’s picture

Issue tags: +8.1.0 release notes

@xjm suggested we should tell people about this in the release notes - on the whole I agree although I have some reservations around testbot resource utilisation, just how experimental this is and how DrupalCI/run-test.sh reports fails from PHPUnit based tests.

  • alexpott committed e6f491d on 8.2.x
    Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic, larowlan,...

  • alexpott committed 3436dfd on 8.1.x
    Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic, larowlan,...

  • alexpott committed 11d0d76 on 8.2.x
    Revert "Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic,...
alexpott’s picture

Status: Fixed » Needs work

Had to roll this back because contrib tests are broken :( see https://dispatcher.drupalci.org/job/default/98960/console

alexpott’s picture

alexpott’s picture

+++ b/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
@@ -0,0 +1,39 @@
+    $result = $session->wait(1000, $javascript);

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -0,0 +1,81 @@
+  /**
+   * Waits for the given time or until the given JS condition becomes TRUE.
+   *
+   * @param int $timeout
+   *   Timeout in milliseconds.
+   * @param string|bool $condition
+   *   JS condition, or FALSE to wait for the full duration of the timeout.
+   *
+   * @return bool
+   *   The result of the JS condition.
+   */
+  protected function wait($timeout, $condition = FALSE) {
+    return $this->getSession()->getDriver()->wait($timeout, $condition);
+  }

Shouldn't the ->wait call use the method on the test base? Or what is the point of the method on the test base?

pfrenssen’s picture

alexpott’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
15.05 KB

Here are the changes that needed applying due to the reviews since #208.

I wonder if JavascriptTestBase::wait() should become ::assertJsCondition() and that assertion should take a timeout to wait for it so be true? And we should default this to 0?

pfrenssen’s picture

I wonder if JavascriptTestBase::wait() should become ::assertJsCondition() and that assertion should take a timeout to wait for it so be true? And we should default this to 0?

On the one hand, ::wait($timeout, $condition) matches the function that is used in other WebDriver implementations, so this is familiar for people that have prior experience with writing browser tests (e.g. Selenium, Mink, or even test frameworks in other programming languages).

On the other hand, wait() encourages bad practices such as hardcoding a waiting period of N seconds for some action to occur. This is common in test code but it is never the correct solution, since you are usually waiting for a specific change to occur to the DOM. Waiting longer than strictly necessary is just a waste of time, and it can also introduce random failures if the action randomly takes longer than expected.

I am in favour of renaming this to ::assertJsCondition(). Defaulting to a timeout of 0 is not a good idea though, changes made by JS in the DOM are not instantaneous. They are usually very fast, but on heavy pages it might take a few milliseconds and then the assertion will fail. I always use a 1000ms timeout for simple DOM actions since on some third-party services (e.g. BrowserStack) the tests run so slowly that even the simplest action might take 100ms or more.

benjy’s picture

So we never got the spin discussed in #125? That seemed like the best approach of all, maybe worth re-visiting?

Status: Needs review » Needs work

The last submitted patch, 222: 2469713-3-222.patch, failed testing.

pfrenssen’s picture

@benjy, the wait() method performs the spin, it is implemented in the webdriver, it will typically poll every 50ms for the JS condition to become true. The method will return as soon as the condition becomes true or the timeout expires.

benjy’s picture

Ah OK, I was confused when you said "I always use 1000ms timeout" because it sounded like it would always wait for that long, but in actual fact, that's only in the worst case, most times it will be much quicker.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
15.15 KB

Here's the ->wait() changed to an assertion - it also always us to have a decent log message rather than lots of asserting true is true.

Sample output when made to fail... (by changing assertTrue to assert False)

phpunit /Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
PHPUnit 4.8.6 by Sebastian Bergmann and contributors.

F

Time: 6.2 seconds, Memory: 13.50Mb

There was 1 failure:

1) Drupal\Tests\simpletest\FunctionalJavascript\BrowserWithJavascriptTest::testJavascript
Javascript condition met:
    (function(){
        var w = window,
        d = document,
        e = d.documentElement,
        g = d.getElementsByTagName('body')[0],
        x = w.innerWidth || e.clientWidth || g.clientWidth,
        y = w.innerHeight || e.clientHeight|| g.clientHeight;
        return x == 400 && y == 300;
    }());
Failed asserting that true is false.

/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/Constraint.php:110
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/Constraint.php:56
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/Assert.php:2183
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/Assert.php:943
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php:79
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php:35
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestCase.php:889
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestCase.php:749
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:601
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestCase.php:705
alexpott’s picture

Proof from the last test run that the tests have run...

12:04:33 cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh  --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/jenkins_default_99427  --color --keep-results --color --concurrency 31 --sqlite /var/www/html/results/simpletest.sqlite --php /opt/phpenv/shims/php --concurrency 1 --keep-results-table --types PHPUnit-FunctionalJavascript --all
12:04:33 Command created as exec id e66327c5
12:04:34 
12:04:34 Drupal test run
12:04:34 ---------------
12:04:34 
12:04:34 All tests will run.
12:04:34 
12:04:34 Test run started:
12:04:34   Wednesday, March 16, 2016 - 12:04
12:04:34 
12:04:34 Test summary
12:04:34 ------------
12:04:34 
12:04:41 Drupal\Tests\simpletest\FunctionalJavascript\BrowserWithJava   1 passes                                      
12:04:51 Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegration   1 passes                                      
12:04:51 
12:04:51 Test run duration: 16 sec
12:04:51 
alexpott’s picture

Let's have a test that proves the assertJsCondition can fail...

pfrenssen’s picture

Looking good! Just one little thing...

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -71,11 +71,12 @@ protected function assertElementNotVisible($css_selector, $message = '') {
+  protected function assertJsCondition($condition, $timeout = 1000, $message = '') {

The $message parameter should be documented.

Status: Needs review » Needs work

The last submitted patch, 230: 2469713-3-230.patch, failed testing.

alexpott’s picture

Here's a patch with the missing docs added.

I have no idea why #230 failed :( Passes locally for me.

d8test --all --types PHPUnit-FunctionalJavascript

Drupal test run
---------------

All tests will run.

Test run started:
  Wednesday, March 16, 2016 - 14:58

Test summary
------------

Drupal\Tests\simpletest\FunctionalJavascript\BrowserWithJava   2 passes
Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegration   1 passes

Test run duration: 25 sec

Status: Needs review » Needs work

The last submitted patch, 233: 2469713-3-233.patch, failed testing.

Mixologic’s picture

The phantomjs process was not staying alive for some reason. I've adjusted how it keeps itself in place, so those failures above were a result of the phantomjs process not existing.

We deployed a fix, and phantomjs should actually be there now.

Mixologic’s picture

Status: Needs work » Needs review
pfrenssen’s picture

For me it looks good now, but I can't RTBC this.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Actually, since this was already committed, and I have not participated in the new version of the patch it is probably OK for me to RTBC this.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5a223b9 and pushed to 8.1.x and 8.2.x. Thanks!

I've committed this because it's not runtime code and we learn more by having this committed than not.

  • alexpott committed 9c9fae7 on 8.2.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...

  • alexpott committed 5a223b9 on 8.1.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...

  • alexpott committed e2a1974 on 8.1.x
    Revert "Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan,...
alexpott’s picture

Status: Fixed » Reviewed & tested by the community

Reverted because the phantomjs going missing issue is not resolved... https://www.drupal.org/pift-ci-job/215899

xjm’s picture

So is this actually RTBC or blocked?

xjm’s picture

Issue tags: +beta target
dawehner’s picture

xjm’s picture

Status: Reviewed & tested by the community » Postponed

So I guess it should be postponed until that is fixed?

nevergone’s picture

:'(

alexpott’s picture

Status: Postponed » Reviewed & tested by the community

#2580007: Add phantomjs2 to the testrunners is done... The only way we're going to find out is committing this.

alexpott’s picture

Issue tags: -beta target +rc target

Discussed with @catch - we think this should be eligible for commit because it was an 8.1.0 target, it was held up in improvements to DrupalCI and has been committed several times already, it is a new test API will not break runtime.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

3rd time lucky?

Committed 94fb7c6 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed ca48ed6 on 8.2.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...

  • alexpott committed 94fb7c6 on 8.1.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...
dawehner’s picture

alexpott’s picture

Shameless plug: if you are looking for a how to run tests locally https://www.chapterthree.com/blog/javascript-testing-comes-to-drupal-8

xjm’s picture

Title: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » [Needs change record] Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary
Status: Fixed » Needs work
Issue tags: +Needs change record, +Needs change record updates

This issue is missing a change record. There is an existing one with related info that also needs to be updated at: https://www.drupal.org/node/2469723

BLACKBERRY_54CA’s picture

Status: Needs work » Active
dawehner’s picture

Status: Active » Needs work
jibran’s picture

dawehner’s picture

Issue tags: -Needs change record

We have one now, but of course we can expand it

dawehner’s picture

Title: [Needs change record] Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary
xjm’s picture

Title: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » [Needs change record updates] Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary

Adding back a title prefix since this keeps confusing me by being open. :)

webchick’s picture

Guys. :( We really need some docs on how to write these tests. :( ATM the best guide is "use the source, Luke" and we're blocking great UX improvements like #2421427: Improve the UX of Quick Editing single-valued image fields on that. I got an offer on Twitter from someone to help write these, and the best I could come up with was the out of date change record, a non-d.o resource, and a list of classes that sub-class BTB. Help. :(

xjm’s picture

It looks like @dawehner had drafted https://www.drupal.org/node/2716803 (attached to this issue) and it just hadn't gotten reviewed/published. I published it now, so that should help some. Maybe others can add some things there.

Some API topic or handbook developer guide docs would be really helpful for sure.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.73 KB

Here is a patch for core.api.php, which itself is horrible out of date, as it still describes old simpletest.

  • alexpott committed 11d0d76 on 8.3.x
    Revert "Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic,...
  • alexpott committed 9c9fae7 on 8.3.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...
  • alexpott committed e6f491d on 8.3.x
    Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic, larowlan,...
  • alexpott committed e8ae668 on 8.3.x
    Revert "Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan,...
  • alexpott committed ca48ed6 on 8.3.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...

  • alexpott committed 11d0d76 on 8.3.x
    Revert "Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic,...
  • alexpott committed 9c9fae7 on 8.3.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...
  • alexpott committed e6f491d on 8.3.x
    Issue #2469713 by dawehner, pfrenssen, elachlan, Mixologic, larowlan,...
  • alexpott committed e8ae668 on 8.3.x
    Revert "Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan,...
  • alexpott committed ca48ed6 on 8.3.x
    Issue #2469713 by dawehner, pfrenssen, alexpott, elachlan, Mixologic,...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

elachlan’s picture

I am trying to implement Javascript testing for extlink in #2587531: Convert Automated Tests to BrowserTestBase and I am running into issues.

Is there no better documentation? Could someone have a quick look and see if I am doing anything obviously wrong?

I'll update the documentation once I get it all working and fill in any blanks that weren't too obvious to me.

almaudoh’s picture

The patch at #265 is quite helpful, but it seems it hasn't been committed yet.

dawehner’s picture

@almaudoh
Do you want to post the patch in a new issue so we can close this one?

xjm’s picture

@dawehner's patch is in #2810621: Improve core.api.php documentation about browsertests and javascript tests. I think this is still open for review of the CR.

Gábor Hojtsy’s picture

Title: [Needs change record updates] Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary
Status: Needs review » Fixed
Issue tags: -Needs change record updates

Updated both https://www.drupal.org/node/2716803 and https://www.drupal.org/node/2469723 (especially the later) by removing outdated info, crosslinking them and linking to the PHPUnit initative.

dawehner’s picture

@Gábor Hojtsy Thanks a ton

Status: Fixed » Closed (fixed)

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