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
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff-2809483-30-35.txt | 1.09 KB | ApacheEx |
#35 | 2809483-35.patch | 7.91 KB | ApacheEx |
#27 | interdiff-26-27.txt | 1.27 KB | michielnugter |
#26 | 2809483-26.patch | 7.84 KB | Lendude |
#22 | 2809483-phpunit-javascripttestbase-formtest-testfieldformjsaddmore-22.patch | 7.8 KB | michielnugter |
Comments
Comment #2
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI 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?
Comment #3
LendudeI will review this face-to-face with @michielnugter.
Comment #4
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI updated the patch after lendude's feedback for some code cleanup, thanks!
interdiff is from before the filename change.
Comment #5
LendudeNice clean up, thanks for that!
This is the only assertion I'm missing in the new test.
Comment #6
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks for the review and spotting that, I added it to the new test.
Comment #7
LendudeThis looks good now, and covers everything in the original test.
Comment #8
dawehnerNice work!
Comment #10
LendudeUnrelated fail, see #2828045: Tests failing with unrelated „Translation: The French translation is not available”, waiting for that to go away before RTBCing again
Comment #11
LendudeBack to RTBC
Comment #14
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedBack to RTBC after a random fail.
Comment #16
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #17
klausiJavascriptTestBase 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!
Comment #18
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedMoving back as the mentioned issue is not critical and not causing random failures anymore.
Comment #19
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #20
alexpott@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.
Comment #21
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented@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?
Comment #22
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedSmall update to conform to the new API where possible.
Also, the mentioned random fails are fixed, this is good to go again?
Comment #24
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #25
dawehnerLet's quickly add a line of doc + add the right visibility.
Comment #26
LendudeNeeded a reroll (so no interdiff), addressed #25, did array() => [] conversion
Comment #27
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks for the reroll, did spot a small error in the test though.
Did a self review:
This can be changed to assertSession()->waitForField instead.
Comment #29
dawehnerIt is amazing what is possible
Nitpick: newline required here :P
Note: The expected value should be first
Comment #30
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks 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.
Comment #31
dawehnerHere is an issue
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.
Comment #32
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWith a little (lot ;)) more experience now as opposed to then I do think that it's solvable with xpath :) Back to needs work!
Comment #33
dawehnerNice!
Comment #35
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Internetdevels, Drupal Ukraine Community commentedI've added a bit xpath stuff based on #30, is it good now?
Comment #36
LendudeThanks @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 :)
Comment #39
xjmI 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!