Problem/Motivation

The tests under the spotlight here:

/Ajax/AjaxFormCacheTest.php
./Ajax/AjaxFormPageCacheTest.php ( see #2809519: Convert AJAX part of \Drupal\system\Tests\Ajax\AjaxFormPageCacheTest to JavascriptTestBase )
./Ajax/AjaxInGroupTest.php
./Ajax/AjaxTestBase.php
./Ajax/CommandsTest.php
./Ajax/DialogTest.php ( see #2874640: Convert DialogTest to a FunctionalJavascript test )
./Ajax/ElementValidationTest.php
./Ajax/FormValuesTest.php
./Ajax/FrameworkTest.php
./Ajax/MultiFormTest.php

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

dawehner created an issue. See original summary.

martin107’s picture

Assigned: Unassigned » martin107
Issue summary: View changes

added test to work on in the issue summary... patch coming..

dawehner’s picture

Thanks a ton for filing this issue!

martin107’s picture

Status: Active » Needs review
FileSize
6.7 KB

First cut, expected to fail.

Just moved files and adjusted namespaces.

There are many calls to drupalPostAjaxForm() which are going to need adjustment.

Status: Needs review » Needs work

The last submitted patch, 4: early-2862510-4.patch, failed testing.

martin107’s picture

I am working on this in small steps, as it fits my pattern of spare time.

In this iteration, AjaxFormCacheTest.php now passes..

So I have a chain of thought that effects some system architecture level decisions, depending on how we proceed....so comments please.

referring to https://www.drupal.org/docs/8/phpunit/phpunit-javascript-testing-tutorial

As we move away from Drupal\simpletest\AssertContentTrait methods like assertOptionWithDrupalSelector() can be simply done with a few lines of code - using lines of code :-

$element = $page->find('css', 'css selector');
$this->assertNotEmpty($field);

assertOptionSelectedWithDrupalSelector() however has just enough complexity for us to create some reusable trait..... For the moment I am NOT going to do that
I just want to see how the rest of the issue plays out....

here is the replacement code - to a single call to $this->assertOptionSelectedWithDrupalSelector()

    // Confirm option1 has been selected.
    $page = $session->getPage();
    $opt1_selector = $page->find('xpath', '//select[@data-drupal-selector="edit-test1"]//option[@value="option1"]');
    $this->assertNotEmpty($opt1_selector);
    $this->assertTrue($opt1_selector->isSelected());
martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: AjaxFormCacheTest-2862510-6.patch, failed testing.

martin107’s picture

I would like to discuss what is the intent of the conversion.

At what level we should be testing - integration or unit testing?

There is a repeating pattern in CommandsTest:testAjaxCommands() which I will express in pseudo-code

    // Tests the 'add_css' command.
    $commands = $this->drupalPostAjaxForm($form_path, $edit, ['op' => t("AJAX 'add_css' command")]);
    $expected = new AddCssCommand('my/file.css');
    $this->assertCommand($commands, $expected->render(), "'add_css' AJAX command issued with correct data.");

1) Simulate the user updating of a SELECT html element, which triggers a AJAX response/response exchange.
2) Compute the expected output AJAX command.
3) Verify the response in (1) is the expected AJAX command.

Mink is very good at simulating users pointing and clicking in a browser and analysing how the page has changed.
BUT this test is not how the browser is updated, but what response drupal generates.

Should I modify tests of this pattern to drop inspection of the response and just assert how the browser should look after update?

If we want to confirm the response then units tests are a faster test.

I am in two minds at this moment.

martin107’s picture

Assigned: martin107 » Unassigned
Issue summary: View changes

Changing the issue summary to note that the conversion of AjaxFormPageCacheTest
is being discussed in another issue.

I think while I consider the arguments from #3 .. it would be a good thing for me to work on that

michielnugter’s picture

Issue tags: +phpunit initiative

Thanks a lot for the progress on the issue!

