From 532210719184db1d0449b09bfded13cb1dfc1759 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Thu, 30 Aug 2018 21:35:25 +1000 Subject: [PATCH] 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 --- tests/common/TestSetup.php | 4 - tests/phpunit/MediaWikiTestCase.php | 153 +++++------------- tests/phpunit/includes/PrefixSearchTest.php | 2 - .../includes/Storage/PageUpdaterTest.php | 8 - .../search/SearchEnginePrefixTest.php | 2 - tests/phpunit/phpunit.php | 3 - .../structure/SpecialPageFatalTest.php | 11 -- tests/phpunit/tests/MediaWikiTestCaseTest.php | 9 -- 8 files changed, 44 insertions(+), 148 deletions(-) diff --git a/tests/common/TestSetup.php b/tests/common/TestSetup.php index c176a67f7f..2feb438835 100644 --- 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 5cc45f5e21..17147eb732 100644 --- 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 0e357678e8..685cb4024e 100644 --- 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 2805ea88f9..517e7c676b 100644 --- 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 41c12188a8..ee272b9b5d 100644 --- 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 d83dedba41..b1cca4ac2e 100755 --- 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 9d85fdebeb..a6bc5a7f0e 100644 --- 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 1850f6fe5c..d2fca72b10 100644 --- 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() ); } /** -- 2.20.1