[backport] Add JavaScript test coverage for adding an exposed filter in Views UI

I looked into it and found 1 actual Javascript bug:

TypeError: event.data is undefined[Meer info] jquery.min.js:43:7
	resetSize http://drupal.dev/core/assets/vendor/jquery/jquery.min.js:43:7
	Drupal.debounce/</later http://drupal.dev/core/misc/debounce.js:41:18

Which is an error triggered in:
core/misc/dialog/dialog.position.js - line 59.

Reproducable with the following steps:
1. Open add filter
2. Check body (body) filter
3. Click Add and configure button
4. Resize your window

I think that this JavaScript error is the cause for the test failure. The problem occurs when clicking the Apply button. This closes the dialog and causes the error. I have seen the error in the test if you change the pressButton('Apply') to:

$this->assertEquals('', $page->getHTML());

Which should work just fine but somehow triggers the Javascript error.

... update:

There is a bug open for the dialog error: #2731419: "cannot call methods on dialog prior to initialization" logged when resizing after closing a modal

When applied I can extract the HTML but the test still failes. Will search some more tomorrow..

... update 2:

I found the actual (javascript) bug, not sure yet on how to fix it.

A new dialog is opened after adding the body field but the previous one isn't closed. This causes the dialog:beforeclose event not to be fired and the window not unbinding the 'dialogResize' event. On the second dialog 2 events are attached to the window, the first causing errors..

update:
Dug a little deeper. The dialogs are opened via commands. However, no close command is send.

To fix this either one of the following must be done:
1. In views, send a closeDialog command before the open command to properly close it before opening it.
2. Adapt the dialog open to check if there already is a dialog with the passed selector open and close it first
3. Change the dialog.position javascript to only add the event once per dialog selector.

Option 1 seems the right way to go but I'm not sure yet on UX, wil the dialog quickly open/close and 'flicker'?
Options 2 and 3 seem quite large in scope and will dig deep into the core javascript, they will provide somewhat better DX though and possibly UX as you don't have to worry about opening dialogs that currently exist and no flickering will be visible..

VERY dirty fix for the popup in this case just to see if it fixes the problems. They don't unfortunately :(

What we learned:
- Yes, there is a small flicker sometimes, more that without the close. The dialog seems to align from the bottom of my screan to the center. Doesn't happen each time but it seems far more frequent than without the close.
- Yes there is a bug: as it's not related to this issue I think we should open a new one and discuss the solution there as I'm not sure requiring a closeDialog is DX / UX friendly.
- Despite the bugs the test still failes with PhantomJS responding the the following 500 error:

REQUEST: {"name":"click","args":[4,261]}

2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript result QVariant(QString, "object")
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript "(function() { return (function() { return PoltergeistAgent.stringify((function (name, args) {\n      return window.__poltergeist.externalCall(name, args);\n    }).apply(this, JSON.parse(\"[\\\"nodeCall\\\",[261,\\\"scrollIntoView\\\",[]]]\"))) })(); })()"
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript result QVariant(QString, "{}")
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript "(function() { return (function () {\n      return typeof window.__poltergeist;\n    })(); })()"
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript result QVariant(QString, "object")
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript "(function() { return (function() { return PoltergeistAgent.stringify((function (name, args) {\n      return window.__poltergeist.externalCall(name, args);\n    }).apply(this, JSON.parse(\"[\\\"nodeCall\\\",[261,\\\"position\\\",[]]]\"))) })(); })()"
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript result QVariant(QString, "{\"value\":{\"top\":680,\"right\":129,\"left\":128,\"bottom\":681,\"width\":1,\"height\":1}}")
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript "(function() { return (function () {\n      return typeof window.__poltergeist;\n    })(); })()"
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript result QVariant(QString, "object")
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript "(function() { return (function() { return PoltergeistAgent.stringify((function (name, args) {\n      return window.__poltergeist.externalCall(name, args);\n    }).apply(this, JSON.parse(\"[\\\"nodeCall\\\",[261,\\\"mouseEventTest\\\",[128.5,680.5]]]\"))) })(); })()"
2017-01-09T08:43:47 [DEBUG] WebPage - evaluateJavaScript result QVariant(QString, "{\"value\":{\"status\":\"failure\",\"selector\":\"html.touchevents.details.js body.user-logged-in.path-admin div.ui-dialog.ui-widget.ui-widget-content.ui-corner-all.ui-front.ui-dialog-buttons.views-ui-dialog-scroll.views-ui-dialog.js-views-ui-dialog div#drupal-modal.ui-front.ui-dialog-content.ui-widget-content div form#views-ui-config-item-form--Tou9urHIYX4 div#edit-options--Nk8ipRM2SSw.scroll.js-form-wrapper.form-wrapper\"}}")
2017-01-09T08:43:47 [DEBUG] HTTP Response - Status Code 500 Internal Server Error
2017-01-09T08:43:47 [DEBUG] HTTP Response - Sending Header "Content-Type" = "application/json"