The JavascriptTestBase test should not check if the Ajax command is added in the correct way as the simpletest did but actually test the result of the action, we have to check the DOM.

As you demonstrated in another issue (#2809519: Convert AJAX part of \Drupal\system\Tests\Ajax\AjaxFormPageCacheTest to JavascriptTestBase) the assertion that an ajax command is created as expected can be a UnitTest.

So I think these assertions should be converted to both Unit tests and Javascript tests to cover it completely.

martin107’s picture

So I think these assertions should be converted to both Unit tests and Javascript tests to cover it completely.

Thanks for all the help, while I work all this out in my head, I am slowly getting clarity,

that is a good way to think about it.

Looking at CommandsTest -- I think that should be a side issue... just to keep this issue reviewable.

martin107’s picture

Assigned: Unassigned » martin107

If you follow the conversation here

https://www.drupal.org/node/2809519#comment-12054242

My attempts to convert AjaxFormCacheTests need to be rewritten without assertWaitOnAjaxRequest

michielnugter++

michielnugter’s picture

@martin2017

No problem! It's great to see someone else working on the JavascriptTestBase tests :)

I am going to expand the article in the documentation later on to have some more pointers on how to write these tests, you really need to get used to dealing with Javascript actions in a test..

In almost all cases the assertWaitOnAjaxRequest method is not required, it just takes a little more time to think about how you're going to test the end result :)

martin107’s picture

Status: Needs work » Postponed
FileSize
18.07 KB
15.31 KB

I am postponing this issue for a short time ... I hope

Over in

https://www.drupal.org/node/2809519#comment-12056056

we are discussing a pattern for avoiding the troublesome assertWaitOnAjaxRequest()

That has thrown a spanner in the works here as I was using the contentious pattern in the next iteration of the patch

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

-- what I have is full of mistakes and partially converted tests.... it is not worth reviewing BUT for one point.

I am using the waitForElement with :contains almost exclusively

So I am postponing on standardization of the way forward.

michielnugter’s picture

Status: Postponed » Needs work
martin107’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
19.72 KB

I am still playing with best practise patterns for testing what we want...

I have worked on AjaxInGroupTest.php I am still not particularly happy with it.

I still want a new method waitUntilTheSequenceOfAJAXCommandsInTheAJAXResponseObjectHasCompleted_I_Pinky_Swear-Honest!() -- did I say I am bad at method names...otherwise lots of alterations here are going to "hand crafted"/brittle/"hard to maintain".

In this case the inserted HTML is identical to what is already on the page....except for the build_id

A second issue I can see .... FormValuesTest the code below is simulating an attacker posting invalid AJAX requests
That is a valid thing to do ... but I am going to have to establish a new pattern to do this...I think.

    foreach (['null', 'empty', 'nonexistent'] as $key) {
      $element_name = 'select_' . $key . '_callback';
      $edit = [
        $element_name => 'red',
      ];
      $commands = $this->drupalPostAjaxForm('ajax_forms_test_get_form', $edit, $element_name);
      $this->assertResponse(500);
    }

Anyway my time is limited until the weekend....

Status: Needs review » Needs work

The last submitted patch, 17: brittle-2862510-17.patch, failed testing.

michielnugter’s picture

In this case the inserted HTML is identical to what is already on the page....except for the build_id

There should always be something checkable after an ajax response. Either by a change in the database or a change in the DOM. In this case it's only the build_id, a bit curious but hey, it's something :) We can still test this without the ajax request wait.

1. Get the build_id before triggering the ajax request
2. Trigger the ajax request
3. Use waitFor() to wait untill the build_id has changed.
4. Assert the build_id has changed.

A second issue I can see .... FormValuesTest the code below is simulating an attacker posting invalid AJAX requests
That is a valid thing to do ... but I am going to have to establish a new pattern to do this...I think.

