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