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

  1. Choose which test should make it in.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#29 interdiff-26-29.txt813 bytesAnonymous (not verified)
#29 2809471-29.patch2.8 KBAnonymous (not verified)
#26 interdiff-22-26.txt981 bytesAnonymous (not verified)
#26 2809471-26.patch2.81 KBAnonymous (not verified)
#22 2809471-22.patch2.79 KBAnonymous (not verified)
#19 interdiff-17-19.txt4.29 KBAnonymous (not verified)
#19 2809471-19.patch11.9 KBAnonymous (not verified)
#19 x200_2809471-19.patch12.46 KBAnonymous (not verified)
#17 interdiff-13-17.txt9 KBAnonymous (not verified)
#17 2809471-17.patch12.09 KBAnonymous (not verified)
#13 interdiff-11-13.txt1.01 KBAnonymous (not verified)
#13 2809471-13.patch11.74 KBAnonymous (not verified)
#11 interdiff-8-11.txt3.8 KBAnonymous (not verified)
#11 2809471-11.patch11.62 KBAnonymous (not verified)
#8 interdiff-3-8.txt6.35 KBAnonymous (not verified)
#8 2809471-8.patch11.82 KBAnonymous (not verified)
#3 2809471-ConfigEntityTest-testCRUDUI-full.patch12.04 KBmichielnugter
#3 2809471-ConfigEntityTest-testCRUDUI-partial.patch3.86 KBmichielnugter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

michielnugter’s picture

Assigned: Unassigned » michielnugter
michielnugter’s picture

I converted the test and created a full convert and a partial convert.

Both patches retain the original test as it tests the non-js functionality as well.

The partial test only covers the AJAX part of the test in the functional javascript test.
The full test covers everything except the non-js validation in the functional javascript test.

I'm not sure which test is preferred. The full test does test 1 additional JavaScripty thing: opening the other actions pulldown and assert the delete button is visible.

Not sure on which test should go in, I'll leave that up to the reviewer :)

Status: Needs review » Needs work

The last submitted patch, 3: 2809471-ConfigEntityTest-testCRUDUI-full.patch, failed testing.

michielnugter’s picture

Would like some feedback on which test should go in before fixing the full test.

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 » config.module
Issue tags: +phpunit initiative
Anonymous’s picture

Status: Needs work » Needs review
FileSize
11.82 KB
6.35 KB

Thanks! It looks logical and simplifies the work in #2870439: Convert web tests to browser tests for config module. Reroll with small corrections of coding standards + replace Empty on Null (although my tests pass in both cases, which is strange).

Status: Needs review » Needs work

The last submitted patch, 8: 2809471-8.patch, failed testing. View results

Anonymous’s picture

Can anybody reproduce this error? I have green test with #8 and with #3 too.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
11.62 KB
3.8 KB

Status: Needs review » Needs work

The last submitted patch, 11: 2809471-11.patch, failed testing. View results

Anonymous’s picture

Status: Needs work » Needs review
FileSize
11.74 KB
1.01 KB
Lendude’s picture

Status: Needs review » Needs work

@vaplas nice work! I tried to reproduce the fails in #8, but couldn't. Why was ->find('css', 'xxxxx') causing problems?

Looking at this, this looks like a great conversion of the original coverage. I think you got everything.

Some nits:

  1. +++ b/core/modules/config/tests/src/FunctionalJavascript/ConfigEntityTest.php
    @@ -0,0 +1,160 @@
    + * Tests the View UI filter criteria group dialog.
    

    Copy/paste left over

  2. +++ b/core/modules/config/tests/src/FunctionalJavascript/ConfigEntityTest.php
    @@ -0,0 +1,160 @@
    +    $assert_session->statusCodeEquals(200);
    ...
    +    $assert_session->statusCodeEquals(200);
    ...
    +    $assert_session->statusCodeEquals(200);
    ...
    +    $assert_session->statusCodeEquals(200);
    ...
    +    $assert_session->statusCodeEquals(200);
    ...
    +    $assert_session->statusCodeEquals(200);
    

    Don't use this please, it will be removed soon, I think we can just leave it out of this test, since after all the calls there are proper assertions to the content of the page. So the statusCodeEquals calls serve not real purpose here anyway

  3. +++ b/core/modules/config/tests/src/FunctionalJavascript/ConfigEntityTest.php
    @@ -0,0 +1,160 @@
    +    $assert_session->assertWaitOnAjaxRequest();
    

    Please try to stay away from assertWaitOnAjaxRequest(), use waitForElement and the methods like that. So be specific about what you are waiting for, this cuts down on possible random fails

