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

CommentFileSizeAuthor
#24 drupal-block-test-1874598-23.patch12.16 KBmojzis
#22 drupal-block-test-1874598-22.patch12.15 KBmojzis
#21 block-test-1874598-21.patch8.48 KBmojzis
#10 block-test-1874598-10.patch21.86 KBxjm
#10 interdiff.txt7.32 KBxjm
#8 custom-block-1874598-8.patch18.6 KBxjm
#8 interdiff.txt2.46 KBxjm
#5 block-1874598-4.patch16.77 KBxjm

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Postponed » Active

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

xjm’s picture

xjm
she/her
Primary language English
commented
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

xjm
she/her
Primary language English
commented
Assigned: xjm » Unassigned
Status: Active » Needs review
StatusFileSize
new block-1874598-4.patch16.77 KB
xjm’s picture

xjm
she/her
Primary language English
commented

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

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.46 KB
new custom-block-1874598-8.patch18.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

xjm
she/her
Primary language English
commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
commented
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

xjm
she/her
Primary language English
commented
Title: Move test coverage for the Custom Block module » Split BlockTest into BlockTestBase, BlockTest, and CustomBlockTest
StatusFileSize
new interdiff.txt7.32 KB
new block-test-1874598-10.patch21.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

xjm
she/her
Primary language English
commented

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
commented

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

xjm
she/her
Primary language English
commented
Status: Needs review » Postponed

No, really.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs tests +Block plugins
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
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

xjm
she/her
Primary language English
commented
Assigned: Unassigned » xjm

Alright, rerolling this now.

mojzis’s picture

mojzis commented
Status: Needs work » Needs review
StatusFileSize
new block-test-1874598-21.patch8.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

mojzis commented
StatusFileSize
new drupal-block-test-1874598-22.patch12.15 KB

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

mojzis commented
StatusFileSize
new drupal-block-test-1874598-23.patch12.16 KB

one more time :)

mojzis’s picture

mojzis commented
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

xjm
she/her
Primary language English
commented
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

mojzis commented

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

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

xjm
she/her
Primary language English
commented

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Quick fix
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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

chrisjlee commented
Issue tags: -Quick fix
chrisjlee’s picture

chrisjlee commented
Issue summary: View changes

Updated issue summary.