Follow up to #2863267: Convert web tests of views

Convert \Drupal\views\Tests\ViewElementTest and \Drupal\views\Tests\Plugin\StyleGridTest to kernel tests. Lets see if we can tackle both in the same issue.

CommentFileSizeAuthor
#3 2876210-3.patch12.31 KBLendude
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude created an issue. See original summary.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Parent issue: #2876209: Convert \Drupal\views\Tests\ViewAjaxTest to a BrowserTestBase test » #2863267: Convert web tests of views
Related issues: +#2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Active » Needs review
FileSize
2876210-3.patch12.31 KB

\Drupal\views\Tests\Plugin\StyleGridTest contained one drupalGet to check if a css file was loaded. Looked at the original issue for that #2529748: Regression: Views module CSS is not attached and there is no need for that to be used on the grid style, any style will do for that check, so moved it to the summary test. Feels like a waste to set up a whole new browser test to just check one css file when we can get the same coverage in an existing test.

\Drupal\views\Tests\ViewElementTest was a wild mix of drupalGet and setRawContent. Split it down the middle. In the kernel test the HTML output is slightly different then in the browsertest so needed to modify some xpath queries to get the right result.

Patch is a little hard to read I think (with file move/copies/splits going on at the same time), but this is the most readable version I could roll I think.

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
Status: Needs review » Needs work

Feels like a waste to set up a whole new browser test to just check one css file when we can get the same coverage in an existing test.

I agree, it's fine to move it to this test.

Was one to really pay attention while reviewing :) direct patch review was kind-of difficult. Did it the old-fasioned side-by-side review way :)

Review comments:

  1. +++ b/core/modules/views/tests/src/Functional/ViewElementTest.php
@@ -28,25 +31,8 @@ protected function setUp() {
-    $xpath = $this->xpath('//div[@class="view-content"]');
-    $this->assertTrue($xpath, 'The view content has been found in the rendered output.');

    Seems we lost this check in the conversion? 

  2. +++ b/core/modules/views/tests/src/Functional/ViewElementTest.php
@@ -98,24 +76,8 @@ public function testViewElement() {
-    $xpath = $this->xpath('//div[@class="view-content"]');
-    $this->assertTrue($xpath, 'The view content has been found in the rendered output.');

    Seems to be lost as well? 

  3. +++ b/core/modules/views/tests/src/Kernel/ViewElementTest.php
@@ -153,12 +114,7 @@ public function testViewElementEmbed() {
+    $xpath = $this->xpath('//div[@class="views-row"]');

    Much better like this :)

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs work » Needs review

#4.1 and #4.2, it's not so much that we lost the test, it's that we lost the wrapper in the kernel test, so something that isn't loaded in the kernel test version seems to add that wrapper.

The test is still there in the new \Drupal\Tests\views\Functional\ViewElementTest::testViewElement so it has coverage.

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
Status: Needs review » Reviewed & tested by the community

I see and agree. As we didn't lose any test coverage lets go forward on this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 96c7b31 to 8.4.x and 5fca9cc to 8.3.x. Thanks!

  • alexpott committed 96c7b31 on 8.4.x 
    Issue #2876210 by Lendude, michielnugter: Convert \Drupal\views\Tests\...

  • alexpott committed 5fca9cc on 8.3.x 
    Issue #2876210 by Lendude, michielnugter: Convert \Drupal\views\Tests\...

  • alexpott committed 421b5f5 on 8.3.x 
    Revert "Issue #2876210 by Lendude, michielnugter: Convert \Drupal\views\...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

This can't go into 8.3.x cause #2863267: Convert web tests of views is not in 8.3.x

Status: Fixed » Closed (fixed)

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