Problem/Motivation

This is a followup issue for #1535868: Convert all blocks into plugins.

  • Currently, the Custom Block module has no test coverage of its own.
  • Some test coverage for custom blocks is still in the parent Block module's test namespace.
  • There is no BlockTestBase.
  • BlockTest runs slowly and includes many unrelated tests. Some are related to basic block module functionality, whereas others are related to custom blocks. The custom block module is enabled for all these tests, which is unnecessary overhead.
  • BlockTest also includes some helper methods and setup that could be useful to other tests and, once improved, could reduce test run time for tests that use them.

Proposed resolution

  • Split BlockTest into CustomBlockTest, BlockTest, and BlockTestBase.
  • Move helper code to remove defined block instances into BlockTestBase::setUp(), to reduce the risk of false positives in block tests.

Remaining tasks

Related issue

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Postponed » Active

Some or perhaps all of BlockTest is actually custom block test coverage.

xjm’s picture

Assigned: Unassigned » xjm

Had my nose in BlockTest half of today, so I'll roll a patch that moves the existing tests.

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
16.77 KB
xjm’s picture

We probably want to wait for #1872540: Provide a test helper method for creating block instances to work on this further.

Status: Needs review » Needs work

The last submitted patch, block-1874598-4.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
18.6 KB

Oops, looks like I accidentally removed a helper:

Fatal error: Call to undefined method Drupal\block\Tests\BlockTest::moveBlockToRegion() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/block/lib/Drupal/block/Tests/BlockTest.php on line 161
FATAL Drupal\block\Tests\BlockTest: test runner returned a non-zero error code (255).
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Move and improve test coverage for the Custom Block module » Move test coverage for the Custom Block module

Thinking about it, I'd like to move the code first and commit that since patches that move code are a pain to reroll. Then we can work on improving the test coverage in a followup issue once #1872540: Provide a test helper method for creating block instances is in.

xjm’s picture

Title: Move test coverage for the Custom Block module » Split BlockTest into BlockTestBase, BlockTest, and CustomBlockTest
FileSize
7.32 KB
21.86 KB

Make that "accidentally removed two helpers". The second should actually just be part of the setup method of a BlockTestBase class, though.

xjm’s picture

#10: block-test-1874598-10.patch queued for re-testing.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Postponing this on #1871696: Convert block instances to configuration entities to resolve architectural issues and #1884758: Remove testing profile default blocks, which will do the same thing the old helper did in BlockTest, but better.

xjm’s picture

Status: Needs review » Postponed

No, really.

larowlan’s picture

Also #1871772: Convert custom blocks to content entities will add CRUD/hook tests for Custom Block

xjm’s picture

Issue tags: -Needs tests +Block plugins
tim.plunkett’s picture

Status: Postponed » Needs review
Issue tags: -Blocks-Layouts, -Block plugins

#10: block-test-1874598-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, block-test-1874598-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#10: block-test-1874598-10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Blocks-Layouts, +Block plugins

The last submitted patch, block-test-1874598-10.patch, failed testing.

xjm’s picture

Assigned: Unassigned » xjm

Alright, rerolling this now.

mojzis’s picture

Status: Needs work » Needs review
FileSize
8.48 KB

reroll :
- removed completely the creation of CustomBlockTest - something has already been done there
- function removeDefaultBlocks has allready been removed
slight change in the Base::setup()
- formats are now entities, using the newer code

mojzis’s picture

oops, forgot the base class ...

Status: Needs review » Needs work

The last submitted patch, drupal-block-test-1874598-22.patch, failed testing.

mojzis’s picture

one more time :)

mojzis’s picture

Status: Needs work » Needs review

pls review :)

Status: Needs review » Needs work

The last submitted patch, drupal-block-test-1874598-23.patch, failed testing.

xjm’s picture

Title: Split BlockTest into BlockTestBase, BlockTest, and CustomBlockTest » Add BlockTestBase
Assigned: xjm » Unassigned
Status: Needs work » Needs review

This looks good -- waiting for the bot to RTBC, though. :)

mojzis’s picture

now we have the remaining task of testing the addition of a block from the custom_block module to a region. I am not sure where that should reside - whether in the tests for custom_block (because a block has to be created first) or here (because this is testing block addition :). Should i create the follow-up issue ?

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, let's do that in a followup issue. Thanks!

xjm’s picture

#24: drupal-block-test-1874598-23.patch queued for re-testing.

xjm’s picture

Issue tags: +Quick fix
webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a useful abstraction.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

chrisjlee’s picture

Issue tags: -Quick fix
chrisjlee’s picture

Issue summary: View changes

Updated issue summary.