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

These tests depend on assertBlock(Not)Appears which lives on WebTestBase but has no equivalent on BrowserTestBase. These methods have been moved to a trait in the block module since we don't want block specific methods on BrowserTestBase and these methods are not utilised a lot in core.

Blockers:

- #2795085: Add assertNoCacheTag to assertLegacyTrait

Out of Scope:
- All update test for now:
- BlockConditionMissingSchemaUpdateTest.php
- BlockContextMappingUpdateFilledTest.php
- BlockContextMappingUpdateTest.php
- BlockRemoveDisabledRegionUpdateTest.php

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Jo Fitzgerald created an issue. See original summary.

Jo Fitzgerald’s picture

Assigned: Jo Fitzgerald » Unassigned
Status: Active » Needs review
FileSize
10.84 KB
  • Moved 10 files (except for BlockTestBase, Update/* and Views/*).
  • Copied BlockTestBase for BC.
  • Updated namespace.
  • Corrected any references to these classes.

Status: Needs review » Needs work

The last submitted patch, 2: 2863644-2.patch, failed testing.

michielnugter’s picture

Issue tags: +phpunit initiative
zviryatko’s picture

zviryatko’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2863644-5.patch, failed testing.

michielnugter’s picture

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

zviryatko’s picture

@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.

michielnugter’s picture

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

naveenvalecha’s picture

Title: Convert web tests of block » [PP-1] Convert web tests of block
Issue summary: View changes
Status: Needs work » Postponed

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

//Naveen

GoZ’s picture

dawehner’s picture

Title: [PP-1] Convert web tests of block » Convert web tests of block
Status: Postponed » Needs review

This is in, yeah!

dawehner’s picture

Status: Needs review » Needs work

Tests aren't passing right now ...

nlisgo’s picture

Assigned: Unassigned » nlisgo
nlisgo’s picture

Assigned: nlisgo » Unassigned

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

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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
901 bytes
24.93 KB

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

Status: Needs review » Needs work

The last submitted patch, 18: 2863644-18.patch, failed testing. View results

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.45 KB
26.08 KB

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

Status: Needs review » Needs work

The last submitted patch, 20: 2863644-20.patch, failed testing. View results

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.99 KB
27.75 KB

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.

Manuel Garcia’s picture

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

dawehner’s picture

  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.

Lendude’s picture

Thanks for the feedback both!

Fixes #23 and #24

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for reducing the patch size!