Closed (fixed)
Project:
Drupal core
Version:
9.1.x-dev
Component:
phpunit
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
14 Mar 2021 at 12:12 UTC
Updated:
12 May 2021 at 12:04 UTC
Jump to comment: Most recent
Problem/Motivation
In #3167880: [meta] Convert assertions involving use of xpath to WebAssert, where possible we identified that use of $this->xpath() can be dangerous as we aren't always testing what we expect to test. In WebAssert we have more appropriate methods to find and assert HTML elements, so we should use them where possible.
This issue is scoped to find XPath selectors for <div> elements and convert those where possible.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3203476
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3203476-convert-assertions-involving
changes, plain diff MR !422
Comments
Comment #2
mondrakeComment #4
mondrakeComment #5
mondrakeAFAICS the remaining instances (regex:
xpath\(.*\/div) are either in Kernel tests or part of more complex loops on results of xpath.Comment #6
mondrakeComment #7
longwaveThis looks pretty good, a few little nits but nothing major - let me know what you think about the followup ideas as well!
Comment #8
mondrakeThanks, fixed as appropriate. I agree changing some Xpath to Css would make things better readable, but would suggest a followup to be postponed on #3167880: [meta] Convert assertions involving use of xpath to WebAssert, where possible, to avoid moving targets.
Comment #9
longwaveThanks, re-reviewed and all looks good now. Agree that any followups to clean up selectors should wait until we have done all these WebAssert conversions.
Comment #10
alexpottThere are outstanding comments to resolve on the MR.
Comment #11
mondrakeHow about adding a
WebAssert::elementTextEquals()method and its negative, instead?Comment #12
longwave@mondrake I think that's a good idea to move this forward
Comment #13
mondrakeOn this
Comment #14
mondrakeComment #15
longwaveelementTextEquals() and the related changes look great, but I question the usefulness of elementTextDoesNotEqual() especially as it's not needed here.
Comment #16
mondrakeSee MR
Comment #17
longwaveLooking good, all review points addressed.
Comment #18
catchWe should add a CR for the new ::elementTextEquals() method, otherwise this looks like good cleanup lots of assertCount() moved to more specific assertions etc.
Comment #19
mondrakeAdded draft CR https://www.drupal.org/node/3211042
Comment #20
longwaveThanks @mondrake!
Comment #23
catchCommitted/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!