Problem/Motivation

KernelTestBase, some tests and some unit tests currently work under the assumption that $this->container is a container builder instance.

This only works, because ContainerBuilder is both a Container and a ContainerBuilder.

However this is an internal implementation detail, which should not be relied upon.

In #2497243: Replace Symfony container with a Drupal one, stored in cache e.g. the Container is changed to always have a production-container and not only when it was loaded from disk.

While it would be possible to continue to run all tests with a container builder this is up to 2 min / 21 min slower => 10% slower and it also means we do not test under production circumstances.

Proposed resolution

- Decouple containerBuilder from container and ensure that things are properly named and used.
- Ensure that the ContainerBuilder is used for asking for definitions and for registering parameters on the fly
- Ensure that the Container is used for testing the system

Remaining tasks

- Fix it

User interface changes

- None

API changes

- Test only changes:

$this->containerBuilder is available in KernelTestBase, while $this->container should only be used as a container implementing IntrospectableContainerInterface.

Data model changes

- None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because tests rely on internal behavior of the ContainerBuilder, so its misnamed.
Issue priority Major because it blocks a critical.
Unfrozen changes Unfrozen because it only changes tests.
Prioritized changes The main goal of this issue is to reduce fragility, by uncoupling things that had been coupled.
Disruption Potentially disruptive for contributed modules that rely on this behavior in KernelTestBase.
CommentFileSizeAuthor
#21 decouple_tests_from-2529516-21.patch9.19 KBkostyashupenko
#5 decouple_tests_from-2529516-5.patch10.89 KBfabianx
#5 interdiff.txt2.16 KBfabianx
#2 decouple_tests_from-2529516-2.patch11.4 KBfabianx

Comments

fabianx’s picture

fabianx commented
Status: Active » Postponed

Uhm, dawehner pointed out that KernelTestBase is potentially deprecated by #2304461: KernelTestBaseTNG™.

=> I tend to won't fix that and remove that chunk from the parent, postponing for now.

fabianx’s picture

fabianx commented
Status: Postponed » Needs review
StatusFileSize
new decouple_tests_from-2529516-2.patch11.4 KB

While that is true, I tried to detangle it and the best compromise is still the attached patch, which makes it as clean as possible and which still improves KernelTestBase in making it more clean.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/system/src/Tests/ServiceProvider/ServiceProviderTest.php
@@ -7,14 +7,14 @@
  */
-class ServiceProviderTest extends WebTestBase {
+class ServiceProviderTest extends KernelTestBase {

@@ -27,13 +27,9 @@ class ServiceProviderTest extends WebTestBase {
-    // The event subscriber method in the test class calls drupal_set_message with
-    // a message saying it has fired. This will fire on every page request so it
-    // should show up on the front page.
-    $this->drupalGet('');
-    $this->assertText(t('The service_provider_test event subscriber fired!'), 'The service_provider_test event subscriber fired');

+1

The change in core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php:18 should be avoidable, given that its an API we work against.
In case your container doens't support that, it has a bug. 

  1. +++ b/core/modules/simpletest/src/KernelTestBase.php
@@ -88,6 +88,13 @@
   /**
+   * The dependency injection container builder used in the test.
+   *
+   * @var \Symfony\Component\DependencyInjection\ContainerBuilder
+   */
+  protected $containerBuilder;

    It should be discouraged to be anything with the container builder, if possible. People might rely on it, even they should not. Let's document that 

  2. +++ b/core/modules/simpletest/src/KernelTestBase.php
@@ -287,18 +294,23 @@ protected function tearDown() {
-      ->addArgument(Database::getConnection())

    Datebase now is an unused use statement.

fabianx’s picture

fabianx commented

#3: We only use ContainerBuilder elsewhere in unit tests.

The IntrospectableContainerInterface only specifies:

setParameter: sets a parameter

... but the compiled container does also throw an Exception there and the ContainerBuilder() after compile was called ...

Should we add compile(), isFrozen(), etc. to support "compiling" the container?

Currently code would need to use:

new Container([], FALSE);

to be able to use setParameter() with my container, which you implemented for tests.

But that is an implementation detail ...

If controverse, I can remove here and leave this change to parent issue, where it probably makes more sense to discuss anyway.

fabianx’s picture

fabianx commented
StatusFileSize
new interdiff.txt2.16 KB
new decouple_tests_from-2529516-5.patch10.89 KB

Fixed the nits of #3 and removed potentially controverse part.

Will switch parent issue over to make container non-frozen by default, then Drupal can pass $frozen = TRUE in explicitly.

dawehner’s picture

dawehner
Primary language German
commented

We only use ContainerBuilder elsewhere in unit tests.

Well, I know, but why? There is no reason. We always use ->set() which works fine. I'm just saying that its not needed to change this as part of this patch.

fabianx’s picture

fabianx commented

#6: The part was completely removed and container made unfrozen by default.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/simpletest/src/KernelTestBase.php
@@ -88,6 +87,20 @@
+   */
+  protected $containerBuilder;
+
+  /**
    * {@inheritdoc}
    */

Can we document the container itself that its the compiled version of it?

fabianx’s picture

fabianx commented
StatusFileSize
new interdiff.txt544 bytes
new decouple_tests_from-2529516-9.patch11.42 KB

Discussed and addressed #8 in IRC.

dawehner’s picture

dawehner
Primary language German
commented

Thank you

needs though still CR

fabianx’s picture

fabianx commented
Priority: Major » Normal
Status: Needs review » Postponed

Alex asked for a test for the bug and I was able to provide one only with rebuilding the container first with allowDump = TRUE in setUp then calling parent::setUp.

That pointed to my container in the parent issue to always dump - even when allowDump is explicitly FALSE.

Therefore I now take $this->allowDump into account and KernelTestBase can continue to use the ContainerBuilder for everything, so works as designed.

Not yet closing though, because I still think its all really 'interesting' behavior, but not critical to fix for the parent.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Why is this postponed? What are we waiting for?

fabianx’s picture

fabianx commented
Status: Postponed » Active

It was postponed on the parent container issue - which is now in.

Thanks, @mgifford

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs work
Issue tags: +Needs reroll

Patch from #9 no longer applies.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new decouple_tests_from-2529516-21.patch9.19 KB

about ServiceProviderTest.php

it was changed few times and moved to tests folder https://github.com/drupal/drupal/commit/20a080288f7342d6fecff46bf7c12286...
so need review.

Status: Needs review » Needs work

The last submitted patch, 21: decouple_tests_from-2529516-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.