Problem/Motivation

While looking into the problems with the random fails in JavascriptTestBase most of the found problems are with an incorrect wait for something to happen.

Issues with waiting problems:
#2832672: [PP-1] Upgrade jcalderonzumba/* for better test performance
#2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly
#2831603: Improve assertion for testEntityReferenceAutocompleteWidget

Currently a lot of tests wait for specific JavaScript action to complete, for example ajax requests and autocomplete actions. This needs to change, the tests should be able to wait for the expected element to mimic the UX as much as possible.

Proposed resolution

Implement helper methods to wait for various types of elements, the list matches the find...() methods.

For the edge cases that are left we need to fix both the assertWaitOnAjaxRequest() and waitOnAutocomplete().

waitOnAutocomplete waits for a fixed time and then for an Ajax request. This must be changed to wait for the expected UI change.
assertWaitOnAjaxRequest needs to wait for the full Drupal controlled AJAX requests to complete, including all the follow-up actions.

Remaining tasks

  1. DONEAgree on the best approach (IS updated based on the comments)
  2. DONEImprove current API methods
  3. DONEImplement new wait..() API methods
  4. DONEWrite test that covers each type of wait?
  5. Update JTB documentation and describe the best way to handle waiting for elements

API changes

Discouraging:

  • Drupal\FunctionalJavascriptTests\JSWebAssert::assertWaitOnAjaxRequest
  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitOnAutocomplete

Adding:

  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElement
  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElementVisible
  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitForButton
  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitForLink
  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitForField
  • Drupal\FunctionalJavascriptTests\JSWebAssert::waitForId
CommentFileSizeAuthor
#82 2837676-2-82.patch18.85 KBalexpott
#82 79-82-interdiff.txt1.55 KBalexpott
#79 2837676-79.patch19.11 KBmichielnugter
#79 interdiff-77-79.txt593 bytesmichielnugter
#77 2837676-77.patch19.98 KBmichielnugter
#77 interdiff-71-77.txt7.24 KBmichielnugter
#71 2837676-71.patch18.25 KBdroplet
#71 interdiff.patch1.2 KBdroplet
#69 2837676-68.patch18.25 KBdroplet
#69 interdiff.patch1.1 KBdroplet
#66 2837676-66.patch18.18 KBdroplet
#66 interdiff.patch3.71 KBdroplet
#63 provide-javascript-activity-validation-2837676-63.patch19.24 KBmichielnugter
#63 interdiff-61-63.txt648 bytesmichielnugter
#61 provide-javascript-activity-validation-2837676-61.patch20.09 KBmichielnugter
#61 interdiff-54-61.txt7.6 KBmichielnugter
#55 provide-javascript-activity-validation-2837676-54.patch16.5 KBmichielnugter
#55 interdiff-52-54.txt1.76 KBmichielnugter
#53 provide-javascript-activity-validation-2837676-52.patch15.41 KBmichielnugter
#53 interdiff-49-52.txt4.01 KBmichielnugter
#49 provide-javascript-activity-validation-2837676-49.patch18.3 KBmichielnugter
#49 interdiff-47-49.txt491 bytesmichielnugter
#48 2837676-47.patch18.3 KBalexpott
#48 46-47-interdiff.txt498 bytesalexpott
#46 2837676-46.patch18.3 KBalexpott
#46 44-46-interdiff.txt3.64 KBalexpott
#44 provide-javascript-activity-validation-2837676-44.patch18.37 KBmichielnugter
#44 interdiff-42-44.txt14.22 KBmichielnugter
#42 2837676-42.patch5.21 KBalexpott
#42 37-42-interdiff.txt4.81 KBalexpott
#37 provide-javascript-activity-validation-2837676-37.patch4.7 KBmichielnugter
#37 interdiff-34-37.txt3.28 KBmichielnugter
#34 provide-javascript-activity-validation-2837676-34.patch1.85 KBmichielnugter
#34 interdiff-32-34.txt648 bytesmichielnugter
#32 provide-javascript-activity-validation-2837676-32.patch1.86 KBmichielnugter
#32 interdiff-24-32.txt3.86 KBmichielnugter
#24 provide-javascript-activity-validation-2837676-24.patch3.76 KBmichielnugter
#24 interdiff-20-24.txt2.21 KBmichielnugter
#20 provide-javascript-activity-validation-2837676-20.patch3.76 KBmichielnugter
#20 interdiff-17-20.txt1.93 KBmichielnugter
#17 provide-javascript-activity-validation-2837676-17.patch3.46 KBmichielnugter
#17 interdiff-15-17.txt1.72 KBmichielnugter
#15 provide-javascript-activity-validation-2837676-15.patch3.49 KBLendude
#15 interdiff-2837676-11-15.txt911 bytesLendude
#11 provide-javascript-activity-validation-2837676-11.patch3.41 KBLendude
#11 interdiff-2837676-9-11.txt1.41 KBLendude
#9 provide-javascript-activity-validation-2837676-9.patch3.46 KBmichielnugter
#9 interdiff.txt2.17 KBmichielnugter
#8 provide-javascript-activity-validation-2837676.patch3.04 KBmichielnugter
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes

