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 » field system
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.

ApacheEx’s picture

Status: Active » Needs review
FileSize
2.29 KB

Here is a patch :)

Status: Needs review » Needs work

The last submitted patch, 5: 2809493-5.patch, failed testing. View results

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
2.45 KB

Oh, was my fault. Should work now.

Anonymous’s picture

@ApacheEx, thank you for the active work on this initiative! But do you think that we need to convert the whole test? It will not be very usefull for those, who will add test coverage only for PHP code (back-end part), because they will have to work in the JavascriptTestBase.

By proposed resolution of this issue summary, we should extract only js-part from this test. Perhaps the story of #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase will be useful too. Where we started with the conversion of the whole test (#19 post), but then moved only few lines (#22 post), and it's great.

ApacheEx’s picture

FileSize
2.92 KB
1.35 KB

thank you for your feedback. I'm wondering how this test can be split.
Js-part depends on previous things like "Set up the field settings.", "Create a content type." and so on.
It's a bit different what we have here #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase
Probably you have an idea how it can be?

p.s. Based on @dawehner review in previous issues here is updated patch.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/field/tests/src/FunctionalJavascript/Boolean/BooleanFormatterSettingsTest.php
@@ -102,17 +109,19 @@ public function testBooleanFormatterSettings() {
+      $assert_session->waitForElement('css', '.ajax-new-content');

heh, nice thing to wait for!

I agree with @ApacheEx that there is not much to split here. Usually the best thing to do when splitting is doing it by method, this test only has one method. So I think this is great as is!

Anonymous’s picture

I agree, my bad, sorry!


Formally, we don't need to test any javascript behavior here at all. But today we haven't drupalPost() in phpunit. And this issue about converting (not about refactoring). So, the proposed solution is absolutely have sense!
Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.5 KB
2.15 KB

Or not?) We can definitely use drupalPostForm here. This test appeared in #2428087: Boolean field formatter - default choice appears in list twice, and it makes sense to check without dependency of javascript.

ApacheEx’s picture

hmm, your patch was failed on my local, but let's see :) I still think this test case should be converted to JTB.
UP:
yay, it works. Now, I learned something.
UP 2:
I'm going to update all my patches that I've posted recently to BTB instead of JTB if that makes sense there :) thank you.

Anonymous’s picture

@ApacheEx, thank you for rethinking the tests! D.org does not send an alert after the comment has been edited, so I did not immediately notice your add-ons to last post.

