Comments

jibran created an issue. See original summary.

jibran’s picture

Status: Active » Needs review
FileSize
28.99 KB

Only cache tests are remaining.

Status: Needs review » Needs work

The last submitted patch, 2: convert_all_aggregator-2757023-2.patch, failed testing.

klausi’s picture

  1. diff --git a/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
    deleted file mode 100644
    

    Can you create the patch with "git diff -M25%" so that we can see that this file has been moved?

  2. +++ b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    @@ -23,13 +23,13 @@ public function testAddFeed() {
    -    $this->assertResponse(200, 'Feed source exists.');
    -    $this->assertText($feed->label(), 'Page title');
    +    $this->assertResponse(200);
    +    $this->assertText($feed->label());
    

    Let's not change this lines if they don't break, only makes patch review harder and is out of scope for this conversion issue.

Looks like you are changing many lines just to remove the assertion messages. I think we should not do that here and just convert the test. A later cleanup can convert the assertions to proper $this->assertSession()->... calls that are not deprecated.

klausi’s picture

jibran’s picture

Status: Needs work » Needs review
FileSize
23.9 KB
15.71 KB
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    @@ -49,7 +49,7 @@ public function testAddFeed() {
    -  public function testFeedLabelEscaping() {
    +  public function _testFeedLabelEscaping() {
    

    So this method should be enabled again, right?

  2. +++ b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    @@ -60,7 +60,7 @@ public function testFeedLabelEscaping() {
    -    $this->assertTrue(strpos(str_replace(["\n", "\r"], '', $this->getRawContent()), 'class="feed-icon">  Subscribe to Test feed title &lt;script&gt;alert(123);&lt;/script&gt; feed</a>') !== FALSE);
    +    $this->assertTrue(strpos(str_replace(["\n", "\r"], '', $this->getTextContent()), 'class="feed-icon">  Subscribe to Test feed title &lt;script&gt;alert(123);&lt;/script&gt; feed</a>') !== FALSE);
    

    Looks like getRawContent() is missing on BrowserTestBase, can you add it there similar to getTextContent()?

  3. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    @@ -67,18 +67,18 @@ function testOverviewPage() {
    -    $this->assertEqual($feed->label(), (string) $result[0]->td[0]->a);
    +    $this->assertSession()->pageTextContains($feed->label());
    

    looks like this is changing the logic of the assertion. The new call should search in the specific table cell and link, right?

  4. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    @@ -67,18 +67,18 @@ function testOverviewPage() {
    -    $this->assertEqual(\Drupal::translation()->formatPlural($count, '1 item', '@count items'), (string) $result[0]->td[1]);
    +    $this->assertSession()->pageTextContains(\Drupal::translation()->formatPlural($count, '1 item', '@count items'));
    

    Same here, we should look in the table cell like the original assertion did.

    Also elsewhere.

  5. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorRenderingTest.php
    @@ -132,10 +132,12 @@ public function testFeedPage() {
    +    // @todo Find better way to handle this.
    +    // Can't do this see http://stackoverflow.com/a/17081317/1177774.
         $outline = $this->xpath('//outline[1]');
    -    $this->assertEqual($outline[0]['type'], 'rss', 'The correct type attribute is used for rss OPML.');
    -    $this->assertEqual($outline[0]['text'], $feed->label(), 'The correct text attribute is used for rss OPML.');
    -    $this->assertEqual($outline[0]['xmlurl'], $feed->getUrl(), 'The correct xmlUrl attribute is used for rss OPML.');
    +//    $this->assertEqual($outline[0]['type'], 'rss', 'The correct type attribute is used for rss OPML.');
    +//    $this->assertEqual($outline[0]['text'], $feed->label(), 'The correct text attribute is used for rss OPML.');
    +//    $this->assertEqual($outline[0]['xmlurl'], $feed->getUrl(), 'The correct xmlUrl attribute is used for rss OPML.');
    

    Ah, you are getting XML back, so you should probably use SimpleXml on the result of drupalGet() to navigate it.

jibran’s picture

Thank you for all your reviews. Please feel free to grab any of these. :-)

  1. It is failing and I was unable to figure it out so I commented it for this test run.
  2. Should we add it to AssertLegacyTrait.
  3. I don't how to switch between NodeElement and SimpleXml.
  4. I don't how to switch between NodeElement and SimpleXml.
  5. I don't like SimpleXml very much so didn't invest a lot of time in it. ;-)
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
15.18 KB
7.21 KB

OK, fixed all from #7.

@jibran, that failure (#8.1) is because Mink is getting the plain text from the browser in a different way. We were wrong in WebBaseTest. Mink thinks every time that the test is a human using a real browser. So, now assertText($test) converts the HTML entities in the character actually viewed by a human.

Let's say the the returned page contains this piece of markup: <p>Bad html &lt;script&gt;alert(123);&lt;/script&gt;</p>

In WebTestBase this is passing: $this->assertText('&lt;script&gt;alert(123);&lt;/script&gt;') because it wrongly consider that the text means only stripping tags.

In BrowserTestBase will fail but $this->assertText('<script>alert(123);</script>') will pass because Mink is putting in plain text the text as is displayed to a human, in browser. And that is < not &lt;.

dawehner’s picture

  1. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorRenderingTest.php
    @@ -132,10 +132,11 @@ public function testFeedPage() {
    +    $xml = simplexml_load_string($this->getRawContent());
    +    $attributes = $xml->xpath('//outline[1]')[0]->attributes();
    +    self::assertEquals($attributes->type, 'rss');
    +    self::assertEquals($attributes->text, $feed->label());
    +    self::assertEquals($attributes->xmlUrl, $feed->getUrl());
    

    Why can't we use $this->xpath instead? Also IMHO we should continue to use $this->assertEquals as we do in every other place in core.

  2. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
    @@ -61,7 +61,7 @@ protected function setUp() {
    -    $this->assertText(t('The feed @name has been added.', array('@name' => $edit['title[0][value]'])), format_string('The feed @name has been added.', array('@name' => $edit['title[0][value]'])));
    +    $this->assertText(t("The feed {$edit['title[0][value]']} has been added."), format_string('The feed @name has been added.', array('@name' => $edit['title[0][value]'])));
    

    Do we really have to change this line? In case we do, we should IMHO drop the t() statement.

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1580,6 +1594,44 @@ protected function getTextContent() {
    +
    +  /**
    +   * Checks for meta refresh tag and if found call drupalGet() recursively.
    +   *
    +   * This function looks for the http-equiv attribute to be set to "Refresh" and
    +   * is case-sensitive.
    +   *
    +   * @return string|false
    +   *   Either the new page content or FALSE.
    +   */
    +  protected function checkForMetaRefresh() {
    +    $refresh = $this->cssSelect('meta[http-equiv="Refresh"]');
    +    if (!empty($refresh) && (!isset($this->maximumMetaRefreshCount) || $this->metaRefreshCount < $this->maximumMetaRefreshCount)) {
    +      // Parse the content attribute of the meta tag for the format:
    +      // "[delay]: URL=[page_to_redirect_to]".
    +      if (preg_match('/\d+;\s*URL=(?<url>.*)/i', $refresh[0]->getAttribute('content'), $match)) {
    +        $this->metaRefreshCount++;
    +        return $this->drupalGet($this->getAbsoluteUrl(Html::decodeEntities($match['url'])));
    +      }
    +    }
    +    return FALSE;
    

    I'm wondering whether mink actually implements the same kind of logic internally already. It feels as if it would belong on that level

claudiu.cristea’s picture

Status: Needs review » Postponed

This I will postpone on #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3). There are several conversions that need that, so let's fix it in a dedicated issue.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jmuzz’s picture

+1 for checkForMetaRefresh().

It would make it possible to make functional tests on stuff that uses the batch API without requiring JavascriptTestBase and without adding the same function to every module that needs it.

It doesn't need to be recursive. A loop would use less memory.

I did some grepping around in /vendor and /core/lib/Drupal/Component for stuff like redirect and refresh and I don't think it is built into Mink.

Look at how Mahara does it: https://git.mahara.org/mahara/mahara/blob/16.04.3_RELEASE/htdocs/testing... in i_wait_to_be_redirected().

Similar example from Behat Drupal Extensions: https://www.drupal.org/node/2011390 .

claudiu.cristea’s picture

Status: Postponed » Needs review
FileSize
2.12 KB
14.38 KB

Un-postponing because #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3) is in.

@dawehner,

#10.1

Why can't we use $this->xpath instead?

Because $this->xpath() doesn't handle non-HTML pages. Here we have OPML so we need this workaround.

#10.3: I searched but no luck. But if you find something..

dawehner’s picture

Status: Needs review » Needs work
  1. diff --git a/core/modules/aggregator/src/Tests/AddFeedTest.php b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    similarity index 98%
    rename from core/modules/aggregator/src/Tests/AddFeedTest.php
    rename to core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    index b7ffe5f..90fff77 100644
    --- a/core/modules/aggregator/src/Tests/AddFeedTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Add feed test.
    ...
    diff --git a/core/modules/aggregator/src/Tests/AggregatorCronTest.php b/core/modules/aggregator/tests/src/Functional/AggregatorCronTest.php
    similarity index 97%
    rename from core/modules/aggregator/src/Tests/AggregatorCronTest.php
    rename to core/modules/aggregator/tests/src/Functional/AggregatorCronTest.php
    index 34a4cb9..ef8dc41 100644
    --- a/core/modules/aggregator/src/Tests/AggregatorCronTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/AggregatorCronTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Update feeds on cron.
    ...
    similarity index 98%
    rename from core/modules/aggregator/src/Tests/AggregatorTestBase.php
    rename to core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
    index 15be144..a6c9c32 100644
    --- a/core/modules/aggregator/src/Tests/AggregatorTestBase.php
    +++ b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
    @@ -1,16 +1,16 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     use Drupal\aggregator\Entity\Feed;
     use Drupal\Component\Utility\Html;
    -use Drupal\simpletest\WebTestBase;
    +use Drupal\Tests\BrowserTestBase;
     use Drupal\aggregator\FeedInterface;
     
     /**
      * Defines a base class for testing the Aggregator module.
      */
    -abstract class AggregatorTestBase extends WebTestBase {
    +abstract class AggregatorTestBase extends BrowserTestBase {
     
       /**
        * A user with permission to administer feeds and create content.
    diff --git a/core/modules/aggregator/src/Tests/DeleteFeedItemTest.php b/core/modules/aggregator/tests/src/Functional/DeleteFeedItemTest.php
    similarity index 96%
    rename from core/modules/aggregator/src/Tests/DeleteFeedItemTest.php
    rename to core/modules/aggregator/tests/src/Functional/DeleteFeedItemTest.php
    index 0261ed5..95202b8 100644
    --- a/core/modules/aggregator/src/Tests/DeleteFeedItemTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/DeleteFeedItemTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Delete feed items from a feed.
    diff --git a/core/modules/aggregator/src/Tests/DeleteFeedTest.php b/core/modules/aggregator/tests/src/Functional/DeleteFeedTest.php
    similarity index 97%
    rename from core/modules/aggregator/src/Tests/DeleteFeedTest.php
    rename to core/modules/aggregator/tests/src/Functional/DeleteFeedTest.php
    index 4cde81a..5bf5bd3 100644
    --- a/core/modules/aggregator/src/Tests/DeleteFeedTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/DeleteFeedTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Delete feed test.
    diff --git a/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php b/core/modules/aggregator/tests/src/Functional/FeedAdminDisplayTest.php
    similarity index 98%
    rename from core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
    rename to core/modules/aggregator/tests/src/Functional/FeedAdminDisplayTest.php
    index 0324e3b..9eb5cac 100644
    --- a/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/FeedAdminDisplayTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Tests the display of a feed on the Aggregator list page.
    diff --git a/core/modules/aggregator/src/Tests/FeedFetcherPluginTest.php b/core/modules/aggregator/tests/src/Functional/FeedFetcherPluginTest.php
    similarity index 95%
    rename from core/modules/aggregator/src/Tests/FeedFetcherPluginTest.php
    rename to core/modules/aggregator/tests/src/Functional/FeedFetcherPluginTest.php
    index 4aac851..1a03186 100644
    --- a/core/modules/aggregator/src/Tests/FeedFetcherPluginTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/FeedFetcherPluginTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Tests the fetcher plugins functionality and discoverability.
    diff --git a/core/modules/aggregator/src/Tests/FeedLanguageTest.php b/core/modules/aggregator/tests/src/Functional/FeedLanguageTest.php
    similarity index 98%
    rename from core/modules/aggregator/src/Tests/FeedLanguageTest.php
    rename to core/modules/aggregator/tests/src/Functional/FeedLanguageTest.php
    index af8335e..81f8b42 100644
    --- a/core/modules/aggregator/src/Tests/FeedLanguageTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/FeedLanguageTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     use Drupal\language\Entity\ConfigurableLanguage;
     
    diff --git a/core/modules/aggregator/src/Tests/FeedParserTest.php b/core/modules/aggregator/tests/src/Functional/FeedParserTest.php
    similarity index 99%
    rename from core/modules/aggregator/src/Tests/FeedParserTest.php
    rename to core/modules/aggregator/tests/src/Functional/FeedParserTest.php
    index 6c4a04d..00c7b70 100644
    --- a/core/modules/aggregator/src/Tests/FeedParserTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/FeedParserTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     use Drupal\Core\Url;
     use Drupal\aggregator\Entity\Feed;
    diff --git a/core/modules/aggregator/src/Tests/FeedProcessorPluginTest.php b/core/modules/aggregator/tests/src/Functional/FeedProcessorPluginTest.php
    similarity index 97%
    rename from core/modules/aggregator/src/Tests/FeedProcessorPluginTest.php
    rename to core/modules/aggregator/tests/src/Functional/FeedProcessorPluginTest.php
    index 8a7ade0..5527d95 100644
    --- a/core/modules/aggregator/src/Tests/FeedProcessorPluginTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/FeedProcessorPluginTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     use Drupal\aggregator\Entity\Feed;
     use Drupal\aggregator\Entity\Item;
    diff --git a/core/modules/aggregator/src/Tests/ImportOpmlTest.php b/core/modules/aggregator/tests/src/Functional/ImportOpmlTest.php
    similarity index 99%
    rename from core/modules/aggregator/src/Tests/ImportOpmlTest.php
    rename to core/modules/aggregator/tests/src/Functional/ImportOpmlTest.php
    index 1f1e90d..8994e3f 100644
    --- a/core/modules/aggregator/src/Tests/ImportOpmlTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/ImportOpmlTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Tests OPML import.
    diff --git a/core/modules/aggregator/src/Tests/UpdateFeedItemTest.php b/core/modules/aggregator/tests/src/Functional/UpdateFeedItemTest.php
    similarity index 98%
    rename from core/modules/aggregator/src/Tests/UpdateFeedItemTest.php
    rename to core/modules/aggregator/tests/src/Functional/UpdateFeedItemTest.php
    index ae78205..ba99dd0 100644
    --- a/core/modules/aggregator/src/Tests/UpdateFeedItemTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/UpdateFeedItemTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     use Drupal\aggregator\Entity\Feed;
     
     /**
    diff --git a/core/modules/aggregator/src/Tests/UpdateFeedTest.php b/core/modules/aggregator/tests/src/Functional/UpdateFeedTest.php
    similarity index 97%
    rename from core/modules/aggregator/src/Tests/UpdateFeedTest.php
    rename to core/modules/aggregator/tests/src/Functional/UpdateFeedTest.php
    index 8c797a5..bb830a5 100644
    --- a/core/modules/aggregator/src/Tests/UpdateFeedTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/UpdateFeedTest.php
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
    

    Those ones are part of the move issue, so let's skip that here.

  2. +++ b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
    similarity index 86%
    rename from core/modules/aggregator/src/Tests/AggregatorAdminTest.php
    
    rename from core/modules/aggregator/src/Tests/AggregatorAdminTest.php
    rename to core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    
    rename to core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    index 487e0ef..2a1bb2e 100644
    
    index 487e0ef..2a1bb2e 100644
    --- a/core/modules/aggregator/src/Tests/AggregatorAdminTest.php
    
    --- a/core/modules/aggregator/src/Tests/AggregatorAdminTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    
    +++ b/core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    +++ b/core/modules/aggregator/tests/src/Functional/AggregatorAdminTest.php
    @@ -1,6 +1,6 @@
    
    @@ -1,6 +1,6 @@
     <?php
     
    -namespace Drupal\aggregator\Tests;
    +namespace Drupal\Tests\aggregator\Functional;
     
     /**
      * Tests aggregator admin pages.
    @@ -67,18 +67,21 @@ function testOverviewPage() {
    
    @@ -67,18 +67,21 @@ function testOverviewPage() {
         // Check if the amount of feeds in the overview matches the amount created.
         $this->assertEqual(1, count($result), 'Created feed is found in the overview');
         // Check if the fields in the table match with what's expected.
    -    $this->assertEqual($feed->label(), (string) $result[0]->td[0]->a);
    +    $link = $this->xpath('//table/tbody/tr//td[1]/a');
    +    $this->assertEqual($feed->label(), $link[0]->getText());
         $count = $this->container->get('entity.manager')->getStorage('aggregator_item')->getItemCount($feed);
    -    $this->assertEqual(\Drupal::translation()->formatPlural($count, '1 item', '@count items'), (string) $result[0]->td[1]);
    +    $td = $this->xpath('//table/tbody/tr//td[2]');
    +    $this->assertEqual(\Drupal::translation()->formatPlural($count, '1 item', '@count items'), $td[0]->getText());
     
         // Update the items of the first feed.
         $feed->refreshItems();
         $this->drupalGet('admin/config/services/aggregator');
    -    $result = $this->xpath('//table/tbody/tr');
         // Check if the fields in the table match with what's expected.
    -    $this->assertEqual($feed->label(), (string) $result[0]->td[0]->a);
    +    $link = $this->xpath('//table/tbody/tr//td[1]/a');
    +    $this->assertEqual($feed->label(), $link[0]->getText());
         $count = $this->container->get('entity.manager')->getStorage('aggregator_item')->getItemCount($feed);
    -    $this->assertEqual(\Drupal::translation()->formatPlural($count, '1 item', '@count items'), (string) $result[0]->td[1]);
    +    $td = $this->xpath('//table/tbody/tr//td[2]');
    +    $this->assertEqual(\Drupal::translation()->formatPlural($count, '1 item', '@count items'), $td[0]->getText());
       }
     
     }
    

    At least those should be able to leverage #2809181: Provide forward compatibility layer for BrowserTestBase::xpath

  3. +++ b/core/modules/aggregator/tests/src/Functional/UpdateFeedTest.php
    diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php
    index f9f443c..a5e4535 100644
    
    index f9f443c..a5e4535 100644
    --- a/core/tests/Drupal/Tests/BrowserTestBase.php
    
    --- a/core/tests/Drupal/Tests/BrowserTestBase.php
    +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    
    +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -279,6 +279,20 @@
    
    @@ -279,6 +279,20 @@
       protected $preserveGlobalState = FALSE;
     
       /**
    +   * The number of meta refresh redirects to follow, or NULL if unlimited.
    +   *
    +   * @var null|int
    +   */
    +  protected $maximumMetaRefreshCount = NULL;
    

    Do we have a dedicated issue for those changes? These should be for example tested separately

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
22.6 KB

Re-rolled.

klausi’s picture

Status: Needs review » Needs work
  1. +++ /dev/null
    @@ -1,382 +0,0 @@
    - * @deprecated Scheduled for removal in Drupal 9.0.0.
    - *   Use \Drupal\Tests\aggregator\Functional\AggregatorTestBase instead.
    - */
    -abstract class AggregatorTestBase extends WebTestBase {
    

    This is an old base class and must not be removed. It is depreacted but it must stay. Sorry for not telling you earlier :)

  2. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorRenderingTest.php
    @@ -132,10 +132,11 @@ public function testFeedPage() {
    +    $xml = simplexml_load_string($this->getRawContent());
    

    This should have a comment why we have to use simplexml_load_string() here. Example: // We can't use Mink XPAth queries here because it only supports HTML pages, but we are dealing with XML here.

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1240,6 +1254,44 @@ protected function getTextContent() {
       /**
    +   * Retrieves the raw content from the current page.
    +   */
    +  protected function getRawContent() {
    

    not sure if we should add this method in this issue or create a new one for that ...

  4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1240,6 +1254,44 @@ protected function getTextContent() {
    +  /**
    +   * Runs cron.
    +   */
    +  protected function cronRun() {
    

    we decided to not add this method to BrowserTestBase. Use the CronRunTrait for test cases that need cron instead.

  5. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1240,6 +1254,44 @@ protected function getTextContent() {
    +  /**
    +   * Checks for meta refresh tag and if found call drupalGet() recursively.
    +   *
    +   * This function looks for the http-equiv attribute to be set to "Refresh" and
    +   * is case-sensitive.
    +   *
    +   * @return string|false
    +   *   Either the new page content or FALSE.
    +   */
    +  protected function checkForMetaRefresh() {
    

    this method is only used by 2 tests in Drupal core. Let's not add it on BrowserTestBase, let's create a new issue to have a trait with that method instead that can be used by Simpletest's WebTestBase and the converted tests.

claudiu.cristea’s picture

Status: Needs work » Postponed
FileSize
7.62 KB
8.74 KB
claudiu.cristea’s picture

Status: Postponed » Active

Hm. I think we should move the trait creation back here for the reasons I mentioned in #2855942-5: Create ::checkForMetaRefresh() on BrowserTestBase. What you think, @klausi?

klausi’s picture

Yes, checkForMetaRefresh() is only used by aggregator and big_pipe, so I think is fine to do here.

GoZ’s picture

Assigned: Unassigned » GoZ
Issue tags: +DevDaysSeville
GoZ’s picture

Assigned: GoZ » Unassigned
Status: Active » Needs review
FileSize
2.79 KB
12.3 KB
klausi’s picture

Status: Needs review » Postponed
+++ b/core/modules/aggregator/tests/src/Functional/AddFeedTest.php
@@ -60,7 +64,7 @@ public function testFeedLabelEscaping() {
-    $this->assertTrue(strpos(str_replace(["\n", "\r"], '', $this->getRawContent()), 'class="feed-icon">  Subscribe to Test feed title &lt;script&gt;alert(123);&lt;/script&gt; feed</a>') !== FALSE);
+    $this->assertTrue(strpos(str_replace(["\n", "\r"], '', $this->getSession()->getPage()->getContent()), 'class="feed-icon">  Subscribe to Test feed title &lt;script&gt;alert(123);&lt;/script&gt; feed</a>') !== FALSE);

We shouldn't change this line, we should have the getRawContent() method on BrowserTestBase or AssertLegacyTrait. The original getRawContent() method is in AssertContentTrait, so I vote to implement it in AssertLegacyTrait. We should open an issue for that and add test coverage for it to BrowserTestBaseTest.

Postponing on that, please open an issue and link it here.

GoZ’s picture

Issue already exists. getRawContent() should be discussed on #2795111: WTB to BTB, add getRawContent() in BTB

dawehner’s picture

Status: Postponed » Needs work

This issue can be worked on again ...

GoZ’s picture

Status: Needs work » Needs review
FileSize
11.7 KB
842 bytes
dawehner’s picture

Issue summary: View changes
Status: Needs review » Postponed
dawehner’s picture

Title: Convert all aggregator web tests to BrowserTestBase » [pp-1] Convert all aggregator web tests to BrowserTestBase
michielnugter’s picture

Issue tags: +phpunit initiative
naveenvalecha’s picture

Title: [pp-1] Convert all aggregator web tests to BrowserTestBase » Convert all aggregator web tests to BrowserTestBase
Status: Postponed » Needs review
FileSize
17.72 KB

This landed #2862885: Batch: Convert system functional tests to phpunit and it unblocked this issue.
//Naveen

Status: Needs review » Needs work

The last submitted patch, 31: 2757023-31.patch, failed testing. View results

naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha

Assigning to myself for fixing the failures.

//Naveen

naveenvalecha’s picture

I have added two patches

2757023-34-2.patch: This is the straightforward conversion of the WTB to BTB
2757023-34-remove-AggregatorTestBase-class.patch: This patch removes the "src/Tests/AggregatorTestBase" Class from the codebase as no other test is using it. What're the thoughts on removing the deprecated Base class? However, tests are not the API as per the BC policy.

//Naveen

dawehner’s picture

I think the discussion to remove the base class should be its own issue. Its one less thing to deal with for reviewers/committers ...

naveenvalecha’s picture

#35: Filed a follow-up #2886547: Add trigger_error to deprecated AggregatorTestBase class
2757023-34-2.patch is ready for review. Is it RTBC?

//Naveen

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'd say so

  • catch committed f62b4c7 on 8.4.x
    Issue #2757023 by claudiu.cristea, naveenvalecha, GoZ, jibran, Jo...

  • catch committed b42403e on 8.3.x
    Issue #2757023 by claudiu.cristea, naveenvalecha, GoZ, jibran, Jo...
catch’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for the separate deprecation issue, makes this a much easier commit for the substantive change, then that one is just yes or no eventually.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!