Convert web tests of block

@zviryatko, @Jo Fitzgerald:

Thanks for the start on this issue!

Aside from the CI aborting the following tests failed:

06:50:43 Drupal\Tests\block\Functional\BlockSystemBrandingTest 0 passes 1 fails
06:51:42 Drupal\Tests\block\Functional\BlockCacheTest 0 passes 1 fails

Code review:

+++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestAccessBlock.php
@@ -20,6 +20,8 @@
+  protected $state;
+

Please add the correct documentation indicating the type of variable.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -720,6 +720,19 @@ protected function assertCacheTag($expected_cache_tag) {
+   * Asserts whether an expected cache tag was absent in the last response.
+   *
+   * @param string $cache_tag
+   *   The expected cache tag.
+   *
+   * @deprecated Scheduled for removal in Drupal 9.0.0.
+   *   Use $this->assertSession()->responseHeaderNotContains() instead.
+   */
+  protected function assertNoCacheTag($cache_tag) {
+    $this->assertSession()->responseHeaderNotContains('X-Drupal-Cache-Tags', $cache_tag);
+  }
+
+  /**

Let's wait untill #2795085: Add assertNoCacheTag to assertLegacyTrait lands and not add the method here.

It's ok to keep it here to get the tests to pass but please note it when posting a patch so everyone know's the intent is to leave it out :)

@michielnugter, also I met problem when I extend BrowserTestBase two another test cases fails, but when extend JavascriptTestBase it's ok. That fails is related to block cache and \Drupal::state(), since block plugins from block_test module is depend on \Drupal::state() object, but for BrowserTestBase probably entire page is cached, but still not understand why for JavascriptTestBase it's ok.

Another problem that I met related to cache context, when BlockRepository gets all block per region and check access, the Access object has own cache tags/context/max-age, so BlockCacheTest fails because blockAccess not respect the block cache settings. I can't describe more now, all data on my home pc, I can provide you more info later if needed.

@zviryatko

also I met problem when I extend BrowserTestBase two another test cases fails, but when extend JavascriptTestBase it's ok. That fails is related to block cache and \Drupal::state(), since block plugins from block_test module is depend on \Drupal::state() object, but for BrowserTestBase probably entire page is cached, but still not understand why for JavascriptTestBase it's ok.

Not sure either why it would work with the conversion, maybe it has something to do with JavascriptTestBase using an actual browser instead of a simulated one. Let me know which tests and I might be able to help you out.

Another problem that I met related to cache context, when BlockRepository gets all block per region and check access, the Access object has own cache tags/context/max-age, so BlockCacheTest fails because blockAccess not respect the block cache settings. I can't describe more now, all data on my home pc, I can provide you more info later if needed.

Not sure on that one, I'm not very deeply familiar with cache tags yet. Let me know which tests are failing on this and I might be able to figure it out, always fun to dive deep into Drupal core :)

Adding #2795085: Add assertNoCacheTag to assertLegacyTrait as a blocker for this issue.

//Naveen

This is in, yeah!

Tests aren't passing right now ...

For now just getting the patch up to speed, removing AssertLegacyTrait::assertNoCacheTag() from this patch, as it was already added in #2795085: Add assertNoCacheTag to assertLegacyTrait

Took care of a fail, don't know why CI times out, something in #18 is hanging it.

Deprecated/copied \Drupal\block\Tests\BlockTestBase instead of moving.
Added a CR, https://www.drupal.org/node/2901823

Moved the Views test.

This will fail because of drupalPostWithFormat

Fixed the fail in \Drupal\Tests\block\Functional\Views\DisplayBlockTest::testBlockContextualLinks by using mink to do the post request.

Created \Drupal\Tests\block\Functional\AssertBlockAppearsTrait to hold the block appears methods, no idea what they were doing on WebTestBase but pretty sure we don't want them on BrowserTestBase and they are used very little in core, so not worth moving to AssertLegacyTrait IMO.

Thanks @Lendude, I couldnt figure out #18 either...

+++ b/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php
@@ -0,0 +1,49 @@
\ No newline at end of file

nitpick, fixable on commit

AssertBlockAppearsTrait seems like a good idea to me, I think #22 is good to go :)

1. +++ b/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php
   @@ -0,0 +1,49 @@
   +    $this->assertTrue(!empty($result), format_string('Ensure the block @id appears on the page', ['@id' => $block->id()]));
   ...
   +    $this->assertFalse(!empty($result), format_string('Ensure the block @id does not appear on the page', ['@id' => $block->id()]));
   

   What about avoiding format_string here and go with FormattableMarkup instead?

2. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
   @@ -261,7 +261,7 @@ public function testBlockThemeSelector() {
   -      $this->assertTrue(!empty($elements), 'The block was found.');
   +      $this->assertNotEmpty($elements, 'The block was found.');
   
   +++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
   @@ -99,13 +99,13 @@ public function testBlockAdminUiPage() {
   -    $this->assertTrue(!empty($blocks_table), 'The blocks table is being rendered.');
   +    $this->assertNotEmpty($blocks_table, 'The blocks table is being rendered.');
   

   While these changes are of course nice to do, I am wondering whether we should rather target a smaller patch size instead. The previous assertions still pass.

Thanks for the feedback both!

Fixes #23 and #24

Thank you for reducing the patch size!

For completeness' sake, linking to a similar issue: #2865336: Add \Drupal\simpletest\WebTestBase::assertBlockAppears to \Drupal\FunctionalTests\AssertLegacyTrait

This is good. Adding assertBlockAppears and assertNoBlockAppears to a Trait in the Block module means that #2865336: Add \Drupal\simpletest\WebTestBase::assertBlockAppears to \Drupal\FunctionalTests\AssertLegacyTrait can be ditched, and #2864088: Convert web tests to browser tests for shortcut module can use the new Block trait.

+++ b/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php
@@ -0,0 +1,50 @@
+    $this->assertTrue(!empty($result), new FormattableMarkup('Ensure the block @id appears on the page', ['@id' => $block->id()]));
...
+    $this->assertFalse(!empty($result), new FormattableMarkup('Ensure the block @id does not appear on the page', ['@id' => $block->id()]));

I'm not sure the word 'Ensure' adds anything here? Can be fixed on commit

fixed on commit

diff --git a/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php b/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php
index 17244ff..ef3ebef 100644
--- a/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php
+++ b/core/modules/block/tests/src/Functional/AssertBlockAppearsTrait.php
@@ -20,7 +20,7 @@
    */
   protected function assertBlockAppears(Block $block) {
     $result = $this->findBlockInstance($block);
-    $this->assertTrue(!empty($result), new FormattableMarkup('Ensure the block @id appears on the page', ['@id' => $block->id()]));
+    $this->assertTrue(!empty($result), new FormattableMarkup('The block @id appears on the page', ['@id' => $block->id()]));
   }
 
   /**
@@ -31,7 +31,7 @@ protected function assertBlockAppears(Block $block) {
    */
   protected function assertNoBlockAppears(Block $block) {
     $result = $this->findBlockInstance($block);
-    $this->assertFalse(!empty($result), new FormattableMarkup('Ensure the block @id does not appear on the page', ['@id' => $block->id()]));
+    $this->assertFalse(!empty($result), new FormattableMarkup('The block @id does not appear on the page', ['@id' => $block->id()]));
   }
 
   /**

Committed as 7ba7715 and pushed to 8.5.x

Cherry picked as 4389c91 and pushed to 8.4.x

Published the change record.

Unpostponed the blocked issues.