I just want to emphasize, that you did absolutely right when you converted the test to JSB (#9). Because it contains js-imitation (drupalPostAjaxForm()). We usually try not to rethink the logic of the tests during the conversion, because there are many implicit moments, which we can miss.

Therefore, we risk to get disapproval, when we try to annul js-methods in test, like #10 :)

On the other hand, we also adhere to the principle of moving to JSB only the js-behavior. So, for this test, we could write a JSB test that check that settings form appears by button click. But I'm not sure that this is worth check inside this test.

In any case, we have JTB and BTB variants. So, let more experienced people choose the right one, and we'll see ;)

Lendude’s picture

@vaplas and @ApacheEx thanks so much for working on these tests.

My initial reaction was, "if this green, I'm happy". But lets look at what happens here. In the old version we had some fake coverage for JS functionality and no coverage for the non-js version. After the conversion we have either coverage for the JS version (#9) or the non-JS version (#12) of this for form. Don't we want both?

Wouldn't it be great to take #12 and add the javascript test from #9? That would give us coverage for both! Since this issue is only about 1 test, I'm happy to let the 'do the minimal conversion' thing go. This would still be a perfectly reviewable scope.

Will ping some people to see if they agree with my assessment.

Lendude’s picture

As a side note and something we might want to fix here while we are at it:

foreach ($settings as $values) {
      $this->drupalGet('admin/structure/types/manage/' . $this->bundle . '/display');
      $result = $this->xpath('//div[contains(@class, :class) and contains(text(), :text)]', [':class' => 'field-plugin-summary', ':text' => 'Display: TRUE / FALSE']);
      $this->assertEqual(count($result), 1, "Boolean formatter settings summary exist.");
    }

This piece of the test is in the wrong place. It never uses the data in the foreach(), it just checks an identical page 3 times for the same hardcoded TRUE / FALSE option.

It should be inside the previous foreach which should look like:

    foreach ($settings as $values) {
      // Set up the field settings.
      $this->drupalGet('admin/structure/types/manage/' . $this->bundle . '/fields/node.' . $this->bundle . '.' . $this->fieldName);
      $this->drupalPostForm(NULL, [
        'settings[on_label]' => $values[0],
        'settings[off_label]' => $values[1],
      ], t('Save settings'));

      // Open the Manage Display page and trigger the field settings form.
      $this->drupalGet('admin/structure/types/manage/' . $this->bundle . '/display');
      $this->drupalPostForm(NULL, [], $this->fieldName . '_settings_edit');

      // Test that the settings options are present in the correct format.
      foreach ($options as $string) {
        $this->assertText($string);
      }
      $this->assertText(SafeMarkup::format($default, ['@on' => $values[0], '@off' => $values[1]]));
      $this->drupalGet('admin/structure/types/manage/' . $this->bundle . '/display');
      $result = $this->xpath('//div[contains(@class, :class) and contains(text(), :text)]', [':class' => 'field-plugin-summary', ':text' => 'Display: ' . implode(' / ', $values)]);
      $this->assertEqual(count($result), 1, "Boolean formatter settings summary exist.");
    }
ApacheEx’s picture

FileSize
3.9 KB
2.01 KB

First of all, thank you for your regular feedback.

I'm a little bit confused about #15:

  1. if we want to have coverage for both, then it's impossible to do in one file (drupalPostForm doesn't have the same behavior in JTB as it is in BTB).
  2. if we do it in two files (one for BTB and another for JTB), then it's likely a duplicate of code, is not it?.
  3. #15 can happen in lots of other tests (like this #2809499: Convert AJAX part of \Drupal\field\Tests\Number\NumberFieldTest to JavascriptTestBase or this #2809491: Convert AJAX part of \Drupal\field\Tests\Boolean\BooleanFieldTest::testBooleanField to JavascriptTestBase), we need to decide what to do in such situation? Prefer BTB or use JTB or somehow coverage both cases.

p.s. I've updated a patch #12 based on #16.

dawehner’s picture

if we do it in two files (one for BTB and another for JTB), then it's likely a duplicate of code, is not it?.

I think duplicate of test code is way less of an issue than missing test coverage.
On the other hand the actual user will deal with the JS user, so that's where we should focus our test coverage on.

Still I think we should try to not loose test coverage and as such duplicate the test code, at least partially.

Lendude’s picture

On the other hand the actual user will deal with the JS user, so that's where we should focus our test coverage on.

Yeah absolutely, if we choose to cover one path, it should be the JS path.

And yes it will create some duplicate code to cover both paths, mostly in setUp. We need to get the system in the same state and then follow two different paths from there. Well worth it I think.

So I'd go for something like this. Interdiff didn't want to make an interdiff for some reason.

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.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

BTB contains the basic logic. JTB checks ajax support. It makes sense! Thanks for the work and discussion of this moment.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
    // Define what the "default" option should look like, depending on the
    // field settings.
    $default = 'Field settings (@on / @off)';
      $this->assertText(SafeMarkup::format($default, ['@on' => $values[0], '@off' => $values[1]]));

Is still in the javascript test. Whilst it was fixed nicely in the BTB test.

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
7.94 KB

Here is updated patch (from #19 comment) based on @alexpott's comment.
It's also rerolled with 8.6.x branch. That's why no interdiff.

scuba_fly’s picture

Assigned: Unassigned » scuba_fly
scuba_fly’s picture

Assigned: scuba_fly » Unassigned
FileSize
7.72 KB
850 bytes

Needed to extend the WebDriverTestBase instead of the JavascriptTestBase.
Here's a new patch.

Status: Needs review » Needs work

The last submitted patch, 25: 2809493-25.patch, failed testing. View results

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
8.11 KB
2.15 KB

Let's try with small improvements.

Status: Needs review » Needs work

The last submitted patch, 27: 2809493-27.patch, failed testing. View results

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
8.32 KB

#25 was placed in old place (field/src/Tests/Boolean/BooleanFormatterSettingsTest).
For interdiff - see #27.

Now, should be fine.

dawehner’s picture

+++ b/core/modules/field/tests/src/Functional/Boolean/BooleanFormatterSettingsTest.php
@@ -46,7 +45,14 @@ protected function setUp() {
-    $admin_user = $this->drupalCreateUser(['access content', 'administer content types', 'administer node fields', 'administer node display', 'bypass node access', 'administer nodes']);
+    $admin_user = $this->drupalCreateUser([
+      'access content',
+      'administer content types',
+      'administer node fields',
+      'administer node display',
+      'bypass node access',
+      'administer nodes',

@@ -101,28 +103,31 @@ public function testBooleanFormatterSettings() {
         'settings[off_label]' => $values[1],
-      ], 'Save settings');
+      ], t('Save settings'));

Some of these changes seems unnecessary and makes it harder to review for others.

ApacheEx’s picture

FileSize
7.52 KB
1.37 KB

Yeah, probably. Always want to improve something :)

scuba_fly’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DevDaysLisbon

I agree with Comment #30 dawehner, I think I had some settings that autocorrects this. I will keep an eye on this for a next patch.
I think #31 is good to go.

Thanks for working on this ApacheEx

+++ b/core/modules/field/tests/src/Functional/Boolean/BooleanFormatterSettingsTest.php
@@ -111,18 +107,20 @@ public function testBooleanFormatterSettings() {
-      $result = $this->xpath('//div[contains(@class, :class) and contains(text(), :text)]', [':class' => 'field-plugin-summary', ':text' => 'Display: TRUE / FALSE']);
+      $result = $this->xpath('//div[contains(@class, :class) and contains(text(), :text)]', [
+        ':class' => 'field-plugin-summary',
+        ':text' => (string) t('Display: @true_label / @false_label', ['@true_label' => $values[0], '@false_label' => $values[1]]),
+      ]);

I think this is nice, when changing things correct to the right code style. But only for the changes that you do. Lets leave the other stuff out for now to make the review easier.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 2809493-31.patch, failed testing. View results

ApacheEx’s picture

FileSize
7.52 KB

rerolled with 8.6.x

ApacheEx’s picture

Status: Needs work » Needs review
dawehner’s picture

Thank you for rerolling @ApacheEx!

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, all feedback has been addressed and we now have proper coverage for both the javascript and non-javascript paths.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ea5f353 and pushed to 8.7.x. Thanks!
Committed 9f72d15 and pushed to 8.6.x. Thanks!

  • alexpott committed ea5f353 on 8.7.x
    Issue #2809493 by ApacheEx, vaplas, scuba_fly, Lendude, dawehner:...

  • alexpott committed 9f72d15 on 8.6.x
    Issue #2809493 by ApacheEx, vaplas, scuba_fly, Lendude, dawehner:...

Status: Fixed » Closed (fixed)

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