Convert AJAX part of \Drupal\system\Tests\Ajax\AjaxFormPageCacheTest to JavascriptTestBase

I am making a start on this

what I have is mostly complete... just a little bug I need to resolve.. but in the spirit of post early post often... the basic structure and general solution is in the place.

1) I have converted AjaxFormPageCacheTest into something that extends JavascriptTestBase and moved it into the Drupal\FunctionalJavascriptTests\Ajax namespace.

Previously we asserted what the text of the AJAX response should be - I have cut those out and just the assertions about how the DOM has been updated remain in the test.

2) The place to make assertions about the text of the AJAX response in now covered in a new unit test - see AjaxCommandsTest::testUpdateBuildIdCommand()

Ah so to highlight a difference between classes that extend

Drupal\simpletest\WebTestBase

and things that extend

Drupal\FunctionalJavascriptTests\JavascriptTestBase

When using drupalGet() - it is not equivalent.

the new call clears the drupal cache with a call to refreshVariables()

Hence the failing test

Here is the fix.

Thanks for the good start on this issue and nice find on the drupalGet()!

Some review comments:

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -26,7 +35,7 @@ protected function setUp() {
        $this->assertEqual(count($build_id_fields), 1, 'One form build id field on the page');
   
   @@ -37,66 +46,50 @@ public function testSimpleAJAXFormValue() {
        $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', 'Page was not cached.');
   ...
   +    $this->assertNotEqual($build_id_initial, $build_id_first_ajax, 'Build id is changed in the form_build_id element on first AJAX submission');
   ...
        $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', 'Page was cached.');
   ...
        $this->assertEqual($build_id_initial, $build_id_from_cache_initial, 'Build id is the same as on the first request');
   ...
        $this->assertNotEqual($build_id_from_cache_initial, $build_id_from_cache_first_ajax, 'Build id is changed in the simpletest-DOM on first AJAX submission');
        $this->assertNotEqual($build_id_first_ajax, $build_id_from_cache_first_ajax, 'Build id from first user is not reused');
   ...
        $this->assertNotEqual($build_id_from_cache_first_ajax, $build_id_from_cache_second_ajax, 'Build id changes on subsequent AJAX submissions');
   

   Let's not use deprecated methods in the JavascriptTestBase tests. The conversions are extensive and more or less complete rewrites most of the times anyway.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -26,7 +35,7 @@ protected function setUp() {
   +    return (string) $build_id_fields[0]->getValue();
   

   You don't need the string cast anymore.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -37,66 +46,50 @@ public function testSimpleAJAXFormValue() {
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   ...
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   ...
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   ...
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   ...
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   

   I think we can wait for something to happen in the DOM to assert the change has been made? We should always try to avoid this method if possible.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -37,66 +46,50 @@ public function testSimpleAJAXFormValue() {
   +    $session->reload();
   

   Nice find! Good to keep this one in mind.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -37,66 +46,50 @@ public function testSimpleAJAXFormValue() {
   -    $edit = ['drivertext' => t('some dumb text')];
   -    $this->drupalPostAjaxForm('ajax_validation_test', $edit, 'drivertext');
   +    $this->drupalGet('ajax_validation_test');
   +    $this->getSession()->getPage()->fillField('drivertext', 'some dumb test');
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   

   It states that it is tested that a AJAX request is done but this test actually doesn't test it. The assert part in assertWaitOnAjaxRequest is misleading, it doesn't assert anything. For the test to be valid we need to check something has changed in the DOM.

6. +++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php
   @@ -429,4 +430,20 @@ public function testRedirectCommand() {
   +    $old = 'ThisStringisOld';
   +    $new = 'ThisStringIsNew';
   +    $command = new UpdateBuildIdCommand($old, $new);
   +    $expected = [
   +      'command' => 'update_build_id',
   +      'old' => $old,
   +      'new' => $new,
   +    ];
   +
   +    $this->assertEquals($expected, $command->render());
   

   Awesome!

Thanks for the expert review and the encouragement - it is much appreciated.

#8.1 fixed - In many places assertEqual becomes assertEquals etc

#8.2 fixed

#8.3

I think we can wait for something to happen in the DOM to assert the change has been made?

I need to ask a question

Programmatically we drive mink into changing the pull down box - There is a AJAX request/response exchange and a DOM element updated.

That means a $this->waitForElement() call will not work because the waitForXYZ method calls poll the page until a new element appears -- in this case we are updating an existing DOM element ... and the new id is not predictable..

I would like get rid of the assertWaitOnAjaxRequest() but I must be missing something?

#8.5 Fixed .... the new element is predictable so the waitFor is simple

#9.1 Fixed.. thanks when I rescan that section of the code and "Question all the things" I see "node" is not a required module.

#9.2 I have updated the existing comments the emphasis that we are pressing the reload button on our virtual browser.

Changes look good!

On #8.3

There are multiple issues with assertWaitOnAjaxRequest which make it prone to random failures in the tests. Some examples:
- Events are debounced, the ajax request starts ~300ms later than expected.
- Postprocessing is done separate from the ajax callback handler.
- Some browser event is interfering with the DOM (e.g. resize event).

When asserting the change in the DOM you work around all these problems as the test waits untill the expected change has happened.

There should always be something different in the DOM we can check on.

For example:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
@@ -0,0 +1,100 @@
+    $session->getPage()->selectFieldOption('select', 'green');
+    $this->assertSession()->assertWaitOnAjaxRequest();
+
+    $build_id_first_ajax = $this->getFormBuildId();
+    $this->assertNotEquals($build_id_initial, $build_id_first_ajax, 'Build id is changed in the form_build_id element on first AJAX submission');

This could be solved with the following:

$page = $this->getSession()->getPage();
$page->waitFor(10, function() use ($build_id_initial) {
  $build_id_first_ajax = $this->getFormBuildId();
  return $build_id_initial != $build_id_first_ajax;
});
$this->assertNotEquals($build_id_initial, $this->getFormBuildId(), 'Build id is changed in the form_build_id element on first AJAX submission');

Although in this case I doubt the performed check. I'd expect a change somewhere in the DOM and not only the form beeing rebuild. I would check the actual change to the form in the waitFor() method. Just make sure no exception is uncaught in the waitFor callback and add an assertion after the wait to really assert the expected state (waitFor is not an assertion and will therefore not make the test fail if it never passes.

On the used timeout (this one is in seconds): please always use 10 seconds or more if you have a good reason for it but never less. Up till now it's been my experience that this is sufficient to never cause random failures.

Thanks for taking my thinking apart - rather than just simply moving the issue forward...

I'd expect a change somewhere in the DOM and not only the form being rebuild

With hindsight the solution looks trivial/natural

Sure! It's awesome to see others working on these tests :) I've really been working on finding the best way of handling the javascript issues and am very glad that I can share them!

So nice to see it's actually rather easy to find an alternative!

Review comments:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
@@ -44,17 +44,32 @@ public function testSimpleAJAXFormValue() {
+    $green_div = $this->assertSession()->waitForElement('css', "#ajax_selected_color div:contains('green')");
+    $this->assertNotNull($green_div, 'DOM update: The selected color DIV is green.');

Didn't even think of it in this way :) But unfortunately the :contains selector is a jQuery specific selector. Earlier it was stated by @droplet (can't remember where though) that we shouldn't use jQuery only selectors to make sure we're not dependent on jQuery in our tests.

I think therefore we have to check this manually (a waitFor with callback that checks the value).

Shame that there's no equivalent in css though, it's such a nice way of doing it..

This is really valueable information we should document somewhere.

I'm going to update the documentation on the Javascript tests soon if I have my head wrapped around all these points completely.

I wonder whether this actually also speeds up things a bit when we use just native APIs of the browser.

As the alternative implementation is a bit more bulky I don't think there will be an overall performance gain. If there would be one I don't think it will be measurable though.

From #13

I think therefore we have to check this manually (a waitFor with callback that checks the value).

I am not ready to give up yet!

waitForElement('css', "option:contains('red')");

When I watch the phantomjs debug output I see the command is auto converted into the equivalent xpath selector.

it is the ugly child of the css version but this is the equivalent

waitForElement('css', "option[contains(string(.), 'red'))";

I have chased this down through a few layer of function calls and have identified the place where it happens

Behat\Mink\Element::findAll() - has a call to selectorToXpath()

IF we can say the testbot will always rely on the xpath equivalent -- Are we safe to proceed as is?

The fallback is to convert to xpath... I hope.

"Needs work" As I still have to correct the 80 chars thing from #14

@martin2017:

Thanks for the extensive research and reply!

I've been looking into this question this morning quite a lot and to me the usage of :contains in a Mink css selector is not a problem and fine by me.

My reasoning for this:
- Yes it's a custom css selector introduced by jQuery but it is fully supported by the Symfony CssSelector component. It does not rely on any additional component.
- It is currently used on several of the old Simpletests. We'll be converting these tests ofcourse but I don't see any reason for changing the css selectors there.
- The :contains is better readable and understandable.

The only con I could think of was that we introduce a non-standard css selector in our tests. Though I don't think you'll need to look far to discover another non-standard css selector in core..

I'm not a huge fan of converting to xpath just for the sake of xpath and a standard selector, these selectors are generally hard to read..

If the comment issues are fixed then I think it's good to go! Thanks very much for all the effort and sticking with it!

Thanks ... that is great news.

I don't think we will kill too many kitten with the css thing....

I have adjust the comments to fit inside the 80 chars limit.

Thanks a lot and it's looking good!

1. This change looks really good

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -0,0 +1,128 @@
   +    //
   

   Empty comment lines like this are against coding standards. I think the comment on the next line is just a continuation from the comment on the previous line.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -0,0 +1,128 @@
   +    // The callback on the form responds with three AJAX command :-
   +    // UpdateBuildIdCommand
   +    // HtmlCommand
   +    // DataCommand
   

   This is not formatted the way a list should be. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -0,0 +1,128 @@
   +    // Wait for the DOM to update.
   +    // The HtmlCommand will update #ajax_selected_color to reflect the color
   +    // change.
   

   These comments should flow together.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
   @@ -0,0 +1,128 @@
   +    // Changing the value of a select input element, triggers a AJAX request/response.
   

   Over 80 chars.

Sorry for missing those.

Last one that needs a fix:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
@@ -46,17 +46,16 @@
-    //
+

Empty line still needs to be dropped.

All the comments from #21 and #24 have been address.... all my mistakes... thank you for such a rapid response.

@alexptott thanks for #21.3 that is good for me to know.

Committed and pushed 3b22e70 to 8.4.x and b636bb7 to 8.3.x. Thanks!

Committed to 8.3.x as this is a test-only change.

Everyone received credit because every contributor on the issue provided a patch or a helpful review.

diff --git a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
index 5bd7c28..3d174b0 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxFormPageCacheTest.php
@@ -45,8 +45,8 @@ public function testSimpleAJAXFormValue() {
     $build_id_initial = $this->getFormBuildId();
 
     // Changing the value of a select input element, triggers a AJAX
-    // request/response.
-    // The callback on the form responds with three AJAX command :-
+    // request/response. The callback on the form responds with three AJAX
+    // commands:
     // - UpdateBuildIdCommand
     // - HtmlCommand
     // - DataCommand

Small fix on commit.