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 link 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-3220255
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:
Comments
Comment #2
mondrakeComment #4
mondrakeOK, I think this is for review now. There are several cases where I decided not to convert because it looked to me that readibility was not improving (for instance where xpath() is used in custom assertions rather than tests, or where there are loops on the result of an xpath()).
Comment #5
longwaveI read all the conversions and they all look sensible. Two changes look to be out of scope, and a few comments about how I wish we could do things another way but probably out of scope to add functionality to WebAssert here? Will wait for your response but I think this is close to RTBC.
Comment #6
mondrakeIt's a good idea to have
linkExistsandlinkByHrefExistsreturn the node, but I think it should be done separately from this issue. We can try that and postpone here, or just move this on and have a follow up.Comment #7
mondrakeOn this, for the other comments.
Comment #8
mondrakeDid the 'possible' fixes from #5.
Comment #9
mondrakeFixed test failure, WebAssert::elementTextEquals() was not dealing properly with array selectors.
Comment #10
longwaveI've re-checked all the changes in this patch and they all look OK to me.
Comment #11
mondrakeRerolled.
Comment #12
catchCommitted/pushed to 9.3.x.
We should backport to 9.2.x to keep the tests in sync for other patches, but this doesn't cherry-pick cleanly so tagging for a re-roll.
Comment #15
spokjeLet's see if I can reroll this for
9.2.xComment #17
spokjeLooks like a random fail #3210432: [random test failure] LayoutBuilderQuickEditTest::testQuickEditIgnoresDuplicateFields(), let's give TestBot another go.
Comment #18
longwaveCompared the patches with
diff -u <(curl https://git.drupalcode.org/project/drupal/-/merge_requests/838.diff) <(curl https://git.drupalcode.org/project/drupal/-/merge_requests/1024.diff)and the one difference is the change to file_url_transform_relative() which is correct, so this looks OK to me. Also looks like a random fail last time, so this is RTBC.Comment #19
spokjeComment #20
catchNeeds another re-roll.
Comment #21
longwaveRebased against 9.2.x, one merge conflict that was trivial to solve:
Comment #22
catchCommitted c3a97f5 and pushed to 9.2.x. Thanks!