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

Command icon 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

mondrake created an issue. See original summary.

mondrake’s picture

Status: Active » Needs review

OK, 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()).

longwave’s picture

I 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.

mondrake’s picture

Status: Needs review » Needs work

It's a good idea to have linkExists and linkByHrefExists return 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.

mondrake’s picture

Assigned: Unassigned » mondrake

On this, for the other comments.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Did the 'possible' fixes from #5.

mondrake’s picture

Fixed test failure, WebAssert::elementTextEquals() was not dealing properly with array selectors.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

I've re-checked all the changes in this patch and they all look OK to me.

mondrake’s picture

Rerolled.

catch’s picture

Title: Convert assertions involving use of xpath on links to WebAssert » [backport] Convert assertions involving use of xpath on links to WebAssert
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Committed/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.

  • catch committed 6f48525 on 9.3.x
    Issue #3220255 by mondrake, longwave: Convert assertions involving use...

spokje’s picture

Version: 9.3.x-dev » 9.2.x-dev
Assigned: Unassigned » spokje

Let's see if I can reroll this for 9.2.x

spokje’s picture

longwave’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Compared 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.

spokje’s picture

Assigned: spokje » Unassigned
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs another re-roll.

longwave’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Rebased against 9.2.x, one merge conflict that was trivial to solve:

--- a/core/modules/views_ui/tests/src/Functional/DisplayAttachmentTest.php
+++ b/core/modules/views_ui/tests/src/Functional/DisplayAttachmentTest.php
@@@ -45,9 -45,8 +45,8 @@@ class DisplayAttachmentTest extends UIT
      $this->submitForm(['displays[page_1]' => 1], 'Apply');
      // Options summary should be escaped.
      $this->assertSession()->assertEscaped('<em>Page</em>');
 -    $this->assertNoRaw('<em>Page</em>');
 +    $this->assertSession()->responseNotContains('<em>Page</em>');
-     $result = $this->xpath('//a[@id = :id]', [':id' => 'views-attachment-1-displays']);
-     $this->assertEquals(t('Page'), $result[0]->getAttribute('title'));
+     $this->assertSession()->elementAttributeContains('xpath', '//a[@id = "views-attachment-1-displays"]', 'title', 'Page');
      $this->submitForm([], 'Save');
  
      $view = Views::getView('test_attachment_ui');
catch’s picture

Title: [backport] Convert assertions involving use of xpath on links to WebAssert » Convert assertions involving use of xpath on links to WebAssert
Status: Reviewed & tested by the community » Fixed

Committed c3a97f5 and pushed to 9.2.x. Thanks!

  • catch committed c3a97f5 on 9.2.x
    Issue #3220255 by Spokje, mondrake, longwave: Convert assertions...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.