Taxonomy tests should not rely on Classy

Going to go bold and to mark RTBC. It's just selector changes. Looks clean to me. Left some notes to help anyways else reviewing.

1. +++ b/core/modules/taxonomy/tests/src/Functional/TermTest.php
   @@ -384,12 +384,12 @@ public function testTermInterface() {
   -    $this->assertSession()->elementExists('xpath', '//div[contains(@class, "field--name-description")]');
   +    $this->assertSession()->elementExists('xpath', '//div[@class="views-element-container"]/div/header/div/div/p');
   ...
   -    $this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "field--entity-taxonomy-term--description")]');
   +    $this->assertSession()->elementNotExists('xpath', '//div[@class="views-element-container"]/div/header/div/div/p');
   

   I was weary about this at first, but realized the test installs Views

   \Drupal::service('module_installer')->install(['views']);
   

2. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -32,9 +32,9 @@ public function testDisplayConfigurable() {
   -    $assert->elementNotExists('css', '.field--name-name .field__item');
   -    $assert->elementNotExists('css', '.field--name-name .field__label');
   ...
   +    $assert->elementNotExists('css', '.views-row > div > div:nth-of-type(2) > div:nth-of-type(2)');
   +    $assert->elementNotExists('css', '.views-row > div > div:nth-of-type(2) > div:nth-of-type(1)');
   

   This feels fragile, but I can't imagine there is a better way.

   nth-of-type docs for anyone who needs them https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-of-type

   Seems the most reasonable way to target a specific sibling div.

Nice work on this!

I spent a good bit of time reviewing the last two tests closely and have some suggestions for making them less fragile.

1. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -165,9 +165,9 @@ public function testExposedFilter() {
   -    $this->assertSession()->elementsCount('xpath', '//div[@class="view-content"]//a', 2);
   -    $this->assertSession()->elementsCount('xpath', "//div[@class='view-content']//a[@href='{$node2->toUrl()->toString()}']", 1);
   -    $this->assertSession()->elementsCount('xpath', "//div[@class='view-content']//a[@href='{$node3->toUrl()->toString()}']", 1);
   +    $this->assertSession()->elementsCount('xpath', '//div[@class="views-row"]//a', 2);
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node2->toUrl()->toString()}']", 1);
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node3->toUrl()->toString()}']", 1);
   
   @@ -183,8 +183,8 @@ public function testExposedFilter() {
   -    $this->assertSession()->elementsCount('xpath', '//div[@class="view-content"]//a', 1);
   -    $this->assertSession()->elementsCount('xpath', "//div[@class='view-content']//a[@href='{$node1->toUrl()->toString()}']", 1);
   +    $this->assertSession()->elementsCount('xpath', '//div[@class="views-row"]//a', 1);
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node1->toUrl()->toString()}']", 1);
   
   @@ -195,10 +195,10 @@ public function testExposedFilter() {
   -    $this->assertSession()->elementsCount('xpath', '//div[@class="view-content"]//a', 3);
   -    $this->assertSession()->elementsCount('xpath', "//div[@class='view-content']//a[@href='{$node2->toUrl()->toString()}']", 1);
   -    $this->assertSession()->elementsCount('xpath', "//div[@class='view-content']//a[@href='{$node3->toUrl()->toString()}']", 1);
   -    $this->assertSession()->elementsCount('xpath', "//div[@class='view-content']//a[@href='{$node4->toUrl()->toString()}']", 1);
   +    $this->assertSession()->elementsCount('xpath', '//div[@class="views-row"]//a', 3);
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node2->toUrl()->toString()}']", 1);
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node3->toUrl()->toString()}']", 1);
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node4->toUrl()->toString()}']", 1);
   
   @@ -227,7 +227,7 @@ public function testExposedFilter() {
   -    $this->assertSession()->elementNotExists('xpath', "//div[@class='view-content']");
   +    $this->assertSession()->elementNotExists('xpath', "//div[@class='views-row']");
   

   So this is somewhat safe, because the test has both positive and negative assertions with the same xpath with different numbers of results.

   However, I think these assertion groups would be more robust if we were actually asserting that the nodes' labels are or aren't present as we expected in every case, so that it's not only dependent on the markup structure. This is especially important for the last assertion, which could easily lead to a false positive with a markup change. The trick to testing against markup is always to have both positive and negative assertions about the thing, and to test both the markup and the text related to it.

2. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -32,9 +32,9 @@ public function testDisplayConfigurable() {
   -    $assert->elementTextContains('css', 'h2 > a > .field--name-name', $this->term1->getName());
   -    $assert->elementNotExists('css', '.field--name-name .field__item');
   -    $assert->elementNotExists('css', '.field--name-name .field__label');
   +    $assert->elementTextContains('css', 'h2 > a > div', $this->term1->getName());
   +    $assert->elementNotExists('css', '.views-row > div > div:nth-of-type(2) > div:nth-of-type(2)');
   +    $assert->elementNotExists('css', '.views-row > div > div:nth-of-type(2) > div:nth-of-type(1)');
   
   @@ -48,8 +48,10 @@ public function testDisplayConfigurable() {
   -    $assert->elementTextContains('css', 'div.field--name-name > div.field__item', $this->term1->getName());
   -    $assert->elementExists('css', 'div.field--name-name > div.field__label');
   +    // Name field should be second field in view row. Value should be displayed.
   +    $assert->elementTextContains('css', '.views-row > div > div:nth-of-type(2) > div:nth-of-type(2)', $this->term1->getName());
   +    // Field label should be displayed.
   +    $assert->elementTextContains('css', '.views-row > div > div:nth-of-type(2) > div:nth-of-type(1)', 'Name');
   
   @@ -57,7 +59,7 @@ public function testDisplayConfigurable() {
   -    $assert->elementNotExists('css', 'div.field--name-name');
   +    $assert->elementNotExists('css', '.views-row > div > div:nth-of-type(2)');
   

   For this test, I don't think we're entirely testing the right things.

   • Initially, it should also assert the presence of both terms' labels and descriptions on the page, and assert that the text "Name" is not present.
   • Then, it should make the current assertions it does about ordering, but also assert the presence of both terms' descriptions (and the absence of the header tags, I guess).
   • Finally, it should assert the absence of both terms' names as well as the text "Name", in addition to the current assertion about the xpath, but also assert that both terms' descriptions are still present.

Thanks!

@danflanagan8, for issues like this where there's a lot of reliance on specific markup, I think adding more assertions (especially positive/negative pairs or text/xpath pairs) to ensure the test is sufficiently robust regardless of theme is in scope, because we're changing the default assumptions of the test.

However, when we have a potential "problem test" like these last two, it's also completely reasonable to split those off into separate issues. So one patch can contain all the tests where decoupling is straightforward, and followup issues can be added for tests that require additional assertions for forward-compatibility.

Thanks!

Before

% egrep -ir classy core/modules/taxonomy/tests
core/modules/taxonomy/tests/src/Functional/TermTranslationTest.php:  protected $defaultTheme = 'classy';
core/modules/taxonomy/tests/src/Functional/TermTest.php:  protected $defaultTheme = 'classy';
core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php:  protected $defaultTheme = 'classy';
core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php:  protected $defaultTheme = 'classy';

After

% egrep -ir classy core/modules/taxonomy/tests
%

Review

1. +++ b/core/modules/taxonomy/tests/src/Functional/TermTest.php
   @@ -645,7 +645,7 @@ public function testTermBreadcrumbs() {
        // Check the breadcrumb on the term edit page.
        $this->drupalGet('taxonomy/term/' . $term->id() . '/edit');
   -    $breadcrumbs = $this->getSession()->getPage()->findAll('css', 'nav.breadcrumb ol li a');
   +    $breadcrumbs = $this->getSession()->getPage()->findAll('css', 'nav[aria-labelledby="system-breadcrumb"] ol li a');
        $this->assertCount(2, $breadcrumbs, 'The breadcrumbs are present on the page.');
        $this->assertSame('Home', $breadcrumbs[0]->getText(), 'First breadcrumb text is Home');
        $this->assertSame($term->label(), $breadcrumbs[1]->getText(), 'Second breadcrumb text is term name on term edit page.');
   

   Can/should this now use Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait, instead?

2. +++ b/core/modules/taxonomy/tests/src/Functional/TermTest.php
   @@ -653,7 +653,7 @@ public function testTermBreadcrumbs() {
        // Check the breadcrumb on the term delete page.
        $this->drupalGet('taxonomy/term/' . $term->id() . '/delete');
   -    $breadcrumbs = $this->getSession()->getPage()->findAll('css', 'nav.breadcrumb ol li a');
   +    $breadcrumbs = $this->getSession()->getPage()->findAll('css', 'nav[aria-labelledby="system-breadcrumb"] ol li a');
        $this->assertCount(2, $breadcrumbs, 'The breadcrumbs are present on the page.');
        $this->assertSame('Home', $breadcrumbs[0]->getText(), 'First breadcrumb text is Home');
        $this->assertSame($term->label(), $breadcrumbs[1]->getText(), 'Second breadcrumb text is term name on term delete page.');
   

   And here?

3. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -169,9 +169,13 @@ public function testExposedFilter() {
   +    $this->assertSession()->elementsCount('xpath', '//div[@class="views-row"]//a', 2);
   +    $this->assertSession()->pageTextNotContains($node1->getTitle());
   

   I know we just did the elementsCount(), but would it be useful to add an explicit one of these here, too:

   $assert->linkByHrefNotExists($node1->toUrl()->toString());
   

   ?

4. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -169,9 +169,13 @@ public function testExposedFilter() {
   +    $this->assertSession()->pageTextContains($node2->getTitle());
   +    $this->assertSession()->elementsCount('xpath', "//div[@class='views-row']//a[@href='{$node2->toUrl()->toString()}']", 1);
   

   This definitely addresses @xjm's concern in #8.1.

   However, for the positive text assertions, would it not be better to assert the label is inside the link we just found? E.g. something more like this?

   $links = $this->assertSession()->findAll('xpath', "//div[@clelementsCountass='views-row']//a[@href='{$node2->toUrl()->toString()}']");
   $this->assertCount(1, $links);
   $this->assertEquals($node2->getTitle(), $links[0]->getText());
   

   This pattern repeats many times in the patch. If this (or something like it) is better, let's fix them everywhere.

5. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -169,9 +169,13 @@ public function testExposedFilter() {
   +    $this->assertSession()->pageTextNotContains($node4->getTitle());
   

   All the pageTextNotContains() in the patch are good as-is. I'm only talking about the new pageTextContains() references to be refactored to check the actual link text, not the whole page response.

   Although, any reference to pageTextNotContains() could be a candidate for another linkByHrefNotExists().

6. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -48,16 +55,39 @@ public function testDisplayConfigurable() {
   +    // Name field should be second field in view row. Value should be displayed
   

   Nit: "... should be the second field..."

7. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -48,16 +55,39 @@ public function testDisplayConfigurable() {
   +    $assert->elementTextContains('css', '.views-row:nth-child(1) > div > div:nth-of-type(1) p', $this->term1->getDescription());
   +    $assert->elementTextContains('css', '.views-row:nth-child(1) > div > div:nth-of-type(2) > div:nth-of-type(1)', 'Name');
   +    $assert->elementTextContains('css', '.views-row:nth-child(1) > div > div:nth-of-type(2) > div:nth-of-type(2)', $this->term1->getName());
   

   Wonder if these would be more readable as xpath. E.g. I find [2] in the right spot in an xpath generally more readable than :nth-of-type(2). Mostly a personal preference thing. Not worth changing just for this, but flagging it in case others agree.

I'm not going to NW for any of this, but not RTBC'ing, either. Curious to hear what others think of these points.

Thanks so much, @danflanagan8!

The new patch looks good to me, I agree that the xpath version is more readable.

Committed and pushed 435dc2011d to 10.0.x and 41b04e45ba to 9.4.x. Thanks!