Reset services before every test

Trying to avoid resetting services introduces a lot of complexity and
several bugs. We were doing a reset for 70% of @group Database tests
anyway.

Instead:

* Reset services at the start of MediaWikiTestCase::run().
* Capture the actual original service container instead of making a
  special shared service container.
* The test-isolated local service container can now only be initialised
  non-statically. Revert the recent conversion of overrideMwServices()
  to static.
* Store a reference to the local service container in the test case
  object. In MediaWikiTestCase, always use the original or local service
  container directly, to avoid confusion about which one is active at
  the time.
* Remove a lot of unnecessary teardown
* Always call ServiceContainer::destroy() before forceGlobalInstance()
  since the memory is not otherwise freed.

Change-Id: I4a17c1c7ec92c14e3bc471f0216473ebe19477b9

diff --git a/tests/common/TestSetup.php b/tests/common/TestSetup.php
index c176a67..2feb438 100644 (file)
--- a/tests/common/TestSetup.php
+++ b/tests/common/TestSetup.php
@@ -104,10 +104,6 @@ class TestSetup {
                // may break testing against floating point values
                // treated with PHP's serialize()
                ini_set( 'serialize_precision', 17 );
-
-               // TODO: we should call MediaWikiTestCase::prepareServices( new GlobalVarConfig() ) here.
-               // But PHPUnit may not be loaded yet, so we have to wait until just
-               // before PHPUnit_TextUI_Command::main() is executed.
        }
 
 }

diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php
index 5cc45f5..17147eb 100644 (file)
--- a/tests/phpunit/MediaWikiTestCase.php
+++ b/tests/phpunit/MediaWikiTestCase.php
@@ -21,13 +21,16 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
        use PHPUnit4And6Compat;
 
        /**
-        * The service locator created by prepareServices(). This service locator will
-        * be restored after each test. Tests that pollute the global service locator
-        * instance should use overrideMwServices() to isolate the test.
+        * The original service locator. This is overridden during setUp().
         *
         * @var MediaWikiServices|null
         */
-       private static $serviceLocator = null;
+       private static $originalServices;
+
+       /**
+        * The local service locator, created during setUp().
+        */
+       private $localServices;
 
        /**
         * $called tracks whether the setUp and tearDown method has been called.
@@ -155,8 +158,10 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
        public static function setUpBeforeClass() {
                parent::setUpBeforeClass();
 
-               // Get the service locator, and reset services if it's not done already
-               self::$serviceLocator = self::prepareServices( new GlobalVarConfig() );
+               // Get the original service locator
+               if ( !self::$originalServices ) {
+                       self::$originalServices = MediaWikiServices::getInstance();
+               }
        }
 
        /**
@@ -245,63 +250,9 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
        }
 
        /**
-        * Prepare service configuration for unit testing.
-        *
-        * This calls MediaWikiServices::resetGlobalInstance() to allow some critical services
-        * to be overridden for testing.
-        *
-        * prepareServices() only needs to be called once, but should be called as early as possible,
-        * before any class has a chance to grab a reference to any of the global services
-        * instances that get discarded by prepareServices(). Only the first call has any effect,
-        * later calls are ignored.
-        *
-        * @note This is called by PHPUnitMaintClass::finalSetup.
-        *
-        * @see MediaWikiServices::resetGlobalInstance()
-        *
-        * @param Config $bootstrapConfig The bootstrap config to use with the new
-        *        MediaWikiServices. Only used for the first call to this method.
-        * @return MediaWikiServices
+        * @deprecated since 1.32
         */
        public static function prepareServices( Config $bootstrapConfig ) {
-               static $services = null;
-
-               if ( !$services ) {
-                       $services = self::resetGlobalServices( $bootstrapConfig );
-               }
-               return $services;
-       }
-
-       /**
-        * Reset global services, and install testing environment.
-        * This is the testing equivalent of MediaWikiServices::resetGlobalInstance().
-        * This should only be used to set up the testing environment, not when
-        * running unit tests. Use MediaWikiTestCase::overrideMwServices() for that.
-        *
-        * @see MediaWikiServices::resetGlobalInstance()
-        * @see prepareServices()
-        * @see MediaWikiTestCase::overrideMwServices()
-        *
-        * @param Config|null $bootstrapConfig The bootstrap config to use with the new
-        *        MediaWikiServices.
-        * @return MediaWikiServices
-        */
-       private static function resetGlobalServices( Config $bootstrapConfig = null ) {
-               $oldServices = MediaWikiServices::getInstance();
-               $oldConfigFactory = $oldServices->getConfigFactory();
-               $oldLoadBalancerFactory = $oldServices->getDBLoadBalancerFactory();
-
-               $testConfig = self::makeTestConfig( $bootstrapConfig );
-
-               MediaWikiServices::resetGlobalInstance( $testConfig );
-
-               $serviceLocator = MediaWikiServices::getInstance();
-               self::installTestServices(
-                       $oldConfigFactory,
-                       $oldLoadBalancerFactory,
-                       $serviceLocator
-               );
-               return $serviceLocator;
        }
 
        /**
@@ -320,7 +271,7 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                $defaultOverrides = new HashConfig();
 
                if ( !$baseConfig ) {
-                       $baseConfig = MediaWikiServices::getInstance()->getBootstrapConfig();
+                       $baseConfig = self::$originalServices->getBootstrapConfig();
                }
 
                /* Some functions require some kind of caching, and will end up using the db,
@@ -415,17 +366,10 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
        }
 
        /**
-        * Resets some well known services that typically have state that may interfere with unit tests.
-        * This is a lightweight alternative to resetGlobalServices().
-        *
-        * @note There is no guarantee that no references remain to stale service instances destroyed
-        * by a call to doLightweightServiceReset().
-        *
-        * @throws MWException if called outside of PHPUnit tests.
-        *
-        * @see resetGlobalServices()
+        * Resets some non-service singleton instances and other static caches. It's not necessary to
+        * reset services here.
         */