There is a warning on the preview in the logs but the preview is disabled in the test:

Warning: Illegal string offset '#markup' in views_ui_preprocess_views_view() (line 137 of /Library/WebServer/Documents/drupal/core/modules/views_ui/views_ui.module)

I couldn't find any other warning in my server logs.

I'm completely stuck here on finding the problem.

I did a small cleanup of the test by the way but I think the final version should be postponed untill #2837676: Provide a better way to validate all javascript activity is completed is in.

Test looks good and this unblocks #2839440: Exposed filter criteria gives InvalidArgumentException: The #default_value property has to be an entity object or an array of entity objects. in Drupal\Core\Entity\Element\EntityAutocomplete::valueCallback() where we need a test for a bug fix.

The fail on D9 is:

Views.Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler\FilterTest
✗	
Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler\FilterTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-142.xml:
PHPunit Test failed to complete; Error: PHPUnit 8.5.3 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler\FilterTest
.                                                                   1 / 1 (100%)

Time: 18.06 seconds, Memory: 4.00 MB

OK (1 test, 8 assertions)

Remaining self deprecation notices (1)

  1x: Declaring ::setUp without a void return typehint in Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler\FilterTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
    1x in DrupalListener::startTest from Drupal\Tests\Listeners

Here I have tried to fix failed tests for Drupal 9.1.x.

Thanks for the patch!

