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
AssertLegacyTrait::assertFieldByXPath() is deprecated in drupal:8.3.0 and is removed from drupal:10.0.0. Use $this->xpath() instead and check the values directly in the test. See https://www.drupal.org/node/3129738
Proposed resolution
Replace assertFieldByXPath() and assertNOFieldByXPath() with $this->xpath().
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#81 | rawdiff_76-81.txt | 2.31 KB | mondrake |
#81 | 3139429-81.patch | 123.33 KB | mondrake |
#79 | 3139429-79.patch | 121.89 KB | paulocs |
#76 | 3139429-76.patch | 123.3 KB | mondrake |
#70 | interdiff_66-70.txt | 2.13 KB | mondrake |
Comments
Comment #2
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #3
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #4
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #6
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #7
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedWorking on failed tests
Comment #8
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedHi, I found the use of $this->xpath() was not correct.
Also, $this->assertNoFieldByXPath is deprecated but still used ion patch.
So solved the issues and uploading the patch.
Comment #9
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #10
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedMissed out a few functions descriptions. Adding the patch and interdiff.
Comment #11
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #12
cburschkapatch isn't applying
Comment #13
cburschkaSimple reroll
Comment #15
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #16
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed some of below errors:
Still WIP. Will take a look into the failed ones after one more pass
Comment #17
cburschkaThis should be ->assertNotEmpty(), not ->assertTrue(!empty()). (There are many of these, and probably similar ones.)
This one looks definitely incorrect. The assertion is turned from asserting that the field does not exist to asserting that the field does exist.
(Note, this was already in the patch before the most recent version, since #8)
Comment #18
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch. and have fixed #17.2. Working on fixing occurances of #17.1
Comment #19
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed some more failing tests .
Comment #20
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedComment #21
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedWorking on it.
Comment #22
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedPlease review the patch.
Thanks
Comment #23
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedFixed PHPLint errors.
Please ignore last path and review this one.
Thanks
Comment #25
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedFixed failing test cases.
Please review the patch.
Thanks
Comment #27
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedComment #28
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedFixed remaining failed test cases.
Please review the patch.
Thanks
Comment #29
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedAll occurrences of $this->assertFieldByXPath() and $this->assertNoFieldByXPath() have been replaced.
The deprecation message suppression has been removed.
Added the @expectedDeprecation to the docblock of
testXpathAsserts
Please review the patch.
Thanks
Comment #31
narendra.rajwar27Comment #32
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedTests are failing since assertFieldByName() uses AssertLegacyTrait::assertFieldByXPath. We will have to wait till https://www.drupal.org/project/drupal/issues/3139406 lands.
Comment #33
narendra.rajwar27Oops, forget to unassign this issue.
Comment #34
mondrakeComment #35
mondrakeBlockers were committed so this can be worked on now. I am on this.
Comment #36
mondrakeTrying to use WebAssert methods instead of PHPUnit assertions on $this->xpath results. Only a start patch.
Comment #38
mondrakeComment #39
mondrakeComment #40
mondrakeComment #41
mondrakeComment #42
mondrakeComment #43
mondrakeComment #44
mondrakeComment #45
jungleFixing PHPLint error
Comment #46
mondrakeComment #47
mondrakeComment #48
mondrakeComment #49
mondrakeThis should complete the removal of calls from Functional tests. Note that the equally named methods in Kernel tests are NOT in scope for removal here. We need a deprecation test, but if it turns back green this is ready for reviews.
Comment #51
mondrakeAdded deprecation test and fixed failures.
Comment #52
mondrakeComment #53
paulocsIt needs reroll.
I'll do it.
Comment #54
paulocsComment #55
paulocsPlease review.
Comment #56
longwaveA few places where things can be improved but overall this is looking good, great effort everyone - it's tricky getting all these right.
Worth breaking this out into two lines?
The
not(@disabled)
check is lost here.Here I think we can use
fieldExists()
thenhasClass()
separately.This can be
elementContains()
for the text part.Same
Same
The
link*
methods returning the element would help here, this will have to do otherwise.This could be
fieldExists()
and then a separate attribute check.Can we use
fieldExists()
and thenhasClass()
here?Again a shame that the
link*
methods don't return the element, as that could mean these could be broken into separate assertions.buttonNotExists()
?Can we use
checkboxChecked()
? (even though it's a radio, it seems like it should still work)Surely we actually want
fieldValueEquals('pass', '')
here?Comment #57
mondrakeThanks, on this.
Comment #58
mondrakeDone #56, with the exception of:
7, 10 - we'd need a separate issue to get
linkExists
andlinkExistsByHref
to return the identified node, oos here. For a couple of usages not sure about the effort, though.6 - it's a subtle difference to include the selection in the xpath vs checking the value: the former works as a filter, the latter just would use the first node returned and check its value. If we are sure that the xpath w/o the condition returns one node only - OK it the same thing. But in this case I'm not convinced that's the case so I prefer to leave it as part of the filter.
Comment #60
longwaveFor #56.6 I agree about the last case but the first two are just
td[1]
and that will always only be one element anyway.Comment #61
mondrakeBut
"//tr/td[1]"
would return all thetd[1]
of all the rows in the table, no?Comment #62
mondrakeFixing failure in #58.
Comment #63
longwaveAhh yes, sorry, you're absolutely right. The other changes look great so RTBC if bot agrees.
Comment #65
mondrakeRe failure in #62,
it looks like Mink cannot process fields of type 'password'. Actually I suspect the current code is just a false negative, since it's asserting absence. Working on turning this to a positive check that the field exists with no value.
Comment #66
longwaveI don't think that's the issue, it's that either the field exists and is empty, or the field doesn't exist at all (in the case of the flood protection being triggered). The old XPath tried to cater for both these, but it's better to assert the two cases separately.
Comment #67
mondrakeI got to solve that differently,
but @longwave's solution in #66 is better, I think.
Comment #69
mondrakemmm, I am afraid we need to escape $label here
Comment #70
mondrakeDoing #69
Comment #71
mondrakeComment #72
longwave+1 to proper escaping, looks good to me.
Comment #74
mondrakeRandom fail
Comment #75
quietone CreditAttribution: quietone as a volunteer commentedPatch no longer applies.
Comment #76
mondrakeRerolled
Comment #77
catchNeeds a re-roll.
Comment #78
paulocsI'll do it.
Comment #79
paulocsComment #80
mondrakeIn #79 we have lost the deprecation test that is in #76,
we no longer use @expectedDeprecation annotations, we should use calls to
$this->expectDeprecation()
.Comment #81
mondrakeRerolled
Comment #85
catchGood clean-up, nice to see so many of the assertions get more readable in the process.
Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!