Provide a better way to validate all javascript activity is completed

Thanks @michielnugter.

Given the number of issues we've had related to this issue, this is a major task.

Lets see how this does.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -24,23 +24,70 @@ class JSWebAssert extends WebAssert {
+        function IsNotAjaxing(instance, index, array) {

Can we move this to ajax.js?

This takes care of the fail locally. Not sure how Drupal can be undefined though, but it is.

It is a good idea to ask JS maintainers. +1 for moving it to Drupal. How about Drupal.waitForResponse() or Drupal.processingRequest()?

Thanks @Lendude. This looks good.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -24,23 +24,74 @@ class JSWebAssert extends WebAssert {
+        if (typeof Drupal === 'undefined') {
...
+            typeof jQuery == "undefined" || (

We should add Drupal and jQuery checks on the same level.

@michielnugter nice cleanup, bit of nitpicking:

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,67 @@ class JSWebAssert extends WebAssert {
   +   *   When the activity is not completed. If left blank, a default message will
   +   *   be displayed.
   

   The second sentence seems to belong to the $message @param.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,67 @@ class JSWebAssert extends WebAssert {
   +    $condition = <<<JS
   

   The whole condition could probably use some comments about what we are doing and why.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,67 @@ class JSWebAssert extends WebAssert {
   +              (typeof Drupal.ajax === 'undefined' || Drupal.ajax.instances.every(IsNotAjaxing))              ¶
   

   Trailing whitespace (PHPStorm doesn't clean up trailing whitespace in HEREDOCs, at least mine doesn't)

And +1 for moving the 'isAjaxing' test to Drupal. but maybe in a follow-up? Would be a shame to put the test improvement on hold while sorting that out.

Some more nitpicks (missed a couple the first time):

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,73 @@ class JSWebAssert extends WebAssert {
   +   *   This method is deprecated in favor or waitForJavascriptActivity().
   ...
   +   *   This method is deprecated in favor or waitForJavascriptActivity().
   

   'or' should be 'of'

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,73 @@ class JSWebAssert extends WebAssert {
   +   * Waits for Javascript activity to completed.
   

   completed => complete (or 'be completed')

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,73 @@ class JSWebAssert extends WebAssert {
   +        * Helper method for asserting there are no Drupal ajax requests running.
   ...
   +              // Assert no ajax request is running through jQuery.
   ...
   +              // Assert Drupal.ajax is finished with all the ajax requests.
   

   ajax => AJAX

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,73 @@ class JSWebAssert extends WebAssert {
   +JS;
   +    ¶
   +    $result = $this->session->wait($timeout, $condition);
   

   whitespace snuck in again

+++ b/core/misc/debounce.js
@@ -40,8 +40,14 @@ Drupal.debounce = function (func, wait, immediate) {
+      Drupal.debounceCount--;
...
+    if (!timeout) {
+      Drupal.debounceCount++;
+    }

Not a good idea. if it's a must, we could override it in test driver only. We didn't force our code to use Drupal.debounce also. It can be _.debounce

the JS engine simulated real behavior. The debounce works never based on another counter. We must see the feedback on the browser. The JS test engine should catch the result rather than a hidden counter we never used in our codebase.

Something like these helpers:
http://codeception.com/docs/modules/WebDriver#waitForElement

In Drupal's test driver is:

$this->session->wait(300, CONDITION);

About the anime condition:

jQuery.active === 0 && jQuery(':animated').length === 0

this condition isn't required but it could help for testing performance. For example, we could set 30 second timeout here. And shotern $this->session->wait(3000, CONDITION).

If networking & anime is stopping after 5 sec, it seldom waiting for another 3s for further rendering. If it does, a BIG UX problem.

I assumed these checks were well thought out.

We'll not exactly. We have it in drupal-extension so it was copied from there. It got added in https://github.com/jhedstrom/drupalextension/pull/30 by @grasmash so I think we should ping him.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,73 @@ class JSWebAssert extends WebAssert {
   +   *
   +   * @deprecated
   +   *   This method is deprecated in favor of waitForJavascriptActivity().
   

   Should be "@deprecated Scheduled for removal in Drupal 9.0.0. Use XYZ instead"

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -24,23 +24,73 @@ class JSWebAssert extends WebAssert {
   +    $condition = <<<JS
   +      (function() {
   

   Having so much JS code as inline string is hard to read. Please put it in a dedicated JS file for better maintainability.

Overall I think that we should avoid calling these ajax completion assertion. Instead, we should test for UI elements as they show up. Example: if you test an autocomplete then you wait until the list of suggestions shows up in the DOM. You don't care that AJAX stuff or debounce stuff is going on.

However, there are some interesting edge cases as experienced in #2832672: [PP-1] Upgrade jcalderonzumba/* for better test performance. How do you wait for a link to be associated with some JS behavior before you are allowed to click it?

+1 on the latest direction it looks neat and self-contained.

Here are some nits and patch attached that fixes them:

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -6,6 +6,8 @@
   +use Behat\Mink\Selector\Xpath\Manipulator;
   
   @@ -14,6 +16,17 @@
   +   * @var Manipulator
   

   Needs to be fully qualified. so \Behat\Mink\Selector\Xpath\Manipulator plus should have a description.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +          // Assert no AJAX request is running (via jQuery or Drupal) and no ¶
   

   Space at the end of the line

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +   *   selector engine name
   ...
   +   * @see ElementInterface::findAll for the supported selectors
   

   Should be just
   @see \Behat\Mink\Element\ElementInterface::findAll()
   @see does not support additional text.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +   *   selector engine name
   

   Should be a full sentence - ie. start with a Capital and finish with a fullstop. Plus this one could say See ElementInterface::findAll() for the supported selectors. Which would then compliment the @see below.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +   * @return NodeElement|null
   ...
   +   * @return NodeElement|null
   ...
   +   * @return NodeElement|null
   ...
   +   * @return NodeElement|null
   ...
   +   * @return NodeElement|null
   

   Should be \Behat\Mink\Element\NodeElement. @return's need a sentence description too.

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +    $condition = "document.evaluate(\"" . str_replace(PHP_EOL, '', $xpath) . "\", document, null, XPathResult.BOOLEAN_TYPE,null).booleanValue";
   

   Let's make the spacing after commas consistent. Also this line would be simpler to read if the JS strings used ' instead of " so there's no need for escaping the double quotes \".

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +   * Waits for a button (input[type=submit|image|button|reset], button) with
   +   * specified locator and returns it.
   ...
   +   * Waits for a field (input, textarea, select) with specified locator and
   +   * returns it when available.
   

   Needs to be a one line summary.

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +39,109 @@ class JSWebAssert extends WebAssert {
   +   * Wait for an element by its id and returns it when available.
   

   Waits...

Update with partial coverage for JSWebAssert. All new / changed methods have coverage, existing methods not (yet).

Let's open a followup to add coverage for existing methods - that might be might out-of-scope here.

Patch attached fixes coding standards and the Drupal\system\Tests\Module\InstallUninstallTest - test modules need to be in the Testing package.

Another little nit.

I think we're good to go here - there's test coverage and evidence on the issue that the changes improve things. I've run the new test locally over 100 times - no fails.

I consider my contributions here all tidy ups so I'm in a good position to rtbc.

1. +++ b/core/modules/system/tests/modules/js_webassert_test/src/Form/JsWebAssertTestForm.php
   index a116c6f..1c70932 100644
   --- a/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
   
   --- a/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
   +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
   

   Those are unrelated fixes from #2831603: Improve JSWebAssert::waitOnAutocomplete. I think we don't need them for this patch?

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,117 @@ class JSWebAssert extends WebAssert {
   +    $condition = <<<JS
   +      (function() {
   +        function IsNotAjaxing(instance, index, array) {
   +          "use strict";
   +          return instance ? !instance.ajaxing : true;
   +        }
   +        return (
   +          // Assert no AJAX request is running (via jQuery or Drupal) and no
   +          // animation is running.
   +          (typeof jQuery === "undefined" || (jQuery.active === 0 && jQuery(':animated').length === 0)) &&
   +          (typeof Drupal === 'undefined' || typeof Drupal.ajax === 'undefined' || Drupal.ajax.instances.every(IsNotAjaxing))
   +        );
   +      }());
   

   I think we have too many inline JS lines here, which makes it very hard to read and maintain this code. I think we should put this in a dedicated JS file and use file_get_contents() here to load it.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,117 @@ class JSWebAssert extends WebAssert {
   +   *
   +   * @deprecated in Drupal 8.3.x, will be removed before Drupal 9.0.0.
       */
   

   This should have instructions what people should use instead. Are they supposed to use for example waitForElement() now?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,117 @@ class JSWebAssert extends WebAssert {
   +    // Wait for the autocomplete to be visible.
   +    $condition = "(jQuery('.ui-autocomplete:visible li').length > 0)";
   +    return $this->session->wait(10000, $condition);
   

   This should use waitForElement() now, right?

The remaining tasks in the issue summary still has some things not marked as done. It sounds like they're actually done? :)

1. +++ b/core/modules/system/tests/modules/js_webassert_test/js/js_webassert_test.js
   @@ -0,0 +1,22 @@
   + * Attaches the behaviors for the Field UI module.
   ...
   +   *   Adds behaviors to the field storage add form.
   

   These are stale comments.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
   @@ -42,6 +42,23 @@ protected function setUp() {
   +  protected function assertResultCount($count, $field_name) {
   

   "result count" sounds very generic.

   What about assertAutocompleteResultCount?

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
   @@ -66,11 +83,11 @@ public function testEntityReferenceAutocompleteWidget() {
   +    // Assert Autocomplete Popup is shown.
   
   @@ -90,11 +107,11 @@ public function testEntityReferenceAutocompleteWidget() {
   +    // Assert Autocomplete Popup is shown.
   

   Unnecessary capitalization. And probably also unnecessary comments.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,117 @@ class JSWebAssert extends WebAssert {
   +          // Assert no AJAX request is running (via jQuery or Drupal) and no
   +          // animation is running.
   +          (typeof jQuery === "undefined" || (jQuery.active === 0 && jQuery(':animated').length === 0)) &&
   +          (typeof Drupal === 'undefined' || typeof Drupal.ajax === 'undefined' || Drupal.ajax.instances.every(IsNotAjaxing))
   

   Woah, clever!

Any good reason to make these shortcuts? Can't it just passing the CSS selectors? KISS :)

Drupal\FunctionalJavascriptTests\JSWebAssert::waitForButton
Drupal\FunctionalJavascriptTests\JSWebAssert::waitForLink
Drupal\FunctionalJavascriptTests\JSWebAssert::waitForField
Drupal\FunctionalJavascriptTests\JSWebAssert::waitForId

Personally, I love the Codeception API design
http://codeception.com/docs/modules/WebDriver#waitForElement

Notable:
waitForElement
Appeared in HTML

waitForElementVisible
Appeared in HTML and HUMAN eye

Theoretically, we should remove `assertWaitOnAjaxRequest()` and promote to use waitForElement. ( assertWaitOnAjaxRequest only useful if it improves test performance. And if so, it can merge into waitForElement with a simple condition)

failure of Autocomplete may be a good example. After Ajax is completed, there're other factors affected. We must verify and give time PhantomJS to render the page and execute JS ..etc

Really great work @michielnugter. Great to see the patch is ready. We are going to need a change notice.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -26,21 +41,114 @@ class JSWebAssert extends WebAssert {
+      (function() {
+        function IsNotAjaxing(instance, index, array) {
+          "use strict";
+          return instance ? !instance.ajaxing : true;
+        }
+        return (
+          // Assert no AJAX request is running (via jQuery or Drupal) and no
+          // animation is running.
+          (typeof jQuery === "undefined" || (jQuery.active === 0 && jQuery(':animated').length === 0)) &&
+          (typeof Drupal === 'undefined' || typeof Drupal.ajax === 'undefined' || Drupal.ajax.instances.every(IsNotAjaxing))
+        );
+      }());

Let's move this to JS.

Re #54 and #50.2 let's not do that here. There is plenty of existing javascript in JsWebAssert - we should try and handle that altogether in the same issue.

Of course I'm not sure yet on random failures.

I bet that came from PhantomJS. I tried diff way to delay the responses about 4 ~ 8 secs and it still passed in my local tests.

(jQuery.active === 0 && jQuery(':animated').length === 0))
We may add this before waitforElement. A modal can be shown but not clickable if it's animating. (I hit this problem with my other apps.)

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,147 @@ class JSWebAssert extends WebAssert {
   +  public function waitForElementVisible($selector, $locator, $timeout = 10000) {
   

   Needs tests.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,147 @@ class JSWebAssert extends WebAssert {
   +    $element = $this->waitForElement($selector, $locator, $timeout);
   

   $element might be null

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -26,21 +41,147 @@ class JSWebAssert extends WebAssert {
   +    $end = $start + $timeout / 1000.0;
   

   I think this timeout needs to be converted to microseconds so that'd be $end = $start + ($timeout * 1000) no?

PhantomJSDriver has an API for visible:

public function isVisible($xpath) {

Can we test if it works on CSS based anime or not. (It's useful for OutsideIn and Contrib modules).

For example, the Codeception engine, it uses webdriver. their `isVisible` function checking CSS anime also.

Example code:
(diff engine but background theory are close)

CSS based anime. Use Bootstrap modal.

FAIL:

        $I->waitForJS("return jQuery(':animated').length === 0", $this->timeout); // checking
        $I->waitForElement('#modal--bootstrap', $this->timeout);
        $I->waitForJS("return jQuery(':animated').length === 0", $this->timeout); // double checking
        // assertions

SUCCESS:

        $I->waitForElementVisible('#modal--bootstrap', $this->timeout);
        // assertions

Re #59.3 - microtime(TRUE) return a float in seconds so we need to convert milliseconds to seconds so dividing by 1000 is correct. See https://secure.php.net/manual/en/function.microtime.php

Sorry for my picky. I'm a bit worried on the new shortcuts.

Take the LINK as example,

http://mink.behat.org/en/latest/guides/traversing-pages.html#traversal-m...

TraversableElement::findLink
Looks for a link with the given text, title, id or alt attribute (for images used inside links).

  /**
   * Ajax callback for the "Add link" button.
   */
  public static function addLink(array $form, FormStateInterface $form_state) {
    $form['added_link'] = [
      '#title' => 'Added link',
      '#type' => 'link',
      '#url' => Url::fromRoute('js_webassert_test.js_webassert_test_form')
    ];
    $form['added_link_wrong'] = [
      '#title' => 'Added link 2',
      '#attributes' => [
        'alt' => ['Added link'],
      ],
      '#type' => 'link',
      '#url' => Url::fromRoute('js_webassert_test.js_webassert_test_form')
    ];
    return $form;
  }

What if we see code like this? We looking for "added_link" but it could be matching "added_link_wrong" (alt) in our testing.

Shouldn't we make it more specified for Drupal?

TraversableElement::findField
Looks for a field (input, textarea or select) with the given label, placeholder, id or name attribute.

Another example, It always has unique ID or NAME from Drupal's FORM API.

-------

Attached a patch to update the waitFor-* functions. Less custom code :)

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -43,15 +30,14 @@ public function __construct(Session $session, $base_url = '') {
+          return instance.ajaxing === false;
...
 JS;

Sorry, I made a mistake in above patch. Hoping our test for test able to catch the error.

it should be TRUE

function isNotAjaxing(element, index, array) {
  return element === true;
}

console.log(![true, false].some(isNotAjaxing)); // false
console.log(![false, false].some(isNotAjaxing)); // true
console.log(![false, true].some(isNotAjaxing)); // false

Use Array.some() to aviod test every instances.

Thanks.

Ahh right. Some instances are NULL.

I will fix the patch later. It returns wrong $element.

--------

Above error hinted me that the test is not so right:

    $test_button->click();
    $result = $page->findButton('Added button');
    $this->assertEmpty($result);
    $result = $assert_session->waitForButton('Added button');
    $this->assertNotEmpty($result); // it verified waitForButton returns only. Buggy custom function still able to return not empty result.

Fix:

    $test_button->click();
    $result = $page->findButton('Added button');
    $this->assertEmpty($result);
    $result = $assert_session->waitForButton('Added button');

// verify waitForButton returns. Strictly, we should test it if returns "@return \Behat\Mink\Element\NodeElement|null"
    $this->assertNotEmpty($result);

// re-verify element on page using Behat Mink API
    $this->assertNotEmpty($page->findButton('Added button')); 

--------

I ran into a new error in my local test:

    $result = $page->findField('added_field');
    $this->assertEmpty($result); // failed at this line

Looks like the PhantomJS on my local env rendered ajax faster than assertEmpty

@michielnugter,

Hope so!! Maybe too slow on my Windows machine also :(

findField spent more time to match the result than others. I change it to `find('css', '...')`, then works!

On my testing, it required an extra 600ms delay anyway.

I think we can add 1 sec delay on `waitForElement` test. Other watiFor-* inherited from `watiForElement` can be tested loosely.

And on the tests:

    $result = $page->findField('added_field'); // move to here. Ensure nothing before testing.
    $this->assertEmpty($result);
    
    $test_field->click();

    $result = $assert_session->waitForField('added_field');
    $this->assertNotEmpty($result);

thoughts?

-------

extra, each waitFor-* must add a 2nd parameter to accept $timeout. Ideally, expose $timeout to a global var also

Thanks. Good to go. Just notice one thing on my above change:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -26,21 +28,149 @@ class JSWebAssert extends WebAssert {
+    $result = $page->waitFor($timeout, function() use ($page, $selector, $locator) {

$timeout / 1000

diff time scale

It's good I think but the last patch missing #78

Here's a fix for #78 plus a couple of coding standards fixes.

Yay! This is ready. We just need a change record and quick IS update.

Created CR https://www.drupal.org/node/2846936

Updated issue summary.

All my fixes on this issue have been nits and coding standards related. Therefore I think it is okay for me to commit this massive improvement to JavascriptTestBase. Also as it applies to 8.2.x I think we should commit this there too.

Committed and pushed 3af641f to 8.3.x and ca1117f to 8.2.x. Thanks!

Where's the documentation? Maybe we could post the link here also. It's handy for someone looking on this issue to make further changes or something. Thank You.