See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Proposed followups:

Notes:

  • \Drupal\views\Tests\ViewTestData not sure if we want to move this, so leaving it alone for now.
  • Updated \Drupal\Tests\views\Functional\ViewTestBase::helperButtonHasLabel to work with BTB (was using field to find a button, that doesn't work)
  • \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.
  • \Drupal\Tests\views\Kernel\Plugin\ExposedFormRenderTest split of from \Drupal\Tests\views\Functional\Plugin\ExposedFormTest because it needed to be a kernel test
  • \Drupal\Tests\views\Kernel\Plugin\DisplayPageTest::testReadMore split from \Drupal\Tests\views\Functional\Plugin\DisplayTest and turned into a kernel test and added to \Drupal\Tests\views\Kernel\Plugin\DisplayPageTest (that sounded like the right place to put it)
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Lendude created an issue. See original summary.

Lendude’s picture

Lendude’s picture

Status: Active » Needs review
FileSize
64.07 KB

First roll, it's a work in progress.

Some points:

  • All the /Plugin tests still need to be moved.
  • \Drupal\views\Tests\ViewAjaxTest probably needs to be moved to a javascript test
  • \Drupal\views\Tests\FieldApiDataTest needs FieldTestBase
  • \Drupal\views\Tests\ViewTestData not sure if we want to move this, so leaving it alone for now.

Status: Needs review » Needs work

The last submitted patch, 3: 2863267-3.patch, failed testing.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
16.62 KB
59.64 KB

Clean up to hopefully get this green. Still only a partial conversion and still very much a work in progress.

Some more points:

  • \Drupal\views\Tests\ViewElementTest probably needs to be changed to a kernel test
  • \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.
dawehner’s picture

  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.

Lendude’s picture

Assigned: Lendude » Unassigned
Issue summary: View changes
FileSize
41.6 KB
107.8 KB

Thanks @dawehner for the feedback.

Converted everything I think doesn't need a follow up, locally green, lets see how it does here.

If we agree on the proposed follow ups in the IS, then this would be ready for a full review.

Notes:

  • \Drupal\Tests\views\Kernel\Plugin\ExposedFormRenderTest split of from \Drupal\Tests\views\Functional\Plugin\ExposedFormTest because it needed to be a kernel test
  • Updated \Drupal\Tests\views\Functional\ViewTestBase::helperButtonHasLabel to work with BTB (was using field to find a button, that doesn't work)
  • \Drupal\views\Tests\Plugin\StyleOpmlTest weird stuff happens when not working with HTML (or something, didn't dig deep)
  • \Drupal\Tests\views\Kernel\Plugin\DisplayPageTest::testReadMore split from \Drupal\Tests\views\Functional\Plugin\DisplayTest and turned into a kernel test and added to \Drupal\Tests\views\Kernel\Plugin\DisplayPageTest (that sounded like the right place to put it)

Nice to know: When casting NodeElement to string using (string) it will just give a 'can't cast to sting' error, but when using the raw NodeElement in an assertion it will throw a 'can't serialise Closure' exception when running the test with no indication where that happens (and no closure anywhere in the test). Knowing that might help people when converting, threw me a bit at first :)

Lendude’s picture

Quick Self review:

  1. +++ b/core/modules/views/src/Tests/ViewTestBase.php
    @@ -146,4 +150,10 @@ protected function dataSet() {
    +  protected function addPageDisplay($view_name, $path) {
    +    $this->drupalPostForm('admin/structure/views/view/test_filter_date_between/edit', [], 'Add Page');
    +    $this->drupalPostForm('admin/structure/views/nojs/display/test_filter_date_between/page_1/path', ['path' => $path], 'Apply');
    +    $this->drupalPostForm(NULL, [], t('Save'));
    +  }
    

    This needs to go. Might be nice to add at some point to the BTB version of ViewTestBase but definitely not on the WTB version

  2. +++ b/core/modules/views/tests/src/Functional/GlossaryTest.php
    @@ -113,8 +114,8 @@ public function testGlossaryView() {
    +      // The rendered output looks like "<a href=''>X</a> | (count)" so let's figure out the int.
    

    wrap at 80 characters

Jo Fitzgerald’s picture

  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.

The last submitted patch, 7: 2863267-7.patch, failed testing.

Lendude’s picture

\Drupal\views\Tests\Plugin\DisplayFeedTest is also not using HTML output and the browsertestbase is not playing nice with that, so taking it out of the conversion for now and adding it to the follow ups

The last submitted patch, 9: 2863267-9.patch, failed testing.

Lendude’s picture

And now with the right patch.....

Jo Fitzgerald’s picture

Status: Needs review » Needs work

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

Lendude’s picture

@Jo Fitzgerald a 107Kb patch and that's all you got for me :D, thanks for taking a look at it!

Jo Fitzgerald’s picture

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

michielnugter’s picture

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?

klausi’s picture

Status: Needs review » Needs work
  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?

Lendude’s picture

Assigned: Unassigned » Lendude

Thanks so much for the feedback both!

@michielnugter those sound like valid points, will look at #17.5, no idea what's going on there.

@klausi some quick feedback on your feedback
#18.1 yeah it's missing, but it seemed like a silly helper method anyway, so that's why i figured I'd just move that logic straight into the test.
#18.3 and #18.7 The empty classes existed before so I went for the 1:1 port, but just leaving them out and extending ViewTestBase directly works for me.
#18.4 It's a debug statement actually, tim.plunkett already opened an issue on this #2853359: Runtime debug statement in Views now prints out object, but for now, yeah this is the expected behaviour, so the test is accurate

will get to work on the rest!

Lendude’s picture

Assigned: Lendude » Unassigned
Status: Needs work » Needs review
FileSize
33.74 KB
108.75 KB

Addressed feedback:

  • #17.1/2 sure, lots of t() in that test but lets get rid of it if it doesn't make the change bigger.
  • #17.3 findField only finds the first one, and we need them all, but that did point me to using $this->getSession()->getPage()->findAll('named', ['field', $relationship_name]); to get rid of the xpath query.
  • #17.5 that was copy/paste from the original test....but it actually stops the test from running! It wasn't green at all and probably wasn't in the Webtest version either. Nice catch!
  • #17.7 That was like that in the original test, just trying to keep the changes to a minimum
  • #18.1 Not using xpath query anymore now, we still might want to open a follow up, but I think we don't need to postpone this on the follow up
  • #18.3/#18.7 Removed the empty classes and updated the deprecated messages
  • #18.5 Created #2865336: Add \Drupal\simpletest\WebTestBase::assertBlockAppears to \Drupal\FunctionalTests\AssertLegacyTrait, not sure if we want to postpone on that, fairly trivial reformatting of the assertion as it stands now
  • #18.6 No assertFieldByXpath() doesn’t work because we are looking for a form element not a field, so assertFieldByXpath() would look for a ‘value’ attribute on the form element and fatal because it doesn’t have that attribute
  • #18.9 Uhh yeah, don’t know what I was doing there. What I was trying to fix was problems with an XML-response where everything expects a HTML tag. So this should be better.

Lets see what I broke....

Status: Needs review » Needs work

The last submitted patch, 20: 2863267-20.patch, failed testing.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.38 KB
108.8 KB

One use statement missing, not sure why interdiff is showing more changes.

michielnugter’s picture

Status: Needs review » Needs work
  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 :)
michielnugter’s picture

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
765 bytes
108.8 KB

#23.1 fixed
#23.2 that was the test with the return at the top, so that was never run in the old webtestbase version (manually tested this and it's indeed never run, and fails if you take the return out)
#23.3 covered by your follow up

Status: Needs review » Needs work

The last submitted patch, 25: 2863267-25.patch, failed testing.

michielnugter’s picture

Status: Needs work » Reviewed & tested by the community

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

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
  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 ...

Lendude’s picture

Status: Needs work » Needs review
FileSize
5.52 KB
109.37 KB

Thanks @dawehner

#28.2 yeah sorry, misunderstood how setExpectedException() works. So I added a properly configured style there to not trigger the debug call. Also added some more fixes to that test further along because that was never run because of the setExpectedException

interdiff shows extra stuff again, only made changes to DisplayTest()

michielnugter’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

dawehner’s picture

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.

michielnugter’s picture

Priority: Normal » Major

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

michielnugter’s picture

Issue tags: +phpunit initiative
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

  • catch committed 6b6ddee on 8.4.x
    Issue #2863267 by Lendude, Jo Fitzgerald, michielnugter, dawehner,...
dawehner’s picture

Thanks a ton catch!!!!!!

Mile23’s picture

Lendude’s picture

Issue summary: View changes

Added followups to the IS

Status: Fixed » Closed (fixed)

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

catch’s picture

Version: 8.4.x-dev » 8.3.x-dev

Cherry-picked to 8.3.x too, thanks!

Lendude’s picture

@catch thanks!