First iteration, based on previous work.

This adds support for:
- jQuery ajax requests
- Animation
- Debounce (new)

TODO:
- Drupal response processing
- Ajax requests on load

michielnugter’s picture

michielnugter’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
2.17 KB
3.46 KB

And now with support for Drupal response processing, was easier than expected.

I'm very much in doubt about this point for this issue:

  • Find a way to make a page reload wait for javascript activity

This would require a lot of overwriting (see https://www.drupal.org/node/2830485#comment-11821833) and I seriously question if it should be done.

Any opinions on this would be great.

xjm’s picture

Priority: Normal » Major

Thanks @michielnugter.

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

Lendude’s picture

Status: Needs review » Needs work

The last submitted patch, 11: provide-javascript-activity-validation-2837676-11.patch, failed testing.

jibran’s picture

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

michielnugter’s picture

@lendude
Looks cleaner, the fail is related to the debounce check though I think. The autocomplete fails and this has a build in 300 ms wait for the debounce.

@jibran
Could be done, I want to leave this to the javascript maintainers in what they prefer. Both options could work, for now I'd say adding it in the test would be enough. Maybe a bigger Drupal.isBusy() containing all the checks would be better suited.

Hope to be able to look into this more later today or at least tomorrow.

Lendude’s picture

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

jibran’s picture

Issue tags: +JavaScript

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’s picture

Small cleanup to make the Drupal === 'undefined' work similar to the jQuery check. Also shortened the IsNotAjaxing method for better overall readability.

Runs fine for me as well locally. Hope the testbot can test this soon!

Lendude’s picture

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

Status: Needs review » Needs work

The last submitted patch, 17: provide-javascript-activity-validation-2837676-17.patch, failed testing.

michielnugter’s picture

Even more cleanup, thanks @lendude for your feedback! You know I love the nitpicks (I actually do!).

Also added comments for the JS code.

@lendude: I agree on the followup. Let's get this in as soon as possible to reduce the likeliness of random fails.

I also think that this item: "Ajax requests fired on load (drupalGet / form submit) to finish" should be adressed in a followup, I think it's a different problem than this issue fixes. This issue just provides us with a better way of waiting for everything javascripty and shouldn't change much more. Trying to keep the scope small so we can move forward.

Status: Needs review » Needs work

The last submitted patch, 20: provide-javascript-activity-validation-2837676-20.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review

Fail seems unrelated(?). Adding others tests as well.

Lendude’s picture

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

droplet’s picture

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

The last submitted patch, 8: provide-javascript-activity-validation-2837676.patch, failed testing.

michielnugter’s picture

Moving the debounce() changes to the test only is no problem, I don't know how yet though. The changes should be added immediately and before anything uses debounce(). Tips/example would be very much appreciated :)

My view on the debounce() check:

There are different situations where debounce() is used. It's not only used for autocomplete but in other ways as well.

Waiting for the expected result would be best but in that line of thinking we should never use any of these methods in the first place. If that's the solution I'm fine with that too but that would make writing tests a little more complex.

This method (and the original methods it was based on) would mainly serve to make writing tests easier and less error prone. Not everyone will know that a debounce() will cause a semi-random delay of about 300 ms before the expected task is performed.

My view on the anime check and shorter timeouts:

The anime check was a copy-paste form the assertWaitOnAjaxRequest(). I assumed these checks were well thought out. Also: splitting this method into different parts will be difficult, we don't know the exact order in which we should wait.

Shortening the timeout to 3000ms will cause random failures on slower computers and I don't really see the use for it. I agree though that anything not rendering within 1000 ms is a big UX issue. I'm not sure though we can catch that in a reliable way in a test. Performance is very random up till now (random failures are evidence to that).

jibran’s picture

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.

klausi’s picture

  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?

michielnugter’s picture

Interesting development, I had some time to think about it and I agree with you both that checking the UI is far better than waiting for anything you shouldn't really have to know about but it has downsides as well.

I think there are 2 ways of solving this problem:

  1. Completely discourage the usage of the wait() methods and reject any tests that contains them.
  2. Automatically add the wait() to each trigger for reload or ajax (drupalGet / button press).

Option 1 would mean:
- More complex test writing, you need to check for the availabily of the expected UI. Sometimes for the execution of behaviors (@klausi's last point)
- More precise testing, you actually check that what should happen, happens
- Complex reviews because the reviewer needs to know if the tests properly waits for the UI
- Higher risk for random fails because of forgotten waits().

Option 2 would mean:
- Easier test writing, *magic* takes care that the expected UI is ready for use.
- Easier reviewing, less deep knowledge required. You can review what you see.
- Might cause unnecessary delays although not too much I think.
- Black box behavior
- No or at least a low number of fails due to timing issues.
- Overriding various bits of the PhantomJS driver.

EDIT: @klausi: I haven't fixed your code reviews yet, let's wait with the patches untill a decision is made on the direction for this issue.

michielnugter’s picture

I thought of a third option:

- Remove the debounce() check, this should always be checked in th UI
- Don't add a new function but improve the existing methods:
-- waitOnAjax: Add the wait for Drupal.ajax isAjaxing
-- waitOnAutocomplete: Remove the 300ms delay and change the validation to a UI check.
- We could promote the use of UI checks instead of the wait() methods in code reviews (and update the documentation)

This would make the scope for this issue significantly smaller because only existing api methods will be improved and made more robust.

michielnugter’s picture

Status: Needs review » Needs work

The last submitted patch, 32: provide-javascript-activity-validation-2837676-32.patch, failed testing.

michielnugter’s picture

This should pass.

Still to be determined:
- Which direction to take, is this patch the best solution?
- If the current patch is it if the javascript should be moved to an external file. The amount is less now and a bit in line with other usages of inline javascript in JTB.

Status: Needs review » Needs work

The last submitted patch, 34: provide-javascript-activity-validation-2837676-34.patch, failed testing.

michielnugter’s picture

If we are going to rely on waiting for the UI some API methods for this should be available I think. I created a base method and a method for each specific find...() method. This way we have a similar interface for finding elements and waiting for them. As a 'bonus' I returned the requested element so the test can just call this one line.

Curious to see what you all think of these methods and option 3..

michielnugter’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: provide-javascript-activity-validation-2837676-37.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
michielnugter’s picture

Should this issue be promoted to critical? In my opinion it's blocking all JTB tests from getting committed because this will decide an essential part of these tests, waiting for various things..

alexpott’s picture

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

michielnugter’s picture

Issue summary: View changes

Nice cleanup, thanks for that and for the +1 on the direction. I updated the IS to reflect the current direction of the issue.

I assume the Needs tests should be a JTB test? Are the currently tests covering the custom JTB code?

update: Discussed with alexpott on IRC. Working on a JTB test for JSWebAssert. I'll post a patch when I have something.

michielnugter’s picture

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

Based on droplet's reply: https://www.drupal.org/node/2831603#comment-11871156 I deprecated waitOnAutocomplete(), I applied his patch from this post and merged it in this patch as I agree with him. Credits to droplet for the change on that. Might be needed to split this off again if it gets committed in another issue.

Status: Needs review » Needs work

The last submitted patch, 44: provide-javascript-activity-validation-2837676-44.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
18.3 KB

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.

michielnugter’s picture

@alexpott Thanks for the improvements, I figured as much on the test module. Hope it passes now.

Btw: once this is in I'll update the documentation on this subject to describe the usage of these methods in the waiting section.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
498 bytes
18.3 KB

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.

michielnugter’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
491 bytes
18.3 KB

Another nit.

Also: @klausi provided feedback on the changes in EntityReferenceAutocompleteWidgetTest.php

https://www.drupal.org/node/2831603#comment-11873723

I think we need to adres this feedback first?

klausi’s picture

Status: Needs review » Needs work
  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 assertion for testEntityReferenceAutocompleteWidget. 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?

Wim Leers’s picture

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!

droplet’s picture

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

michielnugter’s picture

Adressed 1, 3 and 4 on #50.

1: Removed the changes
3: It's not deprecated anymore now. I think #2831603: Improve assertion for testEntityReferenceAutocompleteWidget should handle that I think
4: I changed it to use waitForElement. Interesting thing here is that :visible is not supported by selectorToXpath(). The conversion does not explicitly test the visibility. Is this a problem?

2: Not sure yet on how to proceed here. Asked alexpott on IRC. Might post another update based on his suggestions.

Adressed #1 in #51. Other issues are now not relevant anymore as the code has been removed from the patch.

jibran’s picture

Issue tags: +Needs change record

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.

michielnugter’s picture

@droplet:

With the previous test the test passes for the autocomplete. Of course I'm not sure yet on random failures.

We could add a waitForElementVisible which waits again untill the element is visible. I attached a proposal for this, I changed waitForAutocomplete to use this variant.

michielnugter’s picture

Status: Needs work » Needs review
alexpott’s picture

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.

droplet’s picture

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

alexpott’s picture

Status: Needs review » Needs work
  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?

michielnugter’s picture

Status: Needs work » Needs review

1. True, I'll work on that.
2. True, I'll fix that.
3. It's a copy-paste from the PhantomJS/Mink driver. Didn't completely think it through yet.

Follow-ups created:
#2844580: Add JavaScriptTestBase coverage for methods missing it in JSWebAssert
#2844582: Move inline javascript in JSWebAssert into a separate javascript file

michielnugter’s picture

Adressed 1 and 2.

3: It's a conversion from milliseconds to microseconds. I think it's correct like this.

droplet’s picture

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

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

michielnugter’s picture

@droplet:

I reread your comment. Do you mean that the current isVisible() check is not good enough and also needs checking on animation? If so: I propose to open up a seperate follow-up to add this improvement to isVisible() as I think it would be out of scope for this issue.

droplet’s picture

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 :)

droplet’s picture

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

Status: Needs review » Needs work

The last submitted patch, 66: 2837676-66.patch, failed testing.

droplet’s picture

Status: Needs review » Needs work

The last submitted patch, 69: 2837676-68.patch, failed testing.

droplet’s picture

Ahh right. Some instances are NULL.

michielnugter’s picture

Was posting another patch at the same time, you won :) Hope this passes.