-       private function doLightweightServiceReset() {
+       private function resetNonServiceCaches() {
                global $wgRequest, $wgJobClasses;
 
                foreach ( $wgJobClasses as $type => $class ) {
@@ -434,10 +378,6 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                JobQueueGroup::destroySingletons();
 
                ObjectCache::clear();
-               $services = MediaWikiServices::getInstance();
-               $services->resetServiceForTesting( 'MainObjectStash' );
-               $services->resetServiceForTesting( 'LocalServerObjectCache' );
-               $services->getMainWANObjectCache()->clearProcessCache();
                FileBackendGroup::destroySingleton();
                DeferredUpdates::clearPendingUpdates();
 
@@ -453,6 +393,8 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
        }
 
        public function run( PHPUnit_Framework_TestResult $result = null ) {
+               $this->overrideMwServices();
+
                $needsResetDB = false;
 
                if ( !self::$dbSetup || $this->needsDB() ) {
@@ -492,6 +434,10 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                        $this->testLogger->info( "Resetting DB" );
                        $this->resetDB( $this->db, $this->tablesUsed );
                }
+
+               $this->localServices->destroy();
+               $this->localServices = null;
+               MediaWikiServices::forceGlobalInstance( self::$originalServices );
        }
 
        /**
@@ -591,13 +537,12 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                }
 
                // Reset all caches between tests.
-               $this->doLightweightServiceReset();
+               $this->resetNonServiceCaches();
 
                // XXX: reset maintenance triggers
                // Hook into period lag checks which often happen in long-running scripts
-               $services = MediaWikiServices::getInstance();
-               $lbFactory = $services->getDBLoadBalancerFactory();
-               Maintenance::setLBFactoryTriggers( $lbFactory, $services->getMainConfig() );
+               $lbFactory = $this->localServices->getDBLoadBalancerFactory();
+               Maintenance::setLBFactoryTriggers( $lbFactory, $this->localServices->getMainConfig() );
 
                ob_start( 'MediaWikiTestCase::wfResetOutputBuffersBarrier' );
        }
@@ -654,10 +599,6 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                $this->mwGlobalsToUnset = [];
                $this->restoreLoggers();
 
-               if ( self::$serviceLocator && MediaWikiServices::getInstance() !== self::$serviceLocator ) {
-                       MediaWikiServices::forceGlobalInstance( self::$serviceLocator );
-               }
-
                // TODO: move global state into MediaWikiServices
                RequestContext::resetMain();
                if ( session_id() !== '' ) {
@@ -708,13 +649,12 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
         * @param object $object
         */
        protected function setService( $name, $object ) {
-               // If we did not yet override the service locator, so so now.
-               if ( MediaWikiServices::getInstance() === self::$serviceLocator ) {
-                       $this->overrideMwServices();
+               if ( !$this->localServices ) {
+                       throw new Exception( __METHOD__ . ' must be called after MediaWikiTestCase::run()' );
                }
 
-               MediaWikiServices::getInstance()->disableService( $name );
-               MediaWikiServices::getInstance()->redefineService(
+               $this->localServices->disableService( $name );
+               $this->localServices->redefineService(
                        $name,
                        function () use ( $object ) {
                                return $object;
@@ -796,15 +736,17 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
         * Otherwise old namespace data will lurk and cause bugs.
         */
        private function resetNamespaces() {
+               if ( !$this->localServices ) {
+                       throw new Exception( __METHOD__ . ' must be called after MediaWikiTestCase::run()' );
+               }
                MWNamespace::clearCaches();
                Language::clearCaches();
 
                // We can't have the TitleFormatter holding on to an old Language object either
                // @todo We shouldn't need to reset all the aliases here.
-               $services = MediaWikiServices::getInstance();
-               $services->resetServiceForTesting( 'TitleFormatter' );
-               $services->resetServiceForTesting( 'TitleParser' );
-               $services->resetServiceForTesting( '_MediaWikiTitleCodec' );
+               $this->localServices->resetServiceForTesting( 'TitleFormatter' );
+               $this->localServices->resetServiceForTesting( 'TitleParser' );
+               $this->localServices->resetServiceForTesting( '_MediaWikiTitleCodec' );
        }
 
        /**
@@ -963,16 +905,15 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
         * @return MediaWikiServices
         * @throws MWException
         */
-       protected static function overrideMwServices(
+       protected function overrideMwServices(
                Config $configOverrides = null, array $services = []
        ) {
                if ( !$configOverrides ) {
                        $configOverrides = new HashConfig();
                }
 
-               $oldInstance = MediaWikiServices::getInstance();
-               $oldConfigFactory = $oldInstance->getConfigFactory();
-               $oldLoadBalancerFactory = $oldInstance->getDBLoadBalancerFactory();
+               $oldConfigFactory = self::$originalServices->getConfigFactory();
+               $oldLoadBalancerFactory = self::$originalServices->getDBLoadBalancerFactory();
 
                $testConfig = self::makeTestConfig( null, $configOverrides );
                $newInstance = new MediaWikiServices( $testConfig );
@@ -994,7 +935,13 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                        $oldLoadBalancerFactory,
                        $newInstance
                );
+
+               if ( $this->localServices ) {
+                       $this->localServices->destroy();
+               }
+
                MediaWikiServices::forceGlobalInstance( $newInstance );
+               $this->localServices = $newInstance;
 
                return $newInstance;
        }
@@ -1682,12 +1629,6 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                foreach ( $tables as $tbl ) {
                        $tbl = $db->tableName( $tbl );
                        $db->query( "DROP TABLE IF EXISTS $tbl", __METHOD__ );
-
-                       if ( $tbl === 'page' ) {
-                               // Forget about the pages since they don't
-                               // exist in the DB.
-                               MediaWikiServices::getInstance()->getLinkCache()->clear();
-                       }
                }
        }
 
@@ -1835,12 +1776,6 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                                __METHOD__
                        );
                }
-
-               if ( $tableName === 'page' ) {
-                       // Forget about the pages since they don't
-                       // exist in the DB.
-                       MediaWikiServices::getInstance()->getLinkCache()->clear();
-               }
        }
 
        private static function unprefixTable( &$tableName, $ind, $prefix ) {

diff --git a/tests/phpunit/includes/PrefixSearchTest.php b/tests/phpunit/includes/PrefixSearchTest.php
index 0e35767..685cb40 100644 (file)
--- a/tests/phpunit/includes/PrefixSearchTest.php
+++ b/tests/phpunit/includes/PrefixSearchTest.php
@@ -68,8 +68,6 @@ class PrefixSearchTest extends MediaWikiLangTestCase {
                parent::tearDown();
 
                TestingAccessWrapper::newFromClass( Hooks::class )->handlers = $this->originalHandlers;
-
-               $this->overrideMwServices();
        }
 
        protected function searchProvision( array $results = null ) {

diff --git a/tests/phpunit/includes/Storage/PageUpdaterTest.php b/tests/phpunit/includes/Storage/PageUpdaterTest.php
index 2805ea8..517e7c6 100644 (file)
--- a/tests/phpunit/includes/Storage/PageUpdaterTest.php
+++ b/tests/phpunit/includes/Storage/PageUpdaterTest.php
@@ -19,14 +19,6 @@ use WikiPage;
  * @group Database
  */
 class PageUpdaterTest extends MediaWikiTestCase {
-
-       public static function setUpBeforeClass() {
-               parent::setUpBeforeClass();
-
-               // force service reset!
-               MediaWikiServices::getInstance()->resetServiceForTesting( 'RevisionStore' );
-       }
-
        private function getDummyTitle( $method ) {
                return Title::newFromText( $method, $this->getDefaultWikitextNS() );
        }

diff --git a/tests/phpunit/includes/search/SearchEnginePrefixTest.php b/tests/phpunit/includes/search/SearchEnginePrefixTest.php
index 41c1218..ee272b9 100644 (file)
--- a/tests/phpunit/includes/search/SearchEnginePrefixTest.php
+++ b/tests/phpunit/includes/search/SearchEnginePrefixTest.php
@@ -76,8 +76,6 @@ class SearchEnginePrefixTest extends MediaWikiLangTestCase {
                parent::tearDown();
 
                TestingAccessWrapper::newFromClass( Hooks::class )->handlers = $this->originalHandlers;
-
-               $this->overrideMwServices();
        }
 
        protected function searchProvision( array $results = null ) {

diff --git a/tests/phpunit/phpunit.php b/tests/phpunit/phpunit.php
index d83dedb..b1cca4a 100755 (executable)
--- a/tests/phpunit/phpunit.php
+++ b/tests/phpunit/phpunit.php
@@ -129,9 +129,6 @@ class PHPUnitMaintClass extends Maintenance {
                        'Using HHVM ' . HHVM_VERSION . ' (' . PHP_VERSION . ")\n" :
                        'Using PHP ' . PHP_VERSION . "\n";
 
-               // Prepare global services for unit tests.
-               MediaWikiTestCase::prepareServices( new GlobalVarConfig() );
-
                $phpUnitClass::main();
        }
 

diff --git a/tests/phpunit/structure/SpecialPageFatalTest.php b/tests/phpunit/structure/SpecialPageFatalTest.php
index 9d85fde..a6bc5a7 100644 (file)
--- a/tests/phpunit/structure/SpecialPageFatalTest.php
+++ b/tests/phpunit/structure/SpecialPageFatalTest.php
@@ -13,17 +13,6 @@ use MediaWiki\MediaWikiServices;
  * @author Addshore
  */
 class SpecialPageFatalTest extends MediaWikiTestCase {
-
-       public static function setUpBeforeClass() {
-               parent::setUpBeforeClass();
-               self::overrideMwServices();
-       }
-
-       public static function tearDownAfterClass() {
-               self::overrideMwServices();
-               parent::tearDownAfterClass();
-       }
-
        public function provideSpecialPages() {
                $specialPages = [];
                $spf = MediaWikiServices::getInstance()->getSpecialPageFactory();

diff --git a/tests/phpunit/tests/MediaWikiTestCaseTest.php b/tests/phpunit/tests/MediaWikiTestCaseTest.php
index 1850f6f..d2fca72 100644 (file)
--- a/tests/phpunit/tests/MediaWikiTestCaseTest.php
+++ b/tests/phpunit/tests/MediaWikiTestCaseTest.php
@@ -114,9 +114,6 @@ class MediaWikiTestCaseTest extends MediaWikiTestCase {
 
                $this->overrideMwServices();
                $this->assertNotSame( $initialServices, MediaWikiServices::getInstance() );
-
-               $this->tearDown();
-               $this->assertSame( $initialServices, MediaWikiServices::getInstance() );
        }
 
        public function testSetService() {
@@ -126,17 +123,11 @@ class MediaWikiTestCaseTest extends MediaWikiTestCase {
                        ->disableOriginalConstructor()->getMock();
 
                $this->setService( 'DBLoadBalancer', $mockService );
-               $this->assertNotSame( $initialServices, MediaWikiServices::getInstance() );
                $this->assertNotSame(
                        $initialService,
                        MediaWikiServices::getInstance()->getDBLoadBalancer()
                );
                $this->assertSame( $mockService, MediaWikiServices::getInstance()->getDBLoadBalancer() );
-
-               $this->tearDown();
-               $this->assertSame( $initialServices, MediaWikiServices::getInstance() );
-               $this->assertNotSame( $mockService, MediaWikiServices::getInstance()->getDBLoadBalancer() );
-               $this->assertSame( $initialService, MediaWikiServices::getInstance()->getDBLoadBalancer() );
        }
 
        /**