Closed (fixed)
Project:
Drupal core
Version:
9.2.x-dev
Component:
phpunit
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
23 Jun 2021 at 07:46 UTC
Updated:
23 Aug 2021 at 15:49 UTC
Jump to comment: Most recent
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.
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!