Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- Figure out which part of the test is testing PHP code and which part ajax behaviour
- Extract the ajax behaviour into a test that extends JavascriptTestBase
Remaining tasks
- Choose which test should make it in.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff-26-29.txt | 813 bytes | Anonymous (not verified) |
#29 | 2809471-29.patch | 2.8 KB | Anonymous (not verified) |
#26 | interdiff-22-26.txt | 981 bytes | Anonymous (not verified) |
#26 | 2809471-26.patch | 2.81 KB | Anonymous (not verified) |
#22 | 2809471-22.patch | 2.79 KB | Anonymous (not verified) |
Comments
Comment #2
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #3
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI 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 :)
Comment #5
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWould like some feedback on which test should go in before fixing the full test.
Comment #7
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks! 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).
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedCan anybody reproduce this error? I have green test with #8 and with #3 too.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #14
Lendude@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:
Copy/paste left over
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
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
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks, @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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedWoot! 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.Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commented#14: done + next changes:
$page->findX
with$assert_session->waitForX
,href
checking and pretty look.Comment #18
LendudeI 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?
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedYou'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 tofind()
in all cases except one:Also let Bot check it 200 times, please.
Comment #20
dawehnerJust 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?
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commented#20: nice question! Yes. Should we transfer from
Drupal\Tests\config\Functional\ConfigEntityTest:testCRUDUI()
only a small part related to the javascript?Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedLike 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()
Comment #23
dawehnerThis is sooo much better now! Thank you for rethinking the current patch.
Comment #25
LendudeIt'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?
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commented#23, #25: thanks!
#25: done.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedThis also blocks the #2870439: Convert web tests to browser tests for config module.
Comment #28
dawehnerI think this should be
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentednice spot!
Comment #30
dawehnerThank you
Comment #33
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!