Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 |
---|---|---|---|
#25 | 2809535-25.patch | 993 bytes | alexpott |
#23 | 2809535-23.patch | 990 bytes | Lendude |
#17 | 2809535-17.patch | 9.2 KB | Lendude |
#17 | interdiff-2809535-12-17.txt | 13.3 KB | Lendude |
#12 | 2809535-12.patch | 4.73 KB | Lendude |
Comments
Comment #3
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #6
martin107 CreditAttribution: martin107 as a volunteer commentedI am providing most of the solution here which I am cutting out from a monster patch which is too big to review. ( as a first step I am just providing a cut out patch - I will list the work to be done at the end )
As part of spreading the monster patch in several splinter patches ... things need to be coordinated.
This patch needs AjaxTestBase, which will be provided by #2809521: Convert AJAX part of \Drupal\system\Tests\Ajax\AjaxInGroupTest to WebDriverTestBase when it lands.. in short I am postponing this iisue
As appropriate this patch replaces drupalPostAjaxForm() with a test employing ->pess() ->findAll and ->assertFieldByValue() calls.
So here is a list of things to do when this issue reopens
1) The patch contains function calls to assertWaitOnAjaxRequest() which is a bad code smell and need to be refactored.
2) We must update AjaxTestBase with a trait/method which contains the code in AjaxTestBase:: assertNoDuplicateIds() [ here is link to the original moster patch --- https://www.drupal.org/project/drupal/issues/2862510#comment-12225746 ]
Comment #8
LendudeNo AjaxTestBase, no need to wait.
Comment #9
LendudeSo something like this.
Comment #10
dawehnerShould we wait for this element to appear?
Is there a reason why we don't use just
strtr
Comment #11
borisson_#10.2, we don't need to use strtr or sprintf, we can just do this with string concatenation.
Comment #12
Lendude#10.1 nope, because we need to wait for 2 elements matching that to appear, which would be hard to do, since they match the same selector. I agree it would be better to do so, but don't really see a good way of doing it.
#10.2/#11 yeah simple concat works, it was a straight copy paste from
\Drupal\KernelTests\AssertContentTrait::assertNoDuplicateIds
. Also since it's a fail() the message should probably be negative, not sure, but that would make sense to me.Comment #14
borisson_Awesome,, thanks for those changes @Lendude!
Comment #15
alexpottThis is the second time I'm seeing this method being `added to a class. I think we should add this to some trait that can be shared. Also I don't think we should pass if there are a no IDs found since that could lead to false positives. Asserting for the absence of something is always tricky. Ideally we'd have a test of the assertion to prove it find duplicates. Let's open a follow-up to address this before we commit.
Comment #16
Lendudefollow up added #3000762: Add assertNoDuplicateIds to a functional test trait
We should probably use the version here
\Drupal\Tests\file\Functional\FileFieldDisplayTest::assertNoDuplicateIds
in this issue too, that fails when the id isn't foundComment #17
Lendudecopy/paste of the existing assertNoDuplicateIds. It stops git from seeing this as a move, so diff looks different then #12
Comment #18
dawehnerThat would be convenient, wouldn't it ;)
Comment #19
alexpottRe #18 - complex negative asserts are a pain to get right - see #2886615: comment_empty_title_test has invalid hook for todays example.
Committed and pushed 3ba620b652 to 8.7.x and 531be4993f to 8.6.x. Thanks!
Comment #22
tacituseu CreditAttribution: tacituseu commentedThis introduced test failures on PHP 7.2:
https://www.drupal.org/pift-ci-job/1077348
https://www.drupal.org/pift-ci-job/1077349
Comment #23
Lendude@tacituseu++
I can reproduce the fail locally, this should fix it.
Comment #24
alexpottI think this is not the correct change. I think the correct assertion is
$this->assertCount(1, $field->findAll('xpath', '.' . $field_items_xpath_suffix), 'Found the correct number of field items on the initial page.');
Comment #25
alexpottComment #26
LendudeYeah fair enough, this way you also detect and fail if there is more then one occurrence.
Comment #27
alexpottCommitted and pushed 35a2e4892c to 8.7.x and 3acd12453d to 8.6.x. Thanks!