Ensure a NULL container in UnitTestCase::setUP

It wasn't tagged novice when I first loaded the node!

I'm really not a novice, I swear

Hi guys, I followed the suggestion in #4 and added in the if statement, whilst proudly wearing a novice's cap, of course. :)

This looks like a nice DX improvement. Welcome to the core queue, diarmy. :)

Committed and pushed to 8.x. Thanks!

Somehow this breaks the PHPUnit tests of Payment and Currency when those are run through the UI. Command line execution of the tests works fine.

The problem manifests itself in the form of the following error message that is displayed directly after the test execution batch is started:

ResponseText: If you have just changed code (for example deployed a new module or moved an existing one) read http://drupal.org/documentation/rebuildAdditional uncaught exception thrown while handling exception.OriginalSymfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "module_handler" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of /opt/local/apache2/htdocs/testing/8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).AdditionalSymfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "request" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of /opt/local/apache2/htdocs/testing/8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).Uncaught exception thrown in shutdown function.Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "batch.storage" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of /opt/local/apache2/htdocs/testing/8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

• Most of my code does not access the container, but instead uses constructor injection, yet this happens even when I execute the tests for classes that don't access the container at all.
• No code of mine uses the batch.storage service, which makes me believe this is a problem with the test runner, maybe even UnitTestCase::tearDown() resetting the host site's container?

This is the same approach that TestBase takes, and to me it looks much more sane.

It will also likely fail, because I haven't seen any PHPUnit test case yet that calls parent::setUp()...

I wonder whether we really need a clone, have you tried it?

I assume we do not need to clone the original container. I have not tested it, as I knew TestBase had already solved this problem and figured that taking a similar approach would be good.

No more cloning, a check to see if there even is an original container, and setting an empty new container if that's the case.

Any chance to move this code completely out of the PHPUnit test classes and instead into the responsible function in simpletest.module that wraps the invocation of phpunit?

As a @dawehner already mentioned, phpunit tests are normally executed from the command line. Only the Simpletest/UI wrapper needs this additional futzing with globals.

Thus, instead of making our phpunit tests ugly, we should move the ugliness into the single spot that is simpletest.module.

How can a phpunit test futz with the simpletest test runner environment, if phpunit is executed in a separate shell process?

https://api.drupal.org/api/drupal/core!modules!simpletest!simpletest.mod...

Looking at the originally committed patch in #5, its one and only purpose seems to be to reset \Drupal to nothing, so as to prevent a previously container in \Drupal to leak into a subsequently executed test.

As long as classes covered by PHPUnit tests depend on the global state construct that is \Drupal, that operation actually belongs into setUp() or perhaps even setUpBeforeClass(), because we have to ensure a clean environment before a test is executed, not afterwards.

These words in code.

Well that won't work because it requires something that satisfies the typehint.

And passing a new empty container will pollute subsequent tests even more.

PHP 5 introduces type hinting. Functions are now able to force parameters to be objects (by specifying class name in the function prototype), interfaces, arrays or callable. However, if NULL is used as the default parameter value, it will be allowed as an argument for any later call.

Ugh, PHP core logic strikes back.

tearDown() is irrelevant and not necessarily reached (in case of an exception or whatnot).

Architecturally, that's the essence of the patch in #13 - all it tries to ensure is that the global state container in \Drupal of a previous test does not leak into a subsequently invoked test.

But that's the wrong way of thinking about global state in tests. If you have to deal with global state in the first place, then instead of fragile cleanups after a test is executed, you ensure that no global state exists before a test is executed.

Lastly, any idea of $this->original has no place in PHPUnit tests. There is no Drupal environment. As a consequence, there is no "original."

Does that clarify the situation?

The currently proposed patch will also help with #2196241: Remove string translation services from TestBase container

Both issues share the same requirement: Allow the testing framework to ensure that there is no container in \Drupal.

20: drupal8.phpunit-setup-drupal.20.patch queued for re-testing.

+++ b/core/lib/Drupal.php
@@ -104,7 +104,7 @@ class Drupal {
+  public static function setContainer(ContainerInterface $container = NULL) {

Either NULL should be documented, or we should use a reset method, which I think is better DX.

Not sure whether DX is really relevant here, since this is not supposed to be called by any code other than the testing frameworks.

I prefer the patch in #20 - adding a resetContainer() method to \Drupal is very confusing - as it does not reset the container it obliterates it. But I also agree that the docs of setContainer(NULL) could be improved to explain why one would ever call it with NULL.

Added docs. Added proper issue summary.

30: drupal8.phpunit-setup-drupal.30.patch queued for re-testing.

Committed/pushed to 8.x, thanks!