Well, in this case I doubt the test should be in a JavaScriptTestBase. You cannot (or should not be able to) trigger an invalid request using the Drupal interface alone. Validation the status codes is on track to be removed for JavascriptTestBase by the way, see #2827014: Throw an exception when testing non response body in javascript tests.

This test should therefore remain in BrowserTestBase and the call to drupalPostAjaxForm must be converted to something BrowserTestBase-y.

martin107’s picture

I have been working on converting DialogTest it is not a straight conversion

so I am splitting it off.

I want to add some test - remove some redundant bits.

martin107’s picture

From #19

This test should therefore remain in BrowserTestBase and the call to drupalPostAjaxForm must be converted to something BrowserTestBase-y.

Here is what I can see ....

At first look, I don't think we are going to acheive this using only methods in the relatively new

Drupal\Tests\BrowserTestBase

it seems we are going to have to do something creative/non-standard

We can't use

BrowserTestBase::drupalPostForm()

as code passed through

BrowserTestBase::submitForm()

where there is an assertion that the field must be valid/exists

    // Edit the form values.
    foreach ($edit as $name => $value) {
      $field = $assert_session->fieldExists($name, $form);   /// <--- this assertion 

      // Provide support for the values '1' and '0' for checkboxes instead of
      // TRUE and FALSE.
      // @todo Get rid of supporting 1/0 by converting all tests cases using
      // this to boolean values.
      $field_type = $field->getAttribute('type');
      if ($field_type === 'checkbox') {
        $value = (bool) $value;
      }

      $field->setValue($value);
    }

At the moment it looks like the best way is to execute some javascript in a FunctionJavascriptTest
executing a $.ajax command where we have complete control of the request object...

I want to find a better way.....

michielnugter’s picture

Just to let you know, I've read and tried to fully understand your reply but haven't figured out a useful reply yet. I hope to be able to make some time this weekend to figure it out.

martin107’s picture

Another idea occurs to me

What we have in core at the moment..

1) sends requests into drupal
2) checks the output of the server

So no javascript tests in what would be the client.

When good tests pass, it serves as an integration test ... in the sense that all sub-tasks must be in the chain for the DOM to update properly

I am thinking maybe we can get the extra coverage provided by the troublesome tests by adding assertions to a form like unit test

Anyway I will have more time over the weekend.

michielnugter’s picture

It's an interesting problem and I have been trying various things but haven't found a working solution yet, time for today is up though.

At first look, I don't think we are going to acheive this using only methods in the relatively new

Drupal\Tests\BrowserTestBase

it seems we are going to have to do something creative/non-standard

It does seem like it for now, I can't see a solution using the available methods.

At the moment it looks like the best way is to execute some javascript in a FunctionJavascriptTest
executing a $.ajax command where we have complete control of the request object...

I'd love to see using JavaScript to control the test as a last resort for now.

Options I see:
- Skip browser testing for the invalid callbacks and cover these in KernelTests
- Figure out a good way to make the Ajaxy calls in BrowserTestBase.
-- A good starting point would be $this->getSession()->visit().

martin107’s picture

I'd love to see using JavaScript to control the test as a last resort for now.

On reflection using $.ajax is so wrong .... well I could make it worse, but I would have to find a way of fitting in a call to eval()!

so wrong I am sorry I suggested it ...

What I have now is just a little nudge to get the test passing a few more minor steps

FormValuesTest::testSimpleAjaxFormValue()

now that we are driving things through mink we need to adjust an existing bit of code.

red is the default option selected - selecting

"red->green->blue"

in a loop fails because red is already selected so the first AJAX request response cycle fails. going

"green->blue->red"

fixes an issue.

martin107’s picture

Status: Needs work » Needs review

The last submitted patch, 15: do-not-review-2862510-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: cycle-2862510-25.patch, failed testing.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Postponed

After a little nudge from dawehner

https://www.drupal.org/node/2862508#comment-12096171

I would like to see how we can solve the thorny issue using nightwatch ...

I will try and solve the issue as part of

https://www.drupal.org/node/2869825