Call resetServices() when setting globals in tests
authorAryeh Gregor <ayg@aryeh.name>
Mon, 26 Aug 2019 08:17:59 +0000 (11:17 +0300)
committerAryeh Gregor <ayg@aryeh.name>
Thu, 29 Aug 2019 11:26:13 +0000 (14:26 +0300)
Now that resetServices() will preserve (but reset) customized services,
it should be reasonably safe to call it every time globals are changed,
and much more effective than relying on tests to call it every time
themselves.

Depends-On: Iab8ea3a61bbc6803805d855ef23c071067646f71
Depends-On: I00e35ecea6a27468674b2a6e7d9d9eb6518e3bd5
Change-Id: Ie7a89f6ed7d52a0bc01672019ff92e7ee105a1f3

tests/phpunit/MediaWikiIntegrationTestCase.php
tests/phpunit/includes/GlobalFunctions/GlobalTest.php
tests/phpunit/includes/api/ApiOptionsTest.php
tests/phpunit/includes/api/ApiQuerySiteinfoTest.php
tests/phpunit/includes/auth/AuthManagerTest.php
tests/phpunit/includes/diff/DifferenceEngineTest.php
tests/phpunit/includes/diff/TextSlotDiffRendererTest.php
tests/phpunit/includes/objectcache/ObjectCacheTest.php

index 33518ef..83f27e8 100644 (file)
@@ -584,24 +584,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                $this->tmpFiles = array_merge( $this->tmpFiles, (array)$files );
        }
 
-       // @todo Make const when we no longer support HHVM (T192166)
-       private static $namespaceAffectingSettings = [
-               'wgAllowImageMoving',
-               'wgCanonicalNamespaceNames',
-               'wgCapitalLinkOverrides',
-               'wgCapitalLinks',
-               'wgContentNamespaces',
-               'wgExtensionMessagesFiles',
-               'wgExtensionNamespaces',
-               'wgExtraNamespaces',
-               'wgExtraSignatureNamespaces',
-               'wgNamespaceContentModels',
-               'wgNamespaceProtection',
-               'wgNamespacesWithSubpages',
-               'wgNonincludableNamespaces',
-               'wgRestrictionLevels',
-       ];
-
        protected function tearDown() {
                global $wgRequest, $wgSQLMode;
 
@@ -648,12 +630,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                foreach ( $this->iniSettings as $name => $value ) {
                        ini_set( $name, $value );
                }
-               if (
-                       array_intersect( self::$namespaceAffectingSettings, array_keys( $this->mwGlobals ) ) ||
-                       array_intersect( self::$namespaceAffectingSettings, $this->mwGlobalsToUnset )
-               ) {
-                       $this->resetNamespaces();
-               }
                $this->mwGlobals = [];
                $this->mwGlobalsToUnset = [];
                $this->restoreLoggers();
@@ -742,7 +718,7 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                );
 
                if ( $name === 'ContentLanguage' ) {
-                       $this->doSetMwGlobals( [ 'wgContLang' => $this->localServices->getContentLanguage() ] );
+                       $this->setMwGlobals( [ 'wgContLang' => $this->localServices->getContentLanguage() ] );
                }
        }
 
@@ -753,9 +729,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
         * The key is added to the array of globals that will be reset afterwards
         * in the tearDown().
         *
-        * It may be necessary to call resetServices() to allow any changed configuration variables
-        * to take effect on services that get initialized based on these variables.
-        *
         * @par Example
         * @code
         *     protected function setUp() {
@@ -779,8 +752,7 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
         * @param mixed|null $value Value to set the global to (ignored
         *  if an array is given as first argument).
         *
-        * @note To allow changes to global variables to take effect on global service instances,
-        *       call resetServices().
+        * @note This will call resetServices().
         *
         * @since 1.21
         */
@@ -789,29 +761,13 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                        $pairs = [ $pairs => $value ];
                }
 
