Convert web tests to browser tests for forum module

1. +++ b/core/modules/forum/tests/src/Functional/ForumBlockTest.php
   @@ -51,29 +64,32 @@ public function testNewForumTopicsBlock() {
   -    $this->assertLink(t('More'), 0, 'New forum topics block has a "more"-link.');
   -    $this->assertLinkByHref('forum', 0, 'New forum topics block has a "more"-link.');
   +    $this->assertNotEmpty($page->findLink(t('More')), 'New forum topics block has a "more"-link.');
   +    $this->assertNotEmpty($page->find('xpath', '//a[contains(@href, forum)]'), 'New forum topics block has a "more"-link.');
   ...
   -      $this->assertLink($topic, 0, format_string('Forum topic @topic found in the "New forum topics" block.', array('@topic' => $topic)));
   +      $this->assertNotEmpty($page->findLink($topic), sprintf('Forum topic %s found in the "New forum topics" block.', $topic));
   

   Note: the other issue adds assertLink or something similar

2. +++ b/core/modules/forum/tests/src/Functional/ForumBlockTest.php
   @@ -88,12 +104,13 @@ public function testActiveForumTopicsBlock() {
   +    /** @var \DateTime|\Drupal\Core\Datetime\DrupalDateTime $date */
        $date = new DrupalDateTime();
   

   This is confusing, can't phpstorm figure that out on its own?

3. +++ b/core/modules/forum/tests/src/Functional/ForumBlockTest.php
   @@ -101,42 +118,47 @@ public function testActiveForumTopicsBlock() {
   -    $block->getPlugin()->setConfigurationValue('block_count', 2);
   -    $block->save();
   +    $this->drupalGet($block->toUrl());
   +    $this->submitForm([
   +      'settings[block_count]' => 2,
   +    ], t('Save block'));
   

   I'm wondering the reason to change that. I agree that using APIs in tests is potentially problematic, so is that the reason to change it?

4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1529,4 +1534,42 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
    
   +  /**
   +   * Asserts whether an expected cache tag was present in the last response.
   +   *
   +   * @param string $expected_cache_tag
   +   *   The expected cache tag.
   +   */
   +  protected function assertCacheTag($expected_cache_tag) {
   +    $cache_tags = explode(' ', $this->getSession()->getResponseHeader('X-Drupal-Cache-Tags'));
   +    $this->assertTrue(in_array($expected_cache_tag, $cache_tags), "'" . $expected_cache_tag . "' is present in the X-Drupal-Cache-Tags header.");
   +  }
   

   Let's ensure to add this to the webassert

@larowlan: I can't reproduce the problems with the module install page in ForumUninstallTest. Attached is a conversion that works for me, based on #2735045: Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert.

My patch is incomplete because it converts only the uninstall test.

Borrowed some helper methods from #2750941: Additional BC assertions from WebTestBase to BrowserTestBase.

Converted 2 more forum tests, currently failing at asserting cache tags in HTTP headers.

Cacheability headers for debugging were not set in BrowserTestBase.

Still a couple of old simpletests to go.

Converted all but one test - the Views test, which depends on #2747167: Convert first part of web tests of views_ui.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1076,11 +1078,14 @@ public function installDrupal() {
+
     $settings_services_file = DRUPAL_ROOT . '/' . $this->originalSiteDirectory . '/testing.services.yml';
-    if (file_exists($settings_services_file)) {
-      // Copy the testing-specific service overrides in place.
-      copy($settings_services_file, $directory . '/services.yml');
+    if (!file_exists($settings_services_file)) {
+      // Otherwise, use the default services as a starting point for overrides.
+      $settings_services_file = DRUPAL_ROOT . '/sites/default/default.services.yml';
     }
+    // Copy the testing-specific service overrides in place.
+    copy($settings_services_file, $directory . '/services.yml');

Just checked, this is synching code from WebTestBase with BrowserTestBase. Perfect.

1. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -215,6 +215,52 @@ public function linkExists($label, $index = 0, $message = '') {
   +    // Cast MarkupInterface objects to string.
   +    $label = (string) $label;
   

   So let's skip using the casting?

2. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -215,6 +215,52 @@ public function linkExists($label, $index = 0, $message = '') {
   +    $this->assert(null === $node, sprintf('A link "%s" appears on this page, but it should not.', $label));
   

   Nitick: NULL :)

We should probably spin out the BrowserTestBase fix into its own issue: #2751875: BrowserTestBase must enable cache tags in responses

Rerolled, merged in #2750941: Additional BC assertions from WebTestBase to BrowserTestBase (see that issue for interdiff).

Yes, the "6 6" is right here, added a comment.

The Forum Views test is still missing, but we can skip that until #2747167: Convert first part of web tests of views_ui is done.

+++ b/core/modules/forum/tests/src/Functional/ForumTest.php
@@ -322,8 +322,8 @@ private function doAdminTests($user) {
+    $this->assertSession()->buttonExists('Save');

Should we in general try to reuse existing instances of the assertSession objects?

I think that is up to the taste of the patch author. I'm using $this->assertSession() always in this patch to have a better diff what lines have been replaced with what.

I think that is up to the taste of the patch author. I'm using $this->assertSession() always in this patch to have a better diff what lines have been replaced with what.

Fair point!

Committed and pushed 57432ad42187bfce6e2f82709ee6a7e979e626fb to 8.2.x and 3e86a03 to 8.1.x. Thanks!

Thanks for this first step, this is now postponed on #2747167: Convert first part of web tests of views_ui for the remaining Views integration test.

@klausi we don't reuse issues because that messes up credit and reporting etc. We just can have another issue.

Unfortunately I've had to revert this conversion. It introduced a high rate of random failures for this test group on Postgres, especially on 8.2.x and PHP 7 (although the fails also occur with Postgres on 8.1.x and on PHP 5.5 at a lower rate). See #2776269: High random fail rate in BTB forum tests on Postgres (especially, but not only, with PHP7).

The fails do not seem to occur when the test group is run by itself, only when the full test suite is run together. This suggests that the bug may not actually be introduced by these tests themselves, but that they are exposing an existing bug in the BTB test runner and/or with DrupalCI. However, the very high random fail rate will make it virtually impossible to catch any actual Postgres regression from another patch. Since this patch is not fixing any bug, we'll unfortunately need to block or postpone this conversion on resolving the underlying issue.

The test results on #2776269: High random fail rate in BTB forum tests on Postgres (especially, but not only, with PHP7) can be a starting point. Hopefully someone with more experience with BTB, Postgres, or DrupalCI can debug further to narrow down what the actual bug is.

Postponing explicitly on that, although we should see if we can move this into larger, conceptually scoped conversions.

We figured out the reason, let's get back on track, see #2776269: High random fail rate in BTB forum tests on Postgres (especially, but not only, with PHP7)

Back to RTBC

We need to wait for #2771547: In Browser and FunctionalJavascript tests SIMPLETEST_USER_AGENT cookie needs to be set every 5 seconds, otherwise we will get random fails on Postgres again.

That one got committed

#2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) has not decided the plan yet.

unpostponing, see #2735005-131: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Random test fail, back to needs review.

This issue should try to leverage the forward compatibility layer #2809181: Provide forward compatibility layer for BrowserTestBase::xpath

This needs a reroll.

Re-roll.

It looks like you forgot to move the files to the tests directory.

Sorry, I'm obviously not concentrating enough today!

Here's the re-roll with the files moved.

+++ b/core/modules/forum/tests/src/Functional/ForumNodeAccessTest.php
@@ -2,8 +2,8 @@
-use Drupal\Tests\BrowserTestBase;
 use Drupal\node\Entity\NodeType;
+use Drupal\Tests\BrowserTestBase;

unrelated change that is not needed. This file has already been moved.

Otherwise looks good to me. The only remaining old test in forum module is for Views, which is blocked on #2747167: Convert first part of web tests of views_ui and we decided to convert that one later.

Change mentioned in #53 reverted.

Reviewed, this looks good now.

Committed and pushed 7c5c3fd to 8.4.x and 9d97196 to 8.3.x. Thanks!

Let's hope we truly have fixed the random errors that this original conversion caused. Committed to 8.3.x too to keep tests inline.