Problem/Motivation

drupalPostAjaxForm() is simulating the behaviour of ajax.js, so using it, doesn't really provide fundamental guarantees.
#2809161: Convert Javascript/AJAX testing to use JavascriptTestBase suggests to convert them to JavascriptTestBase

Proposed resolution

  1. Figure out which part of the test is testing PHP code and which part ajax behaviour
  2. Extract the ajax behaviour into a test that extends JavascriptTestBase

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

michielnugter’s picture

Component: phpunit » system.module
Issue tags: +phpunit initiative

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

Status: Active » Postponed
Related issues: +#2862510: Convert system/tests/src/Ajax to JavascriptTestBase
FileSize
3.99 KB

I am providing most of the solution here which I am cutting out from a monster patch which is too big to review. ( as a first step I am just providing a cut out patch - I will list the work to be done at the end )

As part of spreading the monster patch in several splinter patches ... things need to be coordinated.

This patch needs AjaxTestBase, which will be provided by #2809521: Convert AJAX part of \Drupal\system\Tests\Ajax\AjaxInGroupTest to WebDriverTestBase when it lands.. in short I am postponing this iisue

As appropriate this patch replaces drupalPostAjaxForm() with a test employing ->pess() ->findAll and ->assertFieldByValue() calls.

So here is a list of things to do when this issue reopens

1) The patch contains function calls to assertWaitOnAjaxRequest() which is a bad code smell and need to be refactored.
2) We must update AjaxTestBase with a trait/method which contains the code in AjaxTestBase:: assertNoDuplicateIds() [ here is link to the original moster patch --- https://www.drupal.org/project/drupal/issues/2862510#comment-12225746 ]

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Status: Postponed » Needs work

No AjaxTestBase, no need to wait.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
4.76 KB

So something like this.

dawehner’s picture

  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MultiFormTest.php
    @@ -68,31 +71,59 @@ public function testMultiForm() {
    +    $this->assertSession()->assertWaitOnAjaxRequest();
    ...
    +    $fields = $page->findAll('xpath', $form_xpath . $field_xpath);
    

    Should we wait for this element to appear?

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MultiFormTest.php
    @@ -68,31 +71,59 @@ public function testMultiForm() {
    +        $this->fail(new FormattableMarkup('The HTML ID %id is unique.', ['%id' => $id]));
    

    Is there a reason why we don't use just strtr

borisson_’s picture

Status: Needs review » Needs work

#10.2, we don't need to use strtr or sprintf, we can just do this with string concatenation.

Lendude’s picture

Status: Needs work » Needs review
FileSize
665 bytes
4.73 KB
$fields = $page->findAll('xpath', $form_xpath . $field_xpath);
$this->assertEqual(count($fields), 2);

#10.1 nope, because we need to wait for 2 elements matching that to appear, which would be hard to do, since they match the same selector. I agree it would be better to do so, but don't really see a good way of doing it.
#10.2/#11 yeah simple concat works, it was a straight copy paste from \Drupal\KernelTests\AssertContentTrait::assertNoDuplicateIds. Also since it's a fail() the message should probably be negative, not sure, but that would make sense to me.

Status: Needs review » Needs work

The last submitted patch, 12: 2809535-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

Awesome,, thanks for those changes @Lendude!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MultiFormTest.php
@@ -68,31 +71,59 @@ public function testMultiForm() {
+  /**
+   * Asserts that each HTML ID is used for just a single element.
+   *
+   * @return void
+   */
+  protected function assertNoDuplicateIds() {
+    $status = TRUE;
+    foreach ($this->xpath('//*[@id]') as $element) {
+      $id = $element->getAttribute('id');
+      if (isset($seen_ids[$id])) {
+        $this->fail('The HTML ID ' . $id . ' is not unique.');
+        $status = FALSE;
       }
+      $seen_ids[$id] = TRUE;
     }
+    $this->assertTrue($status);
   }

This is the second time I'm seeing this method being `added to a class. I think we should add this to some trait that can be shared. Also I don't think we should pass if there are a no IDs found since that could lead to false positives. Asserting for the absence of something is always tricky. Ideally we'd have a test of the assertion to prove it find duplicates. Let's open a follow-up to address this before we commit.

Lendude’s picture

Issue tags: -Needs followup

follow up added #3000762: Add assertNoDuplicateIds to a functional test trait

We should probably use the version here \Drupal\Tests\file\Functional\FileFieldDisplayTest::assertNoDuplicateIds in this issue too, that fails when the id isn't found

Lendude’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
9.2 KB

copy/paste of the existing assertNoDuplicateIds. It stops git from seeing this as a move, so diff looks different then #12

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

that fails when the id isn't found

That would be convenient, wouldn't it ;)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Re #18 - complex negative asserts are a pain to get right - see #2886615: comment_empty_title_test has invalid hook for todays example.

Committed and pushed 3ba620b652 to 8.7.x and 531be4993f to 8.6.x. Thanks!

  • alexpott committed 3ba620b on 8.7.x
    Issue #2809535 by Lendude, martin107, dawehner, borisson_, alexpott:...

  • alexpott committed 531be49 on 8.6.x
    Issue #2809535 by Lendude, martin107, dawehner, borisson_, alexpott:...
tacituseu’s picture

This introduced test failures on PHP 7.2:
https://www.drupal.org/pift-ci-job/1077348
https://www.drupal.org/pift-ci-job/1077349

There was 1 error:

1) Drupal\FunctionalJavascriptTests\Ajax\MultiFormTest::testMultiForm
count(): Parameter must be an array or an object that implements Countable

/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MultiFormTest.php:82
Lendude’s picture

Status: Fixed » Needs review
FileSize
990 bytes

@tacituseu++

I can reproduce the fail locally, this should fix it.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MultiFormTest.php
@@ -79,7 +79,7 @@ public function testMultiForm() {
-      $this->assertEqual(count($field->find('xpath', '.' . $field_items_xpath_suffix)), 1, 'Found the correct number of field items on the initial page.');

I think this is not the correct change. I think the correct assertion is $this->assertCount(1, $field->findAll('xpath', '.' . $field_items_xpath_suffix), 'Found the correct number of field items on the initial page.');

alexpott’s picture

Status: Needs work » Needs review
FileSize
993 bytes
Lendude’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MultiFormTest.php
@@ -79,7 +79,7 @@ public function testMultiForm() {
+      $this->assertCount(1, $field->findAll('xpath', '.' . $field_items_xpath_suffix), 'Found the correct number of field items on the initial page.');

Yeah fair enough, this way you also detect and fail if there is more then one occurrence.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 35a2e4892c to 8.7.x and 3acd12453d to 8.6.x. Thanks!

  • alexpott committed 35a2e48 on 8.7.x
    Issue #2809535 by Lendude, alexpott, martin107, dawehner, borisson_:...

  • alexpott committed 3acd124 on 8.6.x
    Issue #2809535 by Lendude, alexpott, martin107, dawehner, borisson_:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.