All unit tests should use UnitTestCase base class

Unfortunately at this point we need to accept that \Drupal is called in far too many places and we need to cope with that - by using UnitTestCase amongst other things.

This is backwards. If I'm testing a unit that I expect should have no connection to \Drupal, then using \PHPUnit_Framework_TestCase instead of UnitTestCase is part of the test. If something somewhere else breaks my test, there can only be three reasons:

1. Drupal's PHPUnit setup sucks at isolation, which might very well be true.
2. My expectations are incorrect, in which case I switch to UnitTestCase or KernelTestCase or whatever.
3. The other code is in error and needs to be fixed. This is the most likely culprit.

Since #3 is the most likely culprit, mandating the use of UnitTestCase hides errors from us and prevents us from finally limiting \Drupal to it's intended uses.

In fact, here's a patch that makes all changes necessary to get passing tests when you do phpunit -c core/ --testsuite unit after turning UnitTestCase::setUp() into an essential no-op. It's probably a better solution to #2626832: Figure out how to check for unintentionally covered code than the patch over there. Of course we can't really do that because of BC for contrib. Alas. :-)

It demonstrates that things are better than we might think when it comes to the need for heavy-handed \Drupal isolation.

Setting NR only to kick off the testbot.

Related: #2465727: Some unit tests don't call UnitTestCase::setUp()

+++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
@@ -48,6 +48,7 @@ public function testSetContainer() {
    * @expectedExceptionMessage \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.
    */
   public function testGetContainerException() {
+    \Drupal::unsetContainer();
     \Drupal::getContainer();
   }

See how this test is changed to explicitly show what it's testing? That's what we want everywhere.

1. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -48,6 +48,7 @@ public function testSetContainer() {
       * @expectedExceptionMessage \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.
       */
      public function testGetContainerException() {
   +    \Drupal::unsetContainer();
        \Drupal::getContainer();
      }
   

   This makes a LOT of sense!

2. +++ b/core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php
   @@ -12,6 +12,11 @@
   +  protected function setUp() {
   +    parent::setUp();
   +    \Drupal::unsetContainer();
   +  }
   

   This make no sense.

Either that second case needs a very clear comment, or I'm missing something...

Unlike my previous assumption I believe using UnitTestCase

+++ b/core/modules/content_translation/tests/src/Unit/Menu/ContentTranslationLocalTasksTest.php
@@ -18,6 +19,12 @@ protected function setUp() {
 
+    // Ensure that the NullFileCache implementation is used for the FileCache as
+    // unit tests should not be relying on caches implicitly.
+    FileCacheFactory::setConfiguration([FileCacheFactory::DISABLE_CACHE => TRUE]);
+    // Ensure that FileCacheFactory has a prefix.
+    FileCacheFactory::setPrefix('prefix');

I like to make that explicit. Could we maybe have a helper method for that?

@Mile23: oh the irony, your patch shows that lots of testing blows up unintentionally.

"here's a patch that makes all changes necessary to get passing tests when you do phpunit -c core/ --testsuite unit"

Note that's not the testbot. :-)

Check out the results:

OK, but incomplete, skipped, or risky tests!
Tests: 17121, Assertions: 27154, Skipped: 1723.

1723 skipped tests because I don't have the yaml extension. The testbot reports 45 fails.

So 45 failed tests out of 1732. COMPLETELY INSURMOUNTABLE. :-)

Those fails all deal with the file cache prefix not being properly set, which I would call a bug in the file cache system, but no one else will because we can just sweep it under the rug in UnitTestCase::setUp().

if the outcome of this issue is a moderately small patch that demonstrates that we can use \PHPUnit_Framework_TestCase almost everywhere that would be fine with me. I'm a bit sceptical right now :)

That's not the point. The point is that our unit tests kind of suck because they rely on magic, and requiring UnitTestCase makes the situation worse because it's the magic part.

Not everyone knows or cares what side effects are being introduced into their tests, and that's a problem. Because if someone does care, and uses \PHPUnit_Framework_TestCase as a base class for a considered reason, they'll be told they're doing it rong. And that will be me, and I'll end up re-writing this paragraph again.

Could we maybe provide a test listener, instead of relying on the base class, which does the \Drupal cleanup?

Here is an approach for that.

I'm not sure what problem #11 solves. I just want to be able to write a \PHPUnit_Framework_TestCase for core, and not use UnitTestCase unless I think it's a good idea. Moving the setup to the listener just adds to the magic, in a way that I won't be able to avoid it, making the problem worse.

My objection is to the rule that all unit tests should be UnitTestCase, which is what this issue is about. By way of showing how it's not a good idea, I'm pointing out that tests themselves should be explicit in how they set up their dependencies and expectations. If we want to fix it so the tests are better, then we can do that in another issue.

Also: We kind of have to keep UnitTestCase as it is for BC/contrib, unless we want to formally @deprecate it.

Related: #2825078: Deprecate UnitTestCase dependency on block module

I'd sort of like to be able to use which ever base class I choose as well, I think requiring a specific base class is an odd arbitrary requirement and possibly a bit of a blocker for splitting libraries out of /core/lib. The magic cleanup might work but personally I feel like we should trigger errors similar to the way global state change errors/warnings if something was left in the global container.

That said, as the summary pointed out \Drupal::container and related proxy methods pepper Drupal and their addition in patches will likely lead to a convergence on most if not all test cases extending Drupal's base class... ¯\_(ツ)_/¯