-               if ( isset( $pairs['wgContLang'] ) ) {
-                       throw new MWException(
-                               'No setting $wgContLang, use setContentLang() or setService( \'ContentLanguage\' )'
-                       );
-               }
-
-               $this->doSetMwGlobals( $pairs, $value );
-       }
-
-       /**
-        * An internal method that allows setService() to set globals that tests are not supposed to
-        * touch.
-        */
-       private function doSetMwGlobals( $pairs, $value = null ) {
                $this->doStashMwGlobals( array_keys( $pairs ) );
 
                foreach ( $pairs as $key => $value ) {
                        $GLOBALS[$key] = $value;
                }
 
-               if ( array_intersect( self::$namespaceAffectingSettings, array_keys( $pairs ) ) ) {
-                       $this->resetNamespaces();
-               }
+               $this->resetServices();
        }
 
        /**
@@ -826,23 +782,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                ini_set( $name, $value );
        }
 
-       /**
-        * Must be called whenever namespaces are changed, e.g., $wgExtraNamespaces is altered.
-        * 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()' );
-               }
-
-               if ( $this->localServices !== MediaWikiServices::getInstance() ) {
-                       throw new Exception( __METHOD__ . ' will not work because the global MediaWikiServices '
-                               . 'instance has been replaced by test code.' );
-               }
-
-               Language::clearCaches();
-       }
-
        /**
         * Check if we can back up a value by performing a shallow copy.
         * Values which fail this test are copied recursively.
@@ -939,16 +878,12 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
         * Useful for setting some entries in a configuration array, instead of
         * setting the entire array.
         *
-        * It may be necessary to call resetServices() to allow any changed configuration variables
-        * to take effect on services that get initialized based on these variables.
-        *
         * @param string $name The name of the global, as in wgFooBar
         * @param array $values The array containing the entries to set in that global
         *
         * @throws MWException If the designated global is not an array.
         *
-        * @note To allow changes to global variables to take effect on global service instances,
-        *       call resetServices().
+        * @note This will call resetServices().
         *
         * @since 1.21
         */
@@ -998,6 +933,7 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                }
 
                self::resetGlobalParser();
+               Language::clearCaches();
        }
 
        /**
@@ -1182,16 +1118,11 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
         */
        public function setContentLang( $lang ) {
                if ( $lang instanceof Language ) {
-                       $this->setMwGlobals( 'wgLanguageCode', $lang->getCode() );
                        // Set to the exact object requested
                        $this->setService( 'ContentLanguage', $lang );
+                       $this->setMwGlobals( 'wgLanguageCode', $lang->getCode() );
                } else {
                        $this->setMwGlobals( 'wgLanguageCode', $lang );
-                       // Let the service handler make up the object.  Avoid calling setService(), because if
-                       // we do, overrideMwServices() will complain if it's called later on.
-                       $services = MediaWikiServices::getInstance();
-                       $services->resetServiceForTesting( 'ContentLanguage' );
-                       $this->doSetMwGlobals( [ 'wgContLang' => $services->getContentLanguage() ] );
                }
        }
 
@@ -1202,6 +1133,8 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
         * or three values to set a single permission, like
         *   $this->setGroupPermissions( '*', 'read', false );
         *
+        * @note This will call resetServices().
+        *
         * @since 1.31
         * @param array|string $newPerms Either an array of permissions to change,
         *   in which case the next two parameters are ignored; or a single string
@@ -1224,11 +1157,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                }
 
                $this->setMwGlobals( 'wgGroupPermissions', $newPermissions );
