Problem/Motivation

As we complete the last remaining page variable to block conversions (#507488: Convert page elements (local tasks, actions) into blocks, for example), contrib projects are running into testing errors as page elements may no longer exist on the page to test.

By default, the Classy theme does not have any blocks placed in any regions. Since Classy is used for WebTestBase, tests that rely upon checking for the existence of certain markup need to do extra work to do so. The testing profile does not have the block module enabled by default so it too does not have the markup in these blocks available.

Within core, as these block conversions happen, we have enabled the block module and manually placed blocks as needed within tests. Contrib projects could do the same within their tests.

However, as we eliminate the last remaining non-region page template variables, maybe we should revisit whether the testing profile should have block module enabled by default and/or if Classy should have the same blocks placed by default as, say, Stark, for things like local tasks and actions.

That would allow us to remove the block module enabling parts of core tests, the manual placement of blocks within tests and also reduce the errors contrib modules are seeing in their tests without effort for maintainers to change those tests.

Proposed resolution

We need to decide the proper resolution to this challenge.

Remaining tasks

- Decide if the testing profile should have the block module enabled by default.
- Decide if the Classy theme should have the same blocks installed as Stark (or at least similar blocks).
- Make those changes, if we decide to do anything.
- Update core tests accordingly.
- Publish change notice to let contrb know about any changes.

User interface changes

- Themes based off Classy would have things like tabs and menus placed by default.

API changes

Possible changes to core test defaults.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because while this isn't a core bug, it could help improve the contrib maintainer experience and potentially improve the theming experience for themes based off Classy.
Issue priority Major because numerous contrib maintainers have asked about this since #507488: Convert page elements (local tasks, actions) into blocks was committed.
Unfrozen changes Unfrozen because it primarily changes the default markup of Classy (through block placement) and tests.
Prioritized changes The main goal of this issue is usability for site builders using themes based off Classy and contrib maintainers.
Disruption By enabling block module for testing, would this cause errors for contrib tests that manually enable block module? Or install blocks for tests? This is probably less disruption than what contrib maintainers are currently experiencing.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdrummond created an issue. See original summary.

RainbowArray’s picture

larowlan’s picture

Testing profile should remain light weight for speed sake in my opinion

Dave Reid’s picture

I don't know. It's really strange DX that every module that wants to test it's own UI has to manually install blocks.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
3.03 KB

Not that it would help people right away, but we could add something like this.

almaudoh’s picture

We could always add a 'testing_with_blocks' profile that enables block module and places the blocks needed by contrib in classy for their tests.

That way, tests that need those blocks can use that profile, while tests that need speed don't have to.

Mile23’s picture

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -445,6 +445,17 @@ protected function drupalPlaceBlock($plugin_id, array $settings = array()) {
+  protected function enableLocalActionsAndTasksBlocks() {
+    if (!$this->container->get('module_handler')->moduleExists('block')) {
+      $this->container->get('module_installer')->install(['block']);
+    }
+    $this->drupalPlaceBlock('local_tasks_block');
+    $this->drupalPlaceBlock('local_actions_block');
+  }

Diggit. So a test class then has to do two things: 1) Enable block module, 2) call enableLocalActionsAndTasksBlocks().

Do we maybe call that from WebTestBase::setUp()? Then test classes still have to declare their use of block menu, but then they can override enableLocalActionsAndTasksBlocks() as needed.

Dave Reid’s picture

1. I'm still confused that we provide a theme in core that you cannot actually interact with by default.
2. I think I'm leaning towards using the default theme for anything that does UI tests. It would be nice if WebTestBase let me pick the default theme as easy as the install profile. :/

RainbowArray’s picture

So, just so that I understand:

Testing profile does not include block module simply because tests that don't need blocks run faster that way? Do we know how much of an impact that has on testing performance?

It seems like there are a whole lot of tests that require testing elements that appear in the UI, and particularly if we get in #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) and #2476947: Convert "title" page element into a block, those elements won't exist by default.

Classy is the Drupal theme that most resembles D7's default markup... core's markup is now pretty bare (a good thing for a lot of themers). So it feels strange that we can't have default blocks in Classy. That makes Classy a clunky theme to use if things like tabs, menus and eventually page titles and site names aren't available by default. And that's just so testing performance is maybe faster?

Here are the profiles included in core now:
- minimal
- standard
- testing
- testing_config_import
- testing_config_overrides
- testing_multilingual
- testing_multilingual_with_english

Minimal uses Stark, and it has blocks placed by default.

Testing uses Classy, and it does not have blocks placed by default.

Would it be possible to have another profile, testing_with_blocks, that still uses Classy, but has blocks placed by default?

Can we place blocks by default for Classy in the standard profile, even if we don't do so in testing? That would be a nice win for Classy TX I'd think.

Can WebTestBase be extended to another TestBase that has blocks enabled by default, and which uses a profile with blocks installed by default? Even if the setup was the only difference that might greatly simplify a lot of tests both in core and contrib.

I think it would be a nice win if:
- Classy could have blocks by default in the standard profile
- There was some sort of profile that had blocks by default.
- There was an easy way within tests to use that profile and to have block module enabled.

tim.plunkett’s picture

Can we place blocks by default for Classy in the standard profile

Themes do not provide blocks.
Profiles do.
And standard absolutely places these blocks by default, or Drupal would be completely broken.

Is protected $profile = 'testing_with_blocks'; really that much easier than $this->placeSomeBlocksYo();?

RainbowArray’s picture

The standard profile does not install blocks for Classy. I think it probably would make sense for it to do so, and changing that wouldn't affect the testing profile.

Maybe placeSomeBlocksYo() is just as easy. If that sounds good to those affected by that, cool.

Do you think we need multiple helper functions for the different types of blocks? Local actions, tasks, messages, menus, login, and eventually title and site branding seem like potential blocks that might be useful for testing. Or one block placement helper function that would place several different types of blocks?

Mile23’s picture

@mdrummond #9: The reason we're in this situation is because menus, tasks, and actions are now blocks and you can place them anywhere, which means they're not placed by default.

Basically there are two answers to this issue:

1) It's not a problem. In your test class, enable block module and have code that places the blocks explicitly. I actually like this approach because of speed but mostly the explicit declaration of dependencies.

2) You have 10,000 tests already and it would be a super-duper hassle to change all these tests to explicitly place menu blocks. In this case, we should make the testing profile behave as it did before, by causing the testing profile to place the blocks on install.

If we split the difference we might have testing profile with its old behavior, and testing_no_dependencies with the new behavior. testing_config_import almost fits this bill, except for the name, and I'd rather it be Classy theme to be consistent with testing.

RainbowArray’s picture

Just to be clear, menu blocks and local tasks and action blocks are already being tested in core by explicitly placing those blocks where needed. Contrib could do the same. Just trying to figure out if we can make that process any less painful.

almaudoh’s picture

Version: 8.0.x-dev » 8.2.x-dev
longwave’s picture

I think by this point this is a won't fix. Contrib tests should probably explicitly place any blocks they rely on for testing, just the same way core tests do, and contrib tests that already exist for 8.0.x will already have worked around this.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Priority: Major » Normal
Status: Needs review » Closed (works as designed)

Closing since contrib has gotten along fine in the meantime

tim.plunkett’s picture