About #68
I think this should be correctly implemented in every test, yes it's a pitfall but I'm not sure if we should fix it on the Drupal level. The new API methods use the exact same implementations as the find...() methods. If we want to make them more specific it's a huge change because it might mean we have to update all existing JTB tests.

Status: Needs review » Needs work

The last submitted patch, 71: 2837676-71.patch, failed testing.

droplet’s picture

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’s picture

@droplet:
Wow, fast machine :) Tried the test 100x locally and never had the problem. I can understand however it might cause random failures by itself.

Does the check really make the test better or can we do without? If we need it we should add a delay in the ajax methods to force a delayed response, 200ms should be enough I think?

droplet’s picture

@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

michielnugter’s picture

I updated the test with the changed order of checking and added a specific check on the returned result.

This fixes the random fail if the click() was too fast for the find...() and the incorrect return of $element.

Also: the test failed on an asssertion in EntityReferenceAutocompleteWidgetTest which had me surprised. The autocomplete searched for Test[space] and the result was 2 hits (Test page and Page test, the search is case insensitive and apperently ignoring the space. The test however asserted that only 1 result should be available.
Without this patch the test fails as well for me, the problem is not specific to this patch it seems.

I changed the input but I'm not sure this is in scope for this issue and a proper fix.

droplet’s picture

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

michielnugter’s picture

Did some further testing on EntityReferenceAutocompleteWidgetTest.php, it keeps failing for me and I still think it's weird. It fails with and without this patch. Restarted PhantomJS in between to make sure.

Attached a patch without the change to EntityReferenceAutocompleteWidgetTest.php to see if it fails on the testbot as well. Might just be my local dev that's messed up.

Includes the fix for the timeout conversion mentioned above by @droplet

michielnugter’s picture

Right, time to check my dev environment :) Adding other tests to make sure..

With the latest fix for the timeout: is it ready for RTBC?

droplet’s picture

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

alexpott’s picture

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 3af641f on 8.3.x
    Issue #2837676 by michielnugter, droplet, alexpott, Lendude, jibran,...

  • alexpott committed ca1117f on 8.2.x
    Issue #2837676 by michielnugter, droplet, alexpott, Lendude, jibran,...
michielnugter’s picture

Thanks for the commit! When I find the time I'll look into updating the documentation on this point.

michielnugter’s picture

Documentation has been updated. Not at my best at the moment so the text might need some refinement sometime..

droplet’s picture

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.

  • alexpott committed 3af641f on 8.4.x
    Issue #2837676 by michielnugter, droplet, alexpott, Lendude, jibran,...

  • alexpott committed 3af641f on 8.4.x
    Issue #2837676 by michielnugter, droplet, alexpott, Lendude, jibran,...

Status: Fixed » Closed (fixed)

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