-
-               // Reset services so they pick up the new permissions.
-               // Resetting just PermissionManager is not sufficient, since other services may
-               // have the old instance of PermissionManager injected.
-               $this->resetServices();
        }
 
        /**
@@ -2422,6 +2350,9 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
        /**
         * Create a temporary hook handler which will be reset by tearDown.
         * This replaces other handlers for the same hook.
+        *
+        * @note This will call resetServices().
+        *
         * @param string $hookName Hook name
         * @param mixed $handler Value suitable for a hook handler
         * @since 1.28
index 660734e..8f8f54a 100644 (file)
@@ -321,33 +321,35 @@ class GlobalTest extends MediaWikiTestCase {
                ] );
                $this->setLogger( 'wfDebug', new LegacyLogger( 'wfDebug' ) );
 
+               unlink( $debugLogFile );
                wfDebug( "This is a normal string" );
                $this->assertEquals( "This is a normal string\n", file_get_contents( $debugLogFile ) );
-               unlink( $debugLogFile );
 
+               unlink( $debugLogFile );
                wfDebug( "This is nöt an ASCII string" );
                $this->assertEquals( "This is nöt an ASCII string\n", file_get_contents( $debugLogFile ) );
-               unlink( $debugLogFile );
 
+               unlink( $debugLogFile );
                wfDebug( "\00305This has böth UTF and control chars\003" );
                $this->assertEquals(
                        " 05This has böth UTF and control chars \n",
                        file_get_contents( $debugLogFile )
                );
-               unlink( $debugLogFile );
 
+               unlink( $debugLogFile );
                wfDebugMem();
                $this->assertGreaterThan(
                        1000,
                        preg_replace( '/\D/', '', file_get_contents( $debugLogFile ) )
                );
-               unlink( $debugLogFile );
 
+               unlink( $debugLogFile );
                wfDebugMem( true );
                $this->assertGreaterThan(
                        1000000,
                        preg_replace( '/\D/', '', file_get_contents( $debugLogFile ) )
                );
+
                unlink( $debugLogFile );
        }
 
index bdce70c..2d0ce26 100644 (file)
@@ -42,6 +42,16 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
                $this->mUserMock->method( 'getOptions' )
                        ->willReturn( [] );
 
+               // DefaultPreferencesFactory calls a ton of user methods, but we still want to list all of
+               // them in case bugs are caused by unexpected things returning null that shouldn't.
+               $this->mUserMock->expects( $this->never() )->method( $this->anythingBut(
+                       'getEffectiveGroups', 'getOptionKinds', 'getInstanceForUpdate', 'getOptions', 'getId',
+                       'isAnon', 'getRequest', 'isLoggedIn', 'getName', 'getGroupMemberships', 'getEditCount',
+                       'getRegistration', 'isAllowed', 'getRealName', 'getOption', 'getStubThreshold',
+                       'getBoolOption', 'getEmail', 'getDatePreference', 'useRCPatrol', 'useNPPatrol',
+                       'setOption', 'saveSettings', 'resetOptions', 'isRegistered'
+               ) );
+
                // Create a new context
                $this->mContext = new DerivativeContext( new RequestContext() );
                $this->mContext->getContext()->setTitle( Title::newFromText( 'Test' ) );
@@ -153,6 +163,8 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
 
        private function executeQuery( $request ) {
                $this->mContext->setRequest( new FauxRequest( $request, true, $this->mSession ) );
+               $this->mUserMock->method( 'getRequest' )->willReturn( $this->mContext->getRequest() );
+
                $this->mTested->execute();
 
                return $this->mTested->getResult()->getResultData( null, [ 'Strip' => 'all' ] );
index 282188d..0a2558a 100644 (file)
@@ -210,15 +210,16 @@ class ApiQuerySiteinfoTest extends ApiTestCase {
                        $this->setExpectedApiException( 'apierror-siteinfo-includealldenied' );
                }
 
-               $mockLB = $this->getMockBuilder( LoadBalancer::class )
-                       ->disableOriginalConstructor()
-                       ->setMethods( [ 'getMaxLag', 'getLagTimes', 'getServerName', '__destruct' ] )
-                       ->getMock();
+               $mockLB = $this->createMock( LoadBalancer::class );
                $mockLB->method( 'getMaxLag' )->willReturn( [ null, 7, 1 ] );
                $mockLB->method( 'getLagTimes' )->willReturn( [ 5, 7 ] );
                $mockLB->method( 'getServerName' )->will( $this->returnValueMap( [
                        [ 0, 'apple' ], [ 1, 'carrot' ]
                ] ) );
+               $mockLB->method( 'getLocalDomainID' )->willReturn( 'testdomain' );
+               $mockLB->expects( $this->never() )->method( $this->anythingBut(
+                       'getMaxLag', 'getLagTimes', 'getServerName', 'getLocalDomainID', '__destruct'
+               ) );
                $this->setService( 'DBLoadBalancer', $mockLB );
 
                $this->setMwGlobals( 'wgShowHostnames', $showHostnames );
index fc6f688..16d9d53 100644 (file)
@@ -1610,9 +1610,9 @@ class AuthManagerTest extends \MediaWikiTestCase {
                $this->assertSame( AuthenticationResponse::FAIL, $ret->status );
                $this->assertSame( 'noname', $ret->message->getKey() );
 
+               $this->hook( 'LocalUserCreated', $this->never() );
                $readOnlyMode = \MediaWiki\MediaWikiServices::getInstance()->getReadOnlyMode();
                $readOnlyMode->setReason( 'Because' );
-               $this->hook( 'LocalUserCreated', $this->never() );
                $userReq->username = self::usernameForCreation();
                $ret = $this->manager->beginAccountCreation( $creator, [ $userReq ], 'http://localhost/' );
                $this->unhook( 'LocalUserCreated' );
@@ -1782,11 +1782,11 @@ class AuthManagerTest extends \MediaWikiTestCase {
                        $session, $this->request->getSession()->getSecret( 'AuthManager::accountCreationState' )
                );
 
+               $this->hook( 'LocalUserCreated', $this->never() );
                $this->request->getSession()->setSecret( 'AuthManager::accountCreationState',
                        [ 'username' => $creator->getName() ] + $session );
                $readOnlyMode = \MediaWiki\MediaWikiServices::getInstance()->getReadOnlyMode();
                $readOnlyMode->setReason( 'Because' );
-               $this->hook( 'LocalUserCreated', $this->never() );
                $ret = $this->manager->continueAccountCreation( [] );
                $this->unhook( 'LocalUserCreated' );
                $this->assertSame( AuthenticationResponse::FAIL, $ret->status );
@@ -2486,10 +2486,10 @@ class AuthManagerTest extends \MediaWikiTestCase {
 
                // Wiki is read-only
                $session->clear();
+               $this->hook( 'LocalUserCreated', $this->never() );
                $readOnlyMode = \MediaWiki\MediaWikiServices::getInstance()->getReadOnlyMode();
                $readOnlyMode->setReason( 'Because' );
                $user = \User::newFromName( $username );
-               $this->hook( 'LocalUserCreated', $this->never() );
                $ret = $this->manager->autoCreateUser( $user, AuthManager::AUTOCREATE_SOURCE_SESSION, true );
                $this->unhook( 'LocalUserCreated' );
                $this->assertEquals( \Status::newFatal( wfMessage( 'readonlytext', 'Because' ) ), $ret );
index ba31b4f..c1bb1a0 100644 (file)
@@ -157,8 +157,14 @@ class DifferenceEngineTest extends MediaWikiTestCase {
         * @dataProvider provideGenerateContentDiffBody
         */
        public function testGenerateContentDiffBody(
-               Content $oldContent, Content $newContent, $expectedDiff
+               array $oldContentArgs, array $newContentArgs, $expectedDiff
        ) {
+               $this->mergeMwGlobalArrayValue( 'wgContentHandlers', [
+                       'testing-nontext' => DummyNonTextContentHandler::class,
+               ] );
+               $oldContent = ContentHandler::makeContent( ...$oldContentArgs );
+               $newContent = ContentHandler::makeContent( ...$newContentArgs );
+
                // Set $wgExternalDiffEngine to something bogus to try to force use of
                // the PHP engine rather than wikidiff2.
                $this->setMwGlobals( [
@@ -170,12 +176,9 @@ class DifferenceEngineTest extends MediaWikiTestCase {
                $this->assertSame( $expectedDiff, $this->getPlainDiff( $diff ) );
        }
 
-       public function provideGenerateContentDiffBody() {
-               $this->mergeMwGlobalArrayValue( 'wgContentHandlers', [
-                       'testing-nontext' => DummyNonTextContentHandler::class,
-               ] );
-               $content1 = ContentHandler::makeContent( 'xxx', null, CONTENT_MODEL_TEXT );
-               $content2 = ContentHandler::makeContent( 'yyy', null, CONTENT_MODEL_TEXT );
+       public static function provideGenerateContentDiffBody() {
+               $content1 = [ 'xxx', null, CONTENT_MODEL_TEXT ];
+               $content2 = [ 'yyy', null, CONTENT_MODEL_TEXT ];
 
                return [
                        'self-diff' => [ $content1, $content1, '' ],
index c523561..3eb1201 100644 (file)
@@ -9,14 +9,22 @@ class TextSlotDiffRendererTest extends MediaWikiTestCase {
 
        /**
         * @dataProvider provideGetDiff
-        * @param Content|null $oldContent
-        * @param Content|null $newContent
+        * @param array|null $oldContentArgs To pass to makeContent() (if not null)
+        * @param array|null $newContentArgs
         * @param string|Exception $expectedResult
         * @throws Exception
         */
        public function testGetDiff(
-               Content $oldContent = null, Content $newContent = null, $expectedResult
+               array $oldContentArgs = null, array $newContentArgs = null, $expectedResult
        ) {
+               $this->mergeMwGlobalArrayValue( 'wgContentHandlers', [
+                       'testing' => DummyContentHandlerForTesting::class,
+                       'testing-nontext' => DummyNonTextContentHandler::class,
+               ] );
+
+               $oldContent = $oldContentArgs ? self::makeContent( ...$oldContentArgs ) : null;
+               $newContent = $newContentArgs ? self::makeContent( ...$newContentArgs ) : null;
+
                if ( $expectedResult instanceof Exception ) {
                        $this->setExpectedException( get_class( $expectedResult ), $expectedResult->getMessage() );
                }
@@ -30,31 +38,26 @@ class TextSlotDiffRendererTest extends MediaWikiTestCase {
                $this->assertSame( $expectedResult, $plainDiff );
        }
 
-       public function provideGetDiff() {
-               $this->mergeMwGlobalArrayValue( 'wgContentHandlers', [
-                       'testing' => DummyContentHandlerForTesting::class,
-                       'testing-nontext' => DummyNonTextContentHandler::class,
-               ] );
-
+       public static function provideGetDiff() {
                return [
                        'same text' => [
-                               $this->makeContent( "aaa\nbbb\nccc" ),
-                               $this->makeContent( "aaa\nbbb\nccc" ),
+                               [ "aaa\nbbb\nccc" ],
+                               [ "aaa\nbbb\nccc" ],
                                "",
                        ],
                        'different text' => [
-                               $this->makeContent( "aaa\nbbb\nccc" ),
-                               $this->makeContent( "aaa\nxxx\nccc" ),
+                               [ "aaa\nbbb\nccc" ],
+                               [ "aaa\nxxx\nccc" ],
                                " aaa aaa\n-bbb+xxx\n ccc ccc",
                        ],
                        'no right content' => [
-                               $this->makeContent( "aaa\nbbb\nccc" ),
+                               [ "aaa\nbbb\nccc" ],
                                null,
                                "-aaa+ \n-bbb \n-ccc ",
                        ],
                        'no left content' => [
                                null,
-                               $this->makeContent( "aaa\nbbb\nccc" ),
+                               [ "aaa\nbbb\nccc" ],
                                "- +aaa\n +bbb\n +ccc",
                        ],
                        'no content' => [
@@ -63,13 +66,13 @@ class TextSlotDiffRendererTest extends MediaWikiTestCase {
                                new InvalidArgumentException( '$oldContent and $newContent cannot both be null' ),
                        ],
                        'non-text left content' => [
-                               $this->makeContent( '', 'testing-nontext' ),
-                               $this->makeContent( "aaa\nbbb\nccc" ),
+                               [ '', 'testing-nontext' ],
+                               [ "aaa\nbbb\nccc" ],
                                new ParameterTypeException( '$oldContent', 'TextContent|null' ),
                        ],
                        'non-text right content' => [
-                               $this->makeContent( "aaa\nbbb\nccc" ),
-                               $this->makeContent( '', 'testing-nontext' ),
+                               [ "aaa\nbbb\nccc" ],
+                               [ '', 'testing-nontext' ],
                                new ParameterTypeException( '$newContent', 'TextContent|null' ),
                        ],
                ];
@@ -107,7 +110,7 @@ class TextSlotDiffRendererTest extends MediaWikiTestCase {
         * @param string $model
         * @return null|TextContent
         */
-       private function makeContent( $str, $model = CONTENT_MODEL_TEXT ) {
+       private static function makeContent( $str, $model = CONTENT_MODEL_TEXT ) {
                return ContentHandler::makeContent( $str, null, $model );
        }
 
index 4318852..9aa0712 100644 (file)
@@ -82,9 +82,6 @@ class ObjectCacheTest extends MediaWikiTestCase {
 
        /** @covers ObjectCache::newAnything */
        public function testNewAnythingNoAccelNoDb() {
-               $this->overrideMwServices(); // Ensures restore on tear down
-               MediaWiki\MediaWikiServices::disableStorageBackend();
-
                $this->setMwGlobals( [
                        'wgMainCacheType' => CACHE_ACCEL
                ] );
@@ -94,6 +91,8 @@ class ObjectCacheTest extends MediaWikiTestCase {
                        CACHE_ACCEL => [ 'class' => EmptyBagOStuff::class ]
                ] );
 
+               MediaWiki\MediaWikiServices::disableStorageBackend();
+
                $this->assertInstanceOf(
                        EmptyBagOStuff::class,
                        ObjectCache::newAnything( [] ),