Convert system/tests/src/Ajax to JavascriptTestBase

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

First cut, expected to fail.

Just moved files and adjusted namespaces.

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

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());

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.

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

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.

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.

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

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

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.

With #2809519: Convert AJAX part of \Drupal\system\Tests\Ajax\AjaxFormPageCacheTest to JavascriptTestBase addressed I think we can unpostpone this one as well

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

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 status code or response headers in functional JavaScript tests.

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

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.

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

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.

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.

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

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.

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

I spent a little little time tonight, familiarizing myself with this issue after a break...

There is a comment and reaction of mine I need to correct

In number #11 when michielnugter says

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

The correct push-back is to say the unit test already exist see Drupal\Tests\Core\Ajax\AjaxCommandsTest

It maybe that some other parallel effort has converted things in the inbetween time.

I want to start this up again.

DialogTest is being worked on separately in another issue.

So I am cutting those changes out of this patch

DialogTest was moved out into its own issue because it was fiddly and too intricate to work on in a bulk conversion issue.

Using the same reasoning I am splitting off FrameworkTest

1) Updating the issue summary
2) Cutting out FrameworkTest from this issue.

Sorry if this looks like this issue is proceeding slowly

Before I work on ElementValidationTest further .....

Looking at the test results above ... it reports :-

RuntimeException: Unfinished AJAX requests while tearing down a test

This screw up can be seen when manually repeating the tests... ( I have listed the step to reproduce in the spin off issue )

Now that this Function Javascript is moving closer to what the manually testing represents ... it also gets into an infinite loop.

That is a bug not a conversion task ... so I have spun off a side issue so that we build up a meaningful audit trail keeping things separate.

In this update, I worked on getting MultiFormTest closer to sane.

instead of drupalPostAjaxForm() ... this patch simulates pushing of buttons.

But I have hit another brick wall -- and so am splitting off a side issue because I want to keep bugs separate from conversion tasks

As testing moved closer to what a human would press and click, when the functional tests enable the module as in

drush en form_test

both the test and humans get this error.

ReflectionException: Class Drupal\form_test\EventSubscriber\FormTestEventSubscriber does not exist

I am now going to focus on the two bug reports spun off from this issue.

MultiFormTests is now passing locally.

Like DialogTest and FrameworkTest is questionable if it should be moved out into its own patch
..I will let reviewers decide.

PS

The conversion of FrameworkTest crept back into the patch ... that is why the error count jumped up ...sorry I have stripped it out again.

I accidently included composer.json and composer.lock files

martin107--

Regarding FormValuesTest.php - back in April (#17)

    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);
    }

I was looking at ways to convert this code snippet... I missed the fact that the elements were included in the output DOIM and it would take only a minor change --- to get a elegant fix.

The fix is to supply more than red as a option ... then through clicking the UI the 500 errors can be triggered an observed.

Locally it fails as php errors are not handled correctly ... I am just posting to see how testbot deals with errors.

Just making notes for myself ...when I come back to work on this some more.

I am still thinking about a working solution for the following code snippet :-


foreach (['null', 'empty', 'nonexistent'] as $key) {
$element_name = 'select_' . $key . '_callback';
// Updating the field will trigger a AJAX request/response.
$session->getPage()->selectFieldOption($element_name, 'green');
// The select element is disabled as the AJAX request is issued.
$this->assertSession()->waitForElement('css', "select[name=\"$element_name\"]:disabled");
// The select element is enabled as the response is receieved.
$this->assertSession()->waitForElement('css', "select[name=\"$element_name\"]:enabled");
// The request will fail.
$this->assertSession()->statusCodeEquals(500);
}


A) is is not working at the moment!

BUT

B)

Given

#2827014: Throw an exception when testing status code or response headers in functional JavaScript tests

and this change record

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

calls to statusCodeEquals() are wrong.

decouple each test into a separate issue.

Ok, this issue has stalled a little bit... your suggestion is a good way of working things out.

Yeah splitting this was a good idea, some of the child issue have already landed. Lets leave this as a meta issue?

All the child issues are done! Thanks everybody.