Problem/Motivation

NodeDisplayConfigurableTest relies heavily on RDF to detect specific elements, but we are removing RDF from core.

Steps to reproduce

Proposed resolution

Refactor the test so it no longer relies on RDF.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

smustgrave’s picture

Status: Active » Needs review
FileSize
3.56 KB

Took a shot

longwave’s picture

Status: Needs review » Needs work

Thanks for picking this up.

  1. +++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
    @@ -90,15 +90,12 @@ public function testDisplayConfigurable(string $theme, string $metadata_region,
    -    $assert->elementExists('css', 'div[rel="schema:author"]');
    ...
    -    $assert->elementNotExists('css', 'div[rel="schema:author"]');
    

    Not sure we should just remove these, we need to find another way of identifying the div.

  2. +++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
    @@ -148,12 +145,14 @@ protected function assertNodeHtml(NodeInterface $node, UserInterface $user, bool
    +      $assert->elementTextContains('css', "$uid_selector", $user->getAccountName());
    

    Should this also check $html_element still?

smustgrave’s picture

#3.1
Do we need those assertions? The author value is checked below those. Seemed those assertions were testing for the metadata that is now gone.

#3.2
Without rdf seems to wrap in 1 less div or span

longwave’s picture

Thanks for the swift response.

#4.1 Agree after looking again we don't need these, they aren't testing anything useful except the RDF output which is now gone.

#4.2 The number of elements is the same, the issue is that it's difficult to find the correct one now. The Stark output looks like this:

<article role="article">
  <div>
    <div>Sat, 08/13/2022 - 07:58</div>
    <div><p>oSIlpyA8gKsXFoOqdGMdRa76OCaXyD49</p></div>
    <div>
      <div>Authored by</div>
      <div>k9INAms0KBHjCd</div>
    </div>
  </div>
</article>

The RDF output conveniently tagged the author name, but now it's pretty difficult to find except by counting divs - and that will make the test more fragile for future changes IMO, so I think it's fine just to assert the existence of the output.

Two more minor review comments:

  1. +++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
    @@ -148,12 +145,14 @@ protected function assertNodeHtml(NodeInterface $node, UserInterface $user, bool
    +      $assert->elementTextContains('css', "$uid_selector", $user->getAccountName());
    

    The quotes can be removed from around $uid_selector.

  2. +++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
    @@ -148,12 +145,14 @@ protected function assertNodeHtml(NodeInterface $node, UserInterface $user, bool
    +      if ($field_classes) {
    +        $assert->elementExists('css', $created_selector);
    +      }
    

    Not sure this is necessary; we check elementTextContains() for this above, so we already know that it exists?

longwave’s picture

Issue summary: View changes
smustgrave’s picture

5.2 added that best when stable and stable9 are run the date doesn’t get an attribute or class so the assertion fails

Will address 5.1

longwave’s picture

Re #5.2 but why was this added in the first place? I don't see what this is replacing.

smustgrave’s picture

FileSize
955 bytes
3.56 KB

The original code was

$assert->elementExists('css', 'span[property="schema:dateCreated"]');

Changed it up to look for the css class instead but that only works for themes with classes

Addressed 5.1

longwave’s picture

Status: Needs work » Reviewed & tested by the community

OK, the work here is done, I think this is ready to go in.

  • catch committed 1fbf0bd on 10.0.x
    Issue #3303278 by smustgrave, longwave: Decouple...
  • catch committed 97eb413 on 10.1.x
    Issue #3303278 by smustgrave, longwave: Decouple...
  • catch committed 48c561e on 9.5.x
    Issue #3303278 by smustgrave, longwave: Decouple...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked to 9.5.x and 9.4.x, thanks!

Status: Fixed » Closed (fixed)

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