Anonymous’s picture

Thanks, @Lendude! I have some fresh ideas about the possible reasons for the failure of tests by Mr. Bot. But chief among them - the magic!) I'll try to check the ideas and correct the flaws mentioned in the #14.

Anonymous’s picture

Woot! Mr. Bot just run test in subfolder 'checkout' and has links like:

<a href="/checkout/admin/structure/config_test/manage/id1">Edit</a>

I absolutely forgot about that, although I already came across this with working on rest tests for File. base_path() . "path/to/link" should decide the difference.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
12.09 KB
9 KB

#14: done + next changes:

  • replace random values with static values,
  • replace all $page->findX with $assert_session->waitForX,
  • added new methods for strict href checking and pretty look.
Lendude’s picture

Status: Needs review » Needs work

I like the idea of waiting vs finding but:

Patch in #13
Time: 38.64 seconds, Memory: 6.00MB

OK (1 test, 13 assertions)

Process finished with exit code 0

Patch in #17
Time: 1.16 minutes, Memory: 6.00MB

OK (1 test, 21 assertions)

Process finished with exit code 0

It doubles the run time of the test, so lets just use find. Or are there some faulty waits in there that actually run the full length of the wait?

Anonymous’s picture

You're right! At first I just wanted to reduce the timeout, but seems wait() to really not be of use in this case. So, rollback to find() in all cases except one:

+    // Check that the dependent element is shown after selecting a 'size' value.
+    $field_size_value = $assert_session->waitForField('size_value');

Also let Bot check it 200 times, please.

dawehner’s picture

Just to be clear, we don't need the wait() methods because we actually dont' deal with any kind of JS while accessing the page the first time?

Anonymous’s picture

#20: nice question! Yes. Should we transfer from Drupal\Tests\config\Functional\ConfigEntityTest:testCRUDUI() only a small part related to the javascript?

    $id = strtolower($this->randomMachineName());
    $field_id->setValue($id);
    $field_label->setValue($this->randomString());
    $field_size->setValue('custom');
    $assert_session->assertWaitOnAjaxRequest();

    // Check that the dependent element is shown after selecting a 'size' value.
    $field_size_value = $page->findField('size_value');
    $this->assertNotNull($field_size);
    $this->assertNotNull($field_size_value);
Anonymous’s picture

Like this (without interdiff, because this is bit another direction).

Also test in FunctionalJavascript includes only checking the appearing 'size value' field, but not 'size' field, because it is already tested in the remainder of the ConfigEntityTest:testCRUDUI()

dawehner’s picture

This is sooo much better now! Thank you for rethinking the current patch.

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.

Lendude’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs review » Needs work
+++ b/core/modules/config/src/Tests/ConfigEntityTest.php
@@ -317,29 +317,17 @@ public function testCRUDUI() {
+    // Test the same scenario but it in a non-JS case by using a 'js-hidden'
+    // submit button.

It's not really the same scenario anymore, since the js version is removed.

"Create a configuration entity with a property that uses AJAX to show extra form elements. Test this scenario it in a non-JS case by using a 'js-hidden' submit button.
@see \Drupal\Tests\config\FunctionalJavascript\ConfigEntityTest::testAjaxOnAddPage"

Something like that?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
981 bytes

#23, #25: thanks!
#25: done.

Anonymous’s picture

dawehner’s picture

Test this scenario it in a non-JS case by using a 'js-hidden' submit button.

I think this should be

Test this scenario in a non-JS case by using a 'js-hidden' submit button.
Anonymous’s picture

nice spot!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

  • catch committed 8ec6e83 on 8.5.x
    Issue #2809471 by vaplas, michielnugter, dawehner, Lendude: Convert AJAX...

catch credited catch.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 9122942 on 8.4.x
    Issue #2809471 by vaplas, michielnugter, dawehner, Lendude: Convert AJAX...

Status: Fixed » Closed (fixed)

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