Convert web tests of views

1. +++ b/core/modules/views/tests/src/Functional/Entity/FieldEntityTranslationTest.php
   @@ -164,8 +164,8 @@ protected function assertRows($expected = []) {
   -        'title' => (string) $row->xpath((new CssSelectorConverter())->toXPath('.views-field-title span.field-content a'))[0],
   -        'sticky' => (string) $row->xpath((new CssSelectorConverter())->toXPath('.views-field-sticky span.field-content'))[0],
   +        'title' => $row->find('xpath', (new CssSelectorConverter())->toXPath('.views-field-title span.field-content a'))->getText(),
   +        'sticky' => $row->find('xpath', (new CssSelectorConverter())->toXPath('.views-field-sticky span.field-content'))->getText(),
   

   I so like this $row->find() vs. $row->findAll()

2. +++ b/core/modules/views/tests/src/Functional/Handler/HandlerTest.php
   @@ -260,13 +259,13 @@ public function testRelationshipUI() {
   -    $xpath = $this->constructFieldXpath('name', $relationship_name);
   +    $xpath = $this->buildXPathQuery('//textarea[@name=:value]|//input[@name=:value]|//select[@name=:value]', [':value' => $relationship_name]);
        $fields = $this->xpath($xpath);
   

   I am wondering whether improving the selectors, like by registering an additional SelectorInterface would make this part unneeded.

\Drupal\Tests\views\Kernel\Handler\AreaEmptyTest was split from \Drupal\Tests\views\Functional\Handler\AreaTest because it needed to be a kernel test and that required some changes to the methods used for testing.

Nice!

\Drupal\views\Tests\ViewElementTest probably needs to be changed to a kernel test

Keep in mind that we can allow create followups to keep patches reviewable.

1. Removed addPageDisplay() from WTB ViewTestBase.
2. Wrapped to 80 characters (and another place in the file too.

Also removed 4 unused use statements from AreaEmptyTest.

There is also another unused use statement, this time in ExposedFormTest.

@Lendude your work is so good it's hard to find flaws (even in 107Kb)! :)

I took another stab at the review, found some things I have questions about. No no-go's so I'm not setting it back to needs work. I'll let you be the judge in that :)

1. +++ b/core/modules/views/src/Tests/Handler/HandlerTestBase.php
   @@ -1,11 +1,13 @@
    use Drupal\views\Tests\ViewTestBase;
   ...
   diff --git a/core/modules/views/src/Tests/Plugin/PluginTestBase.php b/core/modules/views/src/Tests/Plugin/PluginTestBase.php
   
   +++ b/core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
   @@ -213,7 +213,7 @@ protected function _testFilterDateUI() {
   @@ -221,22 +221,27 @@ protected function _testFilterDateUI() {
   
   @@ -221,22 +221,27 @@ protected function _testFilterDateUI() {
   +
   ...
   +    $this->drupalPostForm(NULL, ['created' => '1'], t('Apply'));
   

   If we're changing the label anyway could we also drop the t()?

2. +++ b/core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
   @@ -250,15 +255,15 @@ protected function _testFilterDateUI() {
   +    $this->assertEqual($results[0]->getText(), $this->nodes[3]->id());
   +    $this->drupalPostForm(NULL, ['created' => format_date(250000, 'custom', 'Y-m-d H:i:s')], t('Apply'));
   

   Same here.

3. +++ b/core/modules/views/tests/src/Functional/Handler/HandlerTest.php
   @@ -260,13 +259,13 @@ public function testRelationshipUI() {
   +    $xpath = $this->buildXPathQuery('//textarea[@name=:value]|//input[@name=:value]|//select[@name=:value]', [':value' => $relationship_name]);
   

   Is $page->findField() possible here or are you also searching for buttons?

4. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
   @@ -88,11 +88,11 @@ public function testPageDisplayMenu() {
   -    $element = $this->xpath('//ul[contains(@class, :ul_class)]//a[contains(@class, :a_class)]', [
   +    $element = $this->xpath('//ul[contains(@class, :ul_class)]//a[contains(@class, :a_class)]/child::text()', [
   

   Wow ok, nice change. Would never have guessed that one myself..

5. +++ b/core/modules/views/tests/src/Kernel/Plugin/DisplayPageTest.php
   @@ -149,4 +150,74 @@ public function testDependencies() {
   +    if (!isset($this->options['validate']['type'])) {
   +      return;
   +    }
   

   It seems weird without really knowing what's going on :)

   Is this ever going to fail?

6. +++ b/core/modules/views/tests/src/Kernel/Plugin/DisplayPageTest.php
   @@ -149,4 +150,74 @@ public function testDependencies() {
   +    $view = Views::getView('test_display_more');
   +    $this->executeView($view);
   +
   +    $output = $view->preview();
   +    $output = $renderer->renderRoot($output);
   +
   +    $this->setRawContent($output);
   

   This is used multiple times. Would be nice to have it on the base class some day..

7. +++ b/core/modules/views/tests/src/Kernel/Plugin/ExposedFormRenderTest.php
   @@ -0,0 +1,55 @@
   +    $this->setRawContent(\Drupal::service('renderer')->renderRoot($output));
   

   I'm not really against embedding the renderRoot() call here but previously you did it before the setRawContent. Any reason for the change?

1. +++ b/core/modules/views/tests/src/Functional/Handler/HandlerTest.php
   @@ -260,13 +259,13 @@ public function testRelationshipUI() {
        // Check for available options.
   -    $xpath = $this->constructFieldXpath('name', $relationship_name);
   +    $xpath = $this->buildXPathQuery('//textarea[@name=:value]|//input[@name=:value]|//select[@name=:value]', [':value' => $relationship_name]);
   

   looks like the method constructFieldXpath() is missing on our AssertLegacyTrait? I think we should open an issue to add that method and a little test in AssertLegacyTraitTest.

2. +++ b/core/modules/views/tests/src/Functional/Handler/HandlerTest.php
   @@ -260,13 +259,13 @@ public function testRelationshipUI() {
   -      $items = $this->getAllOptions($field);
   +      $items = $field->findAll('css', 'option');
   

   Nice change!! getAllOptions() accepts SimpleXML, so I think it does not make sense to add it on AssertLegacyTrait. This is probably the best way to convert this.

3. +++ b/core/modules/views/tests/src/Functional/Handler/HandlerTestBase.php
   @@ -0,0 +1,12 @@
   +/**
   + * Views handler test base.
   + */
   +abstract class HandlerTestBase extends ViewTestBase {
   +
   +}
   

   why does this class exist if it is just extending the parent class? Please add a comment.

4. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
   @@ -94,6 +93,7 @@ public function testDisplayPlugin() {
   +    $this->setExpectedException(\PHPUnit_Framework_Error::class);
        $output = $view->preview();
        $output = $renderer->renderRoot($output);
   

   this does not look right. We need to fix this test properly, it should not throw exceptions, right?

5. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
   @@ -290,7 +221,8 @@ public function testInvalidDisplayPlugins() {
   -    $this->assertBlockAppears($block);
   +    $result = $this->xpath('//div[@id = :id]', [':id' => 'block-' . $block->id()]);
   +    $this->assertEquals(1, count($result));
   

   So we need a separate issue to add assertBlockAppears to AssertLegacyTrait.

6. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
   @@ -265,7 +244,8 @@ public function testExposedBlock() {
   -    $this->assertFieldByXpath($xpath, $this->getExpectedExposedFormId($view), 'Expected form found in views block.');
   +    $result = $this->xpath($xpath);
   +    $this->assertEquals(1, count($result));
   

   why do we need to change this? Shouldn't this work with assertFieldByXpath()?

7. +++ b/core/modules/views/tests/src/Functional/Plugin/PluginTestBase.php
   @@ -0,0 +1,12 @@
   +/**
   + * Views plugin test base.
   + */
   +abstract class PluginTestBase extends ViewTestBase {
   +
   +}
   

   And another empty base class. I don't think we should introduce those. We can just tell people on the deprecated classes to use the new ViewTestBase instead?

8. +++ b/core/modules/views/tests/src/Functional/ViewTestBase.php
   @@ -0,0 +1,158 @@
   +abstract class ViewTestBase extends BrowserTestBase {
   

   gahh, git should detect this file as copy. Can you create the next patch with git diff --find-copies harder or -M5% or something?

9. +++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
   @@ -80,7 +80,7 @@ public function testViewsWizardAndListing() {
   -    $this->assertTrue(!empty($this->cssSelect('rss[version="2.0"]')));
   +    $this->assertEmpty($this->cssSelect('rss[version="2.0"]'));
   

   That change looks wrong. The assertion should be that the element is not empty?

1. +++ b/core/modules/views/tests/src/Functional/Handler/HandlerTest.php
   @@ -259,8 +260,7 @@ public function testRelationshipUI() {
   -    $xpath = $this->buildXPathQuery('//textarea[@name=:value]|//input[@name=:value]|//select[@name=:value]', [':value' => $relationship_name]);
   -    $fields = $this->xpath($xpath);
   +    $fields = $this->getSession()->getPage()->findAll('named', ['field', $relationship_name]);
   

   The implementation changed from an exact name to a partial name (default behavior or findAll('named'). I think you should use named_exact here.

2. +++ b/core/modules/views/tests/src/Kernel/Plugin/DisplayPageTest.php
   @@ -169,7 +166,7 @@ public function testReadMore() {
   -    $result = $this->xpath('//a[@class=:class]', [':class' => 'more-link']);
   +    $result = $this->xpath('//div[@class=:class]/a', [':class' => 'more-link']);
   

   It seems to me the classes are searched for on the div now, before the class was searched on the A tag right?

   Is it still correct like this and shouldn't we still search on the A tag to keep the test the same?

   Same goes for the other similar changes.

3. I conveniently moved the ViewUI wizard test into our scope, it's still missing right? I'll work on that soon and update your patch to include it :)

Update on 3.: I moved WizardTest out of this scope and into #2867777: Convert views WizardTestBase tests to phpunit

Passed after the last run. Looks like it's ready for RTBC to me!

1. +++ b/core/modules/views/tests/src/Functional/Handler/AreaHTTPStatusCodeTest.php
   @@ -10,7 +11,7 @@
   -class AreaHTTPStatusCodeTest extends HandlerTestBase {
   +class AreaHTTPStatusCodeTest extends ViewTestBase {
   

   I'm wondering whether it would be easier for some potential contrib modules to convert test coverage in case there would be such a parallel test, but well, the trigger_error clearly describes what is going on.

2. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
   @@ -94,6 +94,7 @@ public function testDisplayPlugin() {
        // Check the test option.
        $this->assertIdentical($view->display_handler->getOption('test_option'), '');
    
   +    $this->setExpectedException(\PHPUnit_Framework_Error::class);
        $output = $view->preview();
        $output = $renderer->renderRoot($output);
    
   @@ -103,6 +104,7 @@ public function testDisplayPlugin() {
   
   @@ -103,6 +104,7 @@ public function testDisplayPlugin() {
        $view->display_handler->overrideOption('test_option', 'Test option title');
        $view->save();
    
   +    $this->setExpectedException(\PHPUnit_Framework_Error::class);
   

   Are you sure setting multiple excepted exceptions works? #18.4 was not addressed. I guess this is not RTBC ...

Good one @dawehner, I misunderstood that one. And good that it uncovered that a part of the test didn't run.

Preventing the Exception in the first place sounds like a solid fix to me. Code looks good to me, I'm going for RTBC again. Hope I didn't mis anything this time :)

Looks good for me now!

Well you know, that's the thing. This entire RTBC thing is a fair assumption, but in reality you simply need to look at stuff from different angles / fresh eyes.

It is postponing a lot of issues in the conversion queue, setting to Major because of that.

Committed/pushed to 8.4.x, thanks!

Thanks a ton catch!!!!!!

A little bit of a follow-up: #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace

Cherry-picked to 8.3.x too, thanks!