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

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

michielnugter’s picture

I recreated the behavior as far as I understand the current test. As I see it the test only tests ajax behavior and no php behavior, therefore the test is fully converted to a JavascriptTestBase.

I tried to assume nothing and check everything one step at a time.

I'd love some feedback on it!

1 thing I'm not sure about:
- The original test used entity_get_form_display which is deprecated. Couldn't figure out the correct way yet, should it be converted as well?

Lendude’s picture

Status: Needs review » Needs work

I will review this face-to-face with @michielnugter.

michielnugter’s picture

I updated the patch after lendude's feedback for some code cleanup, thanks!

interdiff is from before the filename change.

Lendude’s picture

Nice clean up, thanks for that!

+++ b/core/modules/field/src/Tests/FormTest.php
@@ -389,62 +389,6 @@ function testFieldFormMultivalueWithRequiredRadio() {
-    $this->assertNoField("{$field_name}[" . ($delta + 1) . '][value]', 'No extraneous widget is displayed');

This is the only assertion I'm missing in the new test.

michielnugter’s picture

Thanks for the review and spotting that, I added it to the new test.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

This looks good now, and covers everything in the original test.

dawehner’s picture

Nice work!

Status: Reviewed & tested by the community » Needs work
Lendude’s picture

Unrelated fail, see #2828045: Tests failing with unrelated „Translation: The French translation is not available”, waiting for that to go away before RTBCing again

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

michielnugter’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC after a random fail.

Status: Reviewed & tested by the community » Needs work
michielnugter’s picture

Status: Needs work » Reviewed & tested by the community
klausi’s picture

Status: Reviewed & tested by the community » Postponed

JavascriptTestBase has at least 3 random test fails right now, so we need to stop converting tests to JavascriptTestBase until #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly is resolved. Sorry!

michielnugter’s picture

Status: Postponed » Needs review

Moving back as the mentioned issue is not critical and not causing random failures anymore.

michielnugter’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

@klausi, @michielnugter - what's the current state of random fails in javascript test base tests. The resolution to #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly was to just skip the test for now - so #17 still seems relevant.

michielnugter’s picture

@alexpott
As I understood it, the critical part was removed by disabling the test and as it's not a critical issue anymore work on other JavascriptTestBase tests could continue.

There is another critical issue for JTB though: #2831603

So postponed untill both are fixed or both are at least not critical anymore?

michielnugter’s picture

Small update to conform to the new API where possible.

Also, the mentioned random fails are fixed, this is good to go again?

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
dawehner’s picture

+++ b/core/modules/field/tests/src/FunctionalJavascript/FormJSAddMoreTest.php
@@ -0,0 +1,119 @@
+
+  function testFieldFormJSAddMore() {

Let's quickly add a line of doc + add the right visibility.

Lendude’s picture

Needed a reroll (so no interdiff), addressed #25, did array() => [] conversion

michielnugter’s picture

Thanks for the reroll, did spot a small error in the test though.

Did a self review:

+++ b/core/modules/field/tests/src/FunctionalJavascript/FormJSAddMoreTest.php
@@ -0,0 +1,122 @@
+
...
+    $assert_session->assertWaitOnAjaxRequest();

This can be changed to assertSession()->waitForField instead.

Status: Needs review » Needs work

The last submitted patch, 27: 2809483-27.patch, failed testing.

dawehner’s picture

It is amazing what is possible

  1. +++ b/core/modules/field/tests/src/FunctionalJavascript/FormJSAddMoreTest.php
    @@ -0,0 +1,123 @@
    +class FormJSAddMoreTest extends JavascriptTestBase {
    +  /**
    

    Nitpick: newline required here :P

  2. +++ b/core/modules/field/tests/src/FunctionalJavascript/FormJSAddMoreTest.php
    @@ -0,0 +1,123 @@
    +    $this->assertEquals($field_0->getValue(), '1', 'Value for the first item has not changed.');
    

    Note: The expected value should be first

michielnugter’s picture

Status: Needs work » Needs review
FileSize
949 bytes
7.91 KB

Thanks for those and they're fixed!

The more I work on the JavascriptTestBase tests the more I dislike the assertWaitOnAjaxRequest method. It promotes incorrect testing and is one of the major reasons for random fails. I think we should always check for what we expect in the DOM. This works around all the problems we've had so far (debounce, ajax handling by drupal, browser based events, etc). I've been thinking about this a lot and will update the documentation soon if I have a good way of putting my thoughts on paper.

Fail was a random fail btw which has an RTBC issue: #2870146: Even more random fails in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest.

dawehner’s picture

The more I work on the JavascriptTestBase tests the more I dislike the assertWaitOnAjaxRequest method. It promotes incorrect testing and is one of the major reasons for random fails. I think we should always check for what we expect in the DOM. This works around all the problems we've had so far (debounce, ajax handling by drupal, browser based events, etc). I've been thinking about this a lot and will update the documentation soon if I have a good way of putting my thoughts on paper.

Here is an issue

+++ b/core/modules/field/tests/src/FunctionalJavascript/FormJSAddMoreTest.php
@@ -0,0 +1,124 @@
+    $field_0_row = $field_0->getParent()->getParent()->getParent();
+    $field_2_row = $field_2->getParent()->getParent()->getParent();
+    $drag_handle = $field_0_row->find('css', '.tabledrag-handle');

Is it just me or are these multiple parent calls a little bit dangerous, when it comes down to changes in the Drupal markup? I'm wondering whether we could use some different xpath magic instead.

michielnugter’s picture

Status: Needs review » Needs work

With a little (lot ;)) more experience now as opposed to then I do think that it's solvable with xpath :) Back to needs work!

dawehner’s picture

Nice!

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: Needs work » Needs review
FileSize
7.91 KB
1.09 KB

I've added a bit xpath stuff based on #30, is it good now?

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @ApacheEx

Still loving how this tests turned out. Feedback has been addressed, random fails in JavascriptTestBase are under control, back to RTBC after 11 months :)

  • xjm committed 99ea6ac on 8.5.x
    Issue #2809483 by michielnugter, ApacheEx, Lendude, dawehner: Convert...

  • xjm committed fc02704 on 8.4.x
    Issue #2809483 by michielnugter, ApacheEx, Lendude, dawehner: Convert...
xjm’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

I read over the old method and the new test side by side. In addition to converting it to a JS test, we're also getting rid of some totally unnecessary random weighting in favor of testing it directly with specific fixtures, and replacing a deprecated function call. That all seems reasonable since we have to rewrite the main portion of the test anyway.

Committed and pushed to 8.5.x. I also backported it to 8.4.x since this doesn't affect public test API to keep the branches in sync for bugfixes. Thanks!

Status: Fixed » Closed (fixed)

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