Problem/Motivation

This is a followup issue for #1535868: Convert all blocks into plugins and #1871696: Convert block instances to configuration entities to resolve architectural issues.

Several of the migrated block tests create block instances with drupalPost(), because there is not currently a clean API for creating block instances programmatically.

Proposed resolution

Where appropriate (when the block configuration form itself is not being tested), change the tests listed below so that blocks are created programmatically. It might also be worthwhile to add boilerplate code for creating a particular block, since the same pattern is repeated many times.

  1. core/modules/views/lib/Drupal/views/Tests/Wizard/ItemsPerPageTest.php
  2. core/modules/views/lib/Drupal/views/Tests/Wizard/BasicTest.php
  3. core/modules/block/lib/Drupal/block/Tests/NewDefaultThemeBlocksTest.php
  4. core/modules/views/lib/Drupal/views/Tests/UI/OverrideDisplaysTest.php
  5. core/modules/user/lib/Drupal/user/Tests/UserBlocksTests.php
  6. core/modules/translation/lib/Drupal/translation/Tests/TranslationTest.php
  7. core/modules/system/lib/Drupal/system/Tests/System/AccessDeniedTest.php
  8. core/modules/system/lib/Drupal/system/Tests/Upgrade/BlockUpgradePathTest.php
  9. core/modules/system/lib/Drupal/system/Tests/Menu/TrailTest.php
  10. core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
  11. core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsReportsTest.php
  12. core/modules/search/lib/Drupal/search/Tests/SearchConfigSettingsFormTest.php
  13. core/modules/search/lib/Drupal/search/Tests/SearchBlockTest.php
  14. core/modules/poll/lib/Drupal/poll/Tests/PollBlockTest.php
  15. core/modules/openid/lib/Drupal/openid/Tests/OpenIDTestBase.php
  16. core/modules/node/lib/Drupal/node/Tests/NodeBlockTest.php
  17. core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.php
  18. core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
  19. core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
  20. core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
  21. core/modules/forum/lib/Drupal/forum/Tests/ForumNodeAccessTest.php
  22. core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
  23. core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.php
  24. core/modules/book/lib/Drupal/book/Tests/BookTest.php
  25. core/modules/block/lib/Drupal/block/Tests/BlockInvalidRegionTest.php
  26. core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php
  27. core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.php
  28. core/modules/block/lib/Drupal/block/Tests/BlockCacheTest.php
  29. core/modules/aggregator/lib/Drupal/aggregator/Tests/ImportOpmlTest.php
  30. core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorRenderingTest.php
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

33 uses of nearly-identical code sounds like a reason to add WebTestBase::addBlock() to me.

xjm’s picture

Assigned: Unassigned » xjm
Status: Postponed » Active

I'll work on this one.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Active » Needs review
FileSize
19.04 KB

Attached converts #24 through #32. I think the first step is to convert all the tests to use this method, and then we can make drupalCreateBlockInstance() add the block instance programmatically rather than through the UI once block instances afre configuration entities.

xjm’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -340,6 +340,78 @@ protected function drupalCreateContentType($settings = array()) {
+   * @return ??|false

Oh--right now this is the array returned by BlockBase::getConfig().

drupalCreateBlockInstance() is a lot of typing; I'm wondering if we could get away with something like drupalAddBlock() even though it's not quite as accurate.

Status: Needs review » Needs work

The last submitted patch, simpletest-1872540-3.patch, failed testing.

tim.plunkett’s picture

drupalPlaceBlock()? Then again, who types out stuff like drupalCreateBlockInstance(), even now I just copy/pasted that :)

xjm’s picture

I think drupalPlaceBlock() works. I considered it before and rejected it because it doesn't place existing instances in a different region, but at worst people will read the documentation and realize it doesn't do that. +1.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
20.49 KB

Attached renames the method, fixes the test failures, and adds some wholly out-of-scope documentation cleanups because seeing "HTML id" causes me emotional distress.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

I think I'm also going to switch the order of arguments; the settings are customized far more frequently than the theme.

Status: Needs review » Needs work

The last submitted patch, simpletest-1872540-8.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
24.24 KB
39 KB

Attatched does that and converts blocks from 16 on.

Status: Needs review » Needs work

The last submitted patch, simpletest-1872540-11.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs work » Needs review
FileSize
24.24 KB
59.64 KB

This should be everything.

Status: Needs review » Needs work

The last submitted patch, simpletest-1872540-13.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
22.44 KB

Actual interdiff.

xjm’s picture

FileSize
848 bytes
59.64 KB
xjm’s picture

Title: Create block instances programmatically in block tests » Provide a test helper method for creating block instances

Re-scoping. It'll be much easier to convert just this one method to use the API following #1871696: Convert block instances to configuration entities to resolve architectural issues, and it will make writing additional tests easier as well.

tim.plunkett’s picture

Priority: Normal » Major

Without this, #1871696: Convert block instances to configuration entities to resolve architectural issues would have to duplicate all of this work, and with this, it would mean that issue only has to update one helper method.

The bot's almost done with this, and I'm going to review it now, but since this effectively blocks that critical, I'm promoting this.

Status: Needs review » Needs work

The last submitted patch, simpletest-1872540-16.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
60.15 KB

Attached resolves the remaining test failures locally.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. Thanks so much, this makes that other issue so much more sane.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Note for reviewers: I willfully renamed NodeBlockTest to NodeSyndicateBlockTest, even though it's a bit out of scope, because it was misleading and it confused me and the test is getting half-rewritten anyway. :)

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Oops, xpost!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

xjm’s picture

Issue tags: +Block plugins

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Issue summary: View changes

Updated issue summary.