+++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
@@ -0,0 +1,169 @@
+  protected function setUp() : void {

There should be no space between () and :, i.e.

protected function setUp(): void {

Here I have addressed comment #25.

Looks good.

Mostly looks good, thanks! A few concerns / nits:

1. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +   * Test adding a filter handler.
   

   s/Test/Tests/

2. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +    $session = $this->getSession();
   

   We don't seem to use this variable anywhere, let's remove it.

3. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +    $web_assert->assertWaitOnAjaxRequest();
   ...
   +    $web_assert->assertWaitOnAjaxRequest();
   ...
   +    $web_assert->assertWaitOnAjaxRequest();
   ...
   +    $web_assert->assertWaitOnAjaxRequest();
   

   According to @Lendude at #2842525-80: Ajax attached to Views exposed filter form does not trigger callbacks we should avoid assertWaitOnAjaxRequest() whenever possible.

4. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +   * @return Bool|null
   +   *   TRUE is the required number was matched, NULL if not.
   ...
   +      return NULL;
   

   Why NULL on failure? Why not just @return bool and use FALSE if we time out before we get the right count?

5. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +    $result = $page->waitFor($timeout / 1000, function () use ($count, $page, $locator) {
   ...
   +    return $result;
   

   Why bother with a local $result variable? Why not just:

       return $page->waitFor(...)
   

6. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +   * @return Bool|null
   +   *   TRUE is the required number was matched, NULL if not.
   

   Same here. Why not FALSE on failure?

7. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,169 @@
   +    $result = $page->waitFor($timeout / 1000, function () use ($page) {
   ...
   +    return $result;
   

   Same here... why have $result at all?

addressed comment #28
please review the patch.

@Lendude Thanks for #30

re #29 / #30 Sorry I wasn't clear. I meant to paste the relevant quote, but forgot:

#78.2 is because assertWaitForAjaxRequest is notoriously fickle, it has been the cause of many a random fail on Drupal CI. It has been shown that actually waiting for the change to appear is much more stable. So unless there is no change to detect, try never to use assertWaitForAjaxRequest. Since this is fairly straight forward I would not expect problems here, but best to be safe.

There are plenty of examples in #2829040: [meta] Known intermittent, random, and environment-specific test failures that in the fixes come down to 'replace assertWaitForAjaxRequest with other wait options'.

So yeah, thanks for #32!

All the changes since my review now look great, and all feedback addressed. I tried this manually on the 9.1.x branch, and it applies and passes there, too (no sense wasting 1.25 hours of bot cycles to tell us that).

RTBC!

Actually, if we're targeting 8.y.x, we need another version of this without the :void typehint in setUp(), or we break stuff on PHP 7.0. Anyone else want to re-roll so I can still re-RTBC?

#32 is still RTBC for 9.y.x branches, but we should probably have the backports ready before this is committed.

Thanks/sorry,
-Derek

Oh, FFS, this is ridiculous. I'm just gonna fix it and self-RTBC. I think I can be trusted to do a 5 byte fix myself. ;)

Great, so the bot results agree with me. ;) #32 fails on PHP 7.0, but #36 passes.

Updated with a proper summary, including links to which patch is RTBC for which branch.

Enjoy,
-Derek

1. There's a deprecation warning in Drupal 9.1.x which should be addressed.

2. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,163 @@
   +   *   (Optional) Timeout in milliseconds, defaults to 10000.
   ...
   +  protected function waitForOnlyContentRows($timeout = 10000) {
   ...
   +    return $page->waitFor($timeout / 1000, function () use ($page) {
   

   Do we have a pre-existing pattern to define durations in milliseconds? Just wondering because it seems potentially confusing to have different unit in comparison to Mink.

3. +++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FilterTest.php
   @@ -0,0 +1,163 @@
   +    $add_button = $page->find('css', '.ui-dialog-buttonset .button--primary');
   

   Any thoughts on using the button text instead of .button--primary for selecting which button to press? This would make it easier for us to run this test on other themes (it's more likely they'll customize the button class compared to customizing the text).

Re: #39:

1. Patch #32 is correct for 9.1.x and 9.0.x. As I tried to make clear (both in my comments and in the summary), #36 is only the D8 backport.

2. Yes, tons. See core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php and search for 'milliseconds'. Every wait*() method core already provides does it this way.

3. We only test this with Stark, so I'm not sure that it matters. I'm also slightly worried that there might be a lot of 'Add' buttons. ;) The existing seems more resilient.

Thanks,
-Derek

Setting to NW because the patch for 9.1.x, in #36, has a failing test.

Uploading #32 as 2754985-44.9_x-only.patch
Uploading #36 as 2754985-44.8_x-only.patch
Re-updating the summary to continue to point to the right patches.

Hopefully everyone will be clear on what to test / commit where.

Thanks!
-Derek

Thank you for clarifying #39.1.

Re: #39.3: Isn't the button text "Add and configure filter criteria" which would be quite specific? We also use that for the Apply button few rows below, so why we couldn't use the same approach there? Stark is one of the themes we are allowed to change and for that reason we could change that class in the future.

Sure, that works. Tested locally on 9.1.x and 8.9.x.

Committed 1f522e2 and pushed to 9.2.x. Thanks!

Leaving open for backport.

Wonderful, thanks!

Discussed with @catch and we agreed that this could be backported to 9.1.x. We also agreed to not backport this to 8.9.x because it's pretty much only open for critical issues at this point.