From d6dd6e4d7220a7b34d0256acf50cbeee8b7f22c0 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 11 Jul 2019 20:48:57 +0100 Subject: [PATCH] resourceloader: Remove use of object registering in test suites This was done as a "clever" shortcut to make sure tests a little but shorter, but also made them less consistent with normal code. Remove this in favour of 'class' or 'factory' options as needed. Also remove a bunch of unneeded register() calls. The tests cover everything affected by this change. Side fix - isFileModule should reject modules with 'factory' the same way it rejected raw objects and non-FileModule 'class' cases already. This is now covered by tests as well. Bug: T222637 Change-Id: I3996317dbcd780cc6e0f82c84e769c08a3fc42bb --- includes/resourceloader/ResourceLoader.php | 2 +- tests/phpunit/ResourceLoaderTestCase.php | 10 +- .../resourceloader/MessageBlobStoreTest.php | 28 +- .../ResourceLoaderClientHtmlTest.php | 2 +- .../ResourceLoaderModuleTest.php | 1 + .../ResourceLoaderStartUpModuleTest.php | 276 ++++++++++++------ .../resourceloader/ResourceLoaderTest.php | 112 +++---- .../ResourceLoaderWikiModuleTest.php | 28 +- 8 files changed, 286 insertions(+), 173 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index ec376e3a3c..6eb9908a64 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -566,7 +566,7 @@ class ResourceLoader implements LoggerAwareInterface { if ( isset( $info['object'] ) ) { return false; } - return ( + return !isset( $info['factory'] ) && ( // The implied default for 'class' is ResourceLoaderFileModule !isset( $info['class'] ) || // Explicit default diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php index 64693b0dc5..2a21351f41 100644 --- a/tests/phpunit/ResourceLoaderTestCase.php +++ b/tests/phpunit/ResourceLoaderTestCase.php @@ -101,6 +101,7 @@ class ResourceLoaderTestModule extends ResourceLoaderModule { protected $type = ResourceLoaderModule::LOAD_GENERAL; protected $targets = [ 'phpunit' ]; protected $shouldEmbed = null; + protected $mayValidateScript = false; public function __construct( $options = [] ) { foreach ( $options as $key => $value ) { @@ -109,7 +110,14 @@ class ResourceLoaderTestModule extends ResourceLoaderModule { } public function getScript( ResourceLoaderContext $context ) { - return $this->validateScriptFile( 'input', $this->script ); + if ( $this->mayValidateScript ) { + // This enables the validation check that replaces invalid + // scripts with a warning message. + // Based on $wgResourceLoaderValidateJS + return $this->validateScriptFile( 'input', $this->script ); + } else { + return $this->script; + } } public function getStyles( ResourceLoaderContext $context ) { diff --git a/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php b/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php index e094d92b9d..628ddb1ba1 100644 --- a/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php +++ b/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php @@ -11,6 +11,8 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { use MediaWikiCoversValidator; use PHPUnit4And6Compat; + const NAME = 'test.blobstore'; + protected function setUp() { parent::setUp(); // MediaWiki's test wrapper sets $wgMainWANCache to CACHE_NONE. @@ -24,12 +26,15 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { } public function testBlobCreation() { - $module = $this->makeModule( [ 'mainpage' ] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); + $rl->register( self::NAME, [ + 'factory' => function () { + return $this->makeModule( [ 'mainpage' ] ); + } + ] ); $blobStore = $this->makeBlobStore( null, $rl ); - $blob = $blobStore->getBlob( $module, 'en' ); + $blob = $blobStore->getBlob( $rl->getModule( self::NAME ), 'en' ); $this->assertEquals( '{"mainpage":"Main Page"}', $blob, 'Generated blob' ); } @@ -37,7 +42,6 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testBlobCreation_empty() { $module = $this->makeModule( [] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( null, $rl ); $blob = $blobStore->getBlob( $module, 'en' ); @@ -48,7 +52,6 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testBlobCreation_unknownMessage() { $module = $this->makeModule( [ 'i-dont-exist', 'mainpage', 'i-dont-exist2' ] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( null, $rl ); // Generating a blob should continue without errors, @@ -58,9 +61,15 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { } public function testMessageCachingAndPurging() { - $module = $this->makeModule( [ 'example' ] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); + // Register it so that MessageBlobStore::updateMessage can + // discover it from the registry as a module that uses this message. + $rl->register( self::NAME, [ + 'factory' => function () { + return $this->makeModule( [ 'example' ] ); + } + ] ); + $module = $rl->getModule( self::NAME ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); // Advance this new WANObjectCache instance to a normal state, @@ -105,7 +114,6 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testPurgeEverything() { $module = $this->makeModule( [ 'example' ] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); // Advance this new WANObjectCache instance to a normal state. $blobStore->getBlob( $module, 'en' ); @@ -139,7 +147,6 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { // Arrange version 1 of a module $module = $this->makeModule( [ 'foo' ] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); $blobStore->expects( $this->once() ) ->method( 'fetchMessage' ) @@ -158,7 +165,6 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { // We do not receive purges for this because no messages were changed. $module = $this->makeModule( [ 'foo', 'bar' ] ); $rl = new EmptyResourceLoader(); - $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); $blobStore->expects( $this->exactly( 2 ) ) ->method( 'fetchMessage' ) @@ -191,7 +197,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { private function makeModule( array $messages ) { $module = new ResourceLoaderTestModule( [ 'messages' => $messages ] ); - $module->setName( 'test.blobstore' ); + $module->setName( self::NAME ); return $module; } } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php index 408a0a24cc..e1ee3248cc 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php @@ -374,7 +374,7 @@ Deprecation message.' ] } private static function makeModule( array $options = [] ) { - return new ResourceLoaderTestModule( $options ); + return $options + [ 'class' => ResourceLoaderTestModule::class ]; } private static function makeSampleModules() { diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php index 3f6e9b00e4..60b40736d9 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php @@ -86,6 +86,7 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase { $context = $this->getResourceLoaderContext(); $module = new ResourceLoaderTestModule( [ + 'mayValidateScript' => true, 'script' => "var a = 'this is';\n {\ninvalid" ] ); $this->assertEquals( diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php index bc7cb6924c..2691ccc726 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php @@ -22,7 +22,7 @@ mw.loader.register( [] );' [ [ 'msg' => 'Basic registry', 'modules' => [ - 'test.blank' => new ResourceLoaderTestModule(), + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], ], 'out' => ' mw.loader.addSource( { @@ -38,10 +38,22 @@ mw.loader.register( [ [ [ 'msg' => 'Optimise the dependency tree (basic case)', 'modules' => [ - 'a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'b', 'c', 'd' ] ] ), - 'b' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'c' ] ] ), - 'c' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ), - 'd' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ), + 'a' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'b', 'c', 'd' ], + ], + 'b' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'c' ], + ], + 'c' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [], + ], + 'd' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [], + ], ], 'out' => ' mw.loader.addSource( { @@ -76,9 +88,18 @@ mw.loader.register( [ [ [ 'msg' => 'Optimise the dependency tree (tolerate unknown deps)', 'modules' => [ - 'a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'b', 'c', 'x' ] ] ), - 'b' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'c', 'x' ] ] ), - 'c' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ), + 'a' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'b', 'c', 'x' ] + ], + 'b' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'c', 'x' ] + ], + 'c' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [] + ], ], 'out' => ' mw.loader.addSource( { @@ -111,11 +132,26 @@ mw.loader.register( [ // Regression test for T223402. 'msg' => 'Optimise the dependency tree (indirect circular dependency)', 'modules' => [ - 'top' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'middle1', 'util' ] ] ), - 'middle1' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'middle2', 'util' ] ] ), - 'middle2' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'bottom' ] ] ), - 'bottom' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'top' ] ] ), - 'util' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ), + 'top' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'middle1', 'util' ], + ], + 'middle1' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'middle2', 'util' ], + ], + 'middle2' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'bottom' ], + ], + 'bottom' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'top' ], + ], + 'util' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [], + ], ], 'out' => ' mw.loader.addSource( { @@ -162,8 +198,14 @@ mw.loader.register( [ // Regression test for T223402. 'msg' => 'Optimise the dependency tree (direct circular dependency)', 'modules' => [ - 'top' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'util', 'top' ] ] ), - 'util' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ), + 'top' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'util', 'top' ], + ], + 'util' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [], + ], ], 'out' => ' mw.loader.addSource( { @@ -187,13 +229,16 @@ mw.loader.register( [ [ [ 'msg' => 'Version falls back gracefully if getVersionHash throws', 'modules' => [ - 'test.fail' => ( - ( $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) - ->setMethods( [ 'getVersionHash' ] )->getMock() ) - && $mock->method( 'getVersionHash' )->will( - $this->throwException( new Exception ) - ) - ) ? $mock : $mock + 'test.fail' => [ + 'factory' => function () { + $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) + ->setMethods( [ 'getVersionHash' ] )->getMock(); + $mock->method( 'getVersionHash' )->will( + $this->throwException( new Exception ) + ); + return $mock; + } + ] ], 'out' => ' mw.loader.addSource( { @@ -212,11 +257,14 @@ mw.loader.state( { [ [ 'msg' => 'Use version from getVersionHash', 'modules' => [ - 'test.version' => ( - ( $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) - ->setMethods( [ 'getVersionHash' ] )->getMock() ) - && $mock->method( 'getVersionHash' )->willReturn( '1234567' ) - ) ? $mock : $mock + 'test.version' => [ + 'factory' => function () { + $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) + ->setMethods( [ 'getVersionHash' ] )->getMock(); + $mock->method( 'getVersionHash' )->willReturn( '1234567' ); + return $mock; + } + ] ], 'out' => ' mw.loader.addSource( { @@ -232,11 +280,14 @@ mw.loader.register( [ [ [ 'msg' => 'Re-hash version from getVersionHash if too long', 'modules' => [ - 'test.version' => ( - ( $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) - ->setMethods( [ 'getVersionHash' ] )->getMock() ) - && $mock->method( 'getVersionHash' )->willReturn( '12345678' ) - ) ? $mock : $mock + 'test.version' => [ + 'factory' => function () { + $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) + ->setMethods( [ 'getVersionHash' ] )->getMock(); + $mock->method( 'getVersionHash' )->willReturn( '12345678' ); + return $mock; + } + ], ], 'out' => ' mw.loader.addSource( { @@ -252,9 +303,15 @@ mw.loader.register( [ [ [ 'msg' => 'Group signature', 'modules' => [ - 'test.blank' => new ResourceLoaderTestModule(), - 'test.group.foo' => new ResourceLoaderTestModule( [ 'group' => 'x-foo' ] ), - 'test.group.bar' => new ResourceLoaderTestModule( [ 'group' => 'x-bar' ] ), + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.group.foo' => [ + 'class' => ResourceLoaderTestModule::class, + 'group' => 'x-foo', + ], + 'test.group.bar' => [ + 'class' => ResourceLoaderTestModule::class, + 'group' => 'x-bar', + ], ], 'out' => ' mw.loader.addSource( { @@ -282,8 +339,11 @@ mw.loader.register( [ [ [ 'msg' => 'Different target (non-test should not be registered)', 'modules' => [ - 'test.blank' => new ResourceLoaderTestModule(), - 'test.target.foo' => new ResourceLoaderTestModule( [ 'targets' => [ 'x-foo' ] ] ), + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.target.foo' => [ + 'class' => ResourceLoaderTestModule::class, + 'targets' => [ 'x-foo' ], + ], ], 'out' => ' mw.loader.addSource( { @@ -300,16 +360,19 @@ mw.loader.register( [ 'msg' => 'Safemode disabled (default; register all modules)', 'modules' => [ // Default origin: ORIGIN_CORE_SITEWIDE - 'test.blank' => new ResourceLoaderTestModule(), - 'test.core-generated' => new ResourceLoaderTestModule( [ + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.core-generated' => [ + 'class' => ResourceLoaderTestModule::class, 'origin' => ResourceLoaderModule::ORIGIN_CORE_INDIVIDUAL - ] ), - 'test.sitewide' => new ResourceLoaderTestModule( [ + ], + 'test.sitewide' => [ + 'class' => ResourceLoaderTestModule::class, 'origin' => ResourceLoaderModule::ORIGIN_USER_SITEWIDE - ] ), - 'test.user' => new ResourceLoaderTestModule( [ + ], + 'test.user' => [ + 'class' => ResourceLoaderTestModule::class, 'origin' => ResourceLoaderModule::ORIGIN_USER_INDIVIDUAL - ] ), + ], ], 'out' => ' mw.loader.addSource( { @@ -339,16 +402,19 @@ mw.loader.register( [ 'extraQuery' => [ 'safemode' => '1' ], 'modules' => [ // Default origin: ORIGIN_CORE_SITEWIDE - 'test.blank' => new ResourceLoaderTestModule(), - 'test.core-generated' => new ResourceLoaderTestModule( [ + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.core-generated' => [ + 'class' => ResourceLoaderTestModule::class, 'origin' => ResourceLoaderModule::ORIGIN_CORE_INDIVIDUAL - ] ), - 'test.sitewide' => new ResourceLoaderTestModule( [ + ], + 'test.sitewide' => [ + 'class' => ResourceLoaderTestModule::class, 'origin' => ResourceLoaderModule::ORIGIN_USER_SITEWIDE - ] ), - 'test.user' => new ResourceLoaderTestModule( [ + ], + 'test.user' => [ + 'class' => ResourceLoaderTestModule::class, 'origin' => ResourceLoaderModule::ORIGIN_USER_INDIVIDUAL - ] ), + ], ], 'out' => ' mw.loader.addSource( { @@ -374,7 +440,10 @@ mw.loader.register( [ ], ], 'modules' => [ - 'test.blank' => new ResourceLoaderTestModule( [ 'source' => 'example' ] ), + 'test.blank' => [ + 'class' => ResourceLoaderTestModule::class, + 'source' => 'example' + ], ], 'out' => ' mw.loader.addSource( { @@ -394,25 +463,28 @@ mw.loader.register( [ [ [ 'msg' => 'Conditional dependency function', 'modules' => [ - 'test.x.core' => new ResourceLoaderTestModule(), - 'test.x.polyfill' => new ResourceLoaderTestModule( [ + 'test.x.core' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.x.polyfill' => [ + 'class' => ResourceLoaderTestModule::class, 'skipFunction' => 'return true;' - ] ), - 'test.y.polyfill' => new ResourceLoaderTestModule( [ + ], + 'test.y.polyfill' => [ + 'class' => ResourceLoaderTestModule::class, 'skipFunction' => 'return !!(' . ' window.JSON &&' . ' JSON.parse &&' . ' JSON.stringify' . ');' - ] ), - 'test.z.foo' => new ResourceLoaderTestModule( [ + ], + 'test.z.foo' => [ + 'class' => ResourceLoaderTestModule::class, 'dependencies' => [ 'test.x.core', 'test.x.polyfill', 'test.y.polyfill', ], - ] ), + ], ], 'out' => ' mw.loader.addSource( { @@ -463,52 +535,62 @@ mw.loader.register( [ ], ], 'modules' => [ - 'test.blank' => new ResourceLoaderTestModule(), - 'test.x.core' => new ResourceLoaderTestModule(), - 'test.x.util' => new ResourceLoaderTestModule( [ + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.x.core' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.x.util' => [ + 'class' => ResourceLoaderTestModule::class, 'dependencies' => [ 'test.x.core', ], - ] ), - 'test.x.foo' => new ResourceLoaderTestModule( [ + ], + 'test.x.foo' => [ + 'class' => ResourceLoaderTestModule::class, 'dependencies' => [ 'test.x.core', ], - ] ), - 'test.x.bar' => new ResourceLoaderTestModule( [ + ], + 'test.x.bar' => [ + 'class' => ResourceLoaderTestModule::class, 'dependencies' => [ 'test.x.core', 'test.x.util', ], - ] ), - 'test.x.quux' => new ResourceLoaderTestModule( [ + ], + 'test.x.quux' => [ + 'class' => ResourceLoaderTestModule::class, 'dependencies' => [ 'test.x.foo', 'test.x.bar', 'test.x.util', 'test.x.unknown', ], - ] ), - 'test.group.foo.1' => new ResourceLoaderTestModule( [ + ], + 'test.group.foo.1' => [ + 'class' => ResourceLoaderTestModule::class, 'group' => 'x-foo', - ] ), - 'test.group.foo.2' => new ResourceLoaderTestModule( [ + ], + 'test.group.foo.2' => [ + 'class' => ResourceLoaderTestModule::class, 'group' => 'x-foo', - ] ), - 'test.group.bar.1' => new ResourceLoaderTestModule( [ + ], + 'test.group.bar.1' => [ + 'class' => ResourceLoaderTestModule::class, 'group' => 'x-bar', - ] ), - 'test.group.bar.2' => new ResourceLoaderTestModule( [ + ], + 'test.group.bar.2' => [ + 'class' => ResourceLoaderTestModule::class, 'group' => 'x-bar', 'source' => 'example', - ] ), - 'test.target.foo' => new ResourceLoaderTestModule( [ + ], + 'test.target.foo' => [ + 'class' => ResourceLoaderTestModule::class, 'targets' => [ 'x-foo' ], - ] ), - 'test.target.bar' => new ResourceLoaderTestModule( [ + ], + 'test.target.bar' => [ + 'class' => ResourceLoaderTestModule::class, 'source' => 'example', 'targets' => [ 'x-foo' ], - ] ), + ], ], 'out' => ' mw.loader.addSource( { @@ -614,8 +696,9 @@ mw.loader.register( [ public static function provideRegistrations() { return [ [ [ - 'test.blank' => new ResourceLoaderTestModule(), - 'test.min' => new ResourceLoaderTestModule( [ + 'test.blank' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.min' => [ + 'class' => ResourceLoaderTestModule::class, 'skipFunction' => 'return !!(' . ' window.JSON &&' . @@ -625,7 +708,7 @@ mw.loader.register( [ 'dependencies' => [ 'test.blank', ], - ] ), + ], ] ] ]; } @@ -728,8 +811,8 @@ mw.loader.register( [ $context1 = $this->getResourceLoaderContext(); $rl1 = $context1->getResourceLoader(); $rl1->register( [ - 'test.a' => new ResourceLoaderTestModule(), - 'test.b' => new ResourceLoaderTestModule(), + 'test.a' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.b' => [ 'class' => ResourceLoaderTestModule::class ], ] ); $module = new ResourceLoaderStartupModule(); $version1 = $module->getVersionHash( $context1 ); @@ -737,8 +820,8 @@ mw.loader.register( [ $context2 = $this->getResourceLoaderContext(); $rl2 = $context2->getResourceLoader(); $rl2->register( [ - 'test.b' => new ResourceLoaderTestModule(), - 'test.c' => new ResourceLoaderTestModule(), + 'test.b' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.c' => [ 'class' => ResourceLoaderTestModule::class ], ] ); $module = new ResourceLoaderStartupModule(); $version2 = $module->getVersionHash( $context2 ); @@ -746,8 +829,11 @@ mw.loader.register( [ $context3 = $this->getResourceLoaderContext(); $rl3 = $context3->getResourceLoader(); $rl3->register( [ - 'test.a' => new ResourceLoaderTestModule(), - 'test.b' => new ResourceLoaderTestModule( [ 'script' => 'different' ] ), + 'test.a' => [ 'class' => ResourceLoaderTestModule::class ], + 'test.b' => [ + 'class' => ResourceLoaderTestModule::class, + 'script' => 'different', + ], ] ); $module = new ResourceLoaderStartupModule(); $version3 = $module->getVersionHash( $context3 ); @@ -773,7 +859,10 @@ mw.loader.register( [ $context = $this->getResourceLoaderContext(); $rl = $context->getResourceLoader(); $rl->register( [ - 'test.a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'x', 'y' ] ] ), + 'test.a' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'x', 'y' ], + ], ] ); $module = new ResourceLoaderStartupModule(); $version1 = $module->getVersionHash( $context ); @@ -781,7 +870,10 @@ mw.loader.register( [ $context = $this->getResourceLoaderContext(); $rl = $context->getResourceLoader(); $rl->register( [ - 'test.a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'x', 'z' ] ] ), + 'test.a' => [ + 'class' => ResourceLoaderTestModule::class, + 'dependencies' => [ 'x', 'z' ], + ], ] ); $module = new ResourceLoaderStartupModule(); $version2 = $module->getVersionHash( $context ); diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php index 544afae95c..f47bdaf7bf 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php @@ -70,28 +70,19 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { $this->assertTrue( ResourceLoader::isValidModuleName( $name ) ); } - /** - * @covers ResourceLoader::register - * @covers ResourceLoader::getModule - */ - public function testRegisterValidObject() { - $module = new ResourceLoaderTestModule(); - $resourceLoader = new EmptyResourceLoader(); - $resourceLoader->register( 'test', $module ); - $this->assertEquals( $module, $resourceLoader->getModule( 'test' ) ); - } - /** * @covers ResourceLoader::register * @covers ResourceLoader::getModule */ public function testRegisterValidArray() { - $module = new ResourceLoaderTestModule(); $resourceLoader = new EmptyResourceLoader(); // Covers case of register() setting $rl->moduleInfos, // but $rl->modules lazy-populated by getModule() - $resourceLoader->register( 'test', [ 'object' => $module ] ); - $this->assertEquals( $module, $resourceLoader->getModule( 'test' ) ); + $resourceLoader->register( 'test', [ 'class' => ResourceLoaderTestModule::class ] ); + $this->assertInstanceOf( + ResourceLoaderTestModule::class, + $resourceLoader->getModule( 'test' ) + ); } /** @@ -99,10 +90,12 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { * @group medium */ public function testRegisterEmptyString() { - $module = new ResourceLoaderTestModule(); $resourceLoader = new EmptyResourceLoader(); - $resourceLoader->register( '', $module ); - $this->assertEquals( $module, $resourceLoader->getModule( '' ) ); + $resourceLoader->register( '', [ 'class' => ResourceLoaderTestModule::class ] ); + $this->assertInstanceOf( + ResourceLoaderTestModule::class, + $resourceLoader->getModule( '' ) + ); } /** @@ -112,7 +105,7 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { public function testRegisterInvalidName() { $resourceLoader = new EmptyResourceLoader(); $this->setExpectedException( MWException::class, "name 'test!invalid' is invalid" ); - $resourceLoader->register( 'test!invalid', new ResourceLoaderTestModule() ); + $resourceLoader->register( 'test!invalid', [] ); } /** @@ -133,11 +126,13 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { ->method( 'warning' ); $resourceLoader = new EmptyResourceLoader( null, $logger ); - $module1 = new ResourceLoaderTestModule(); - $module2 = new ResourceLoaderTestModule(); - $resourceLoader->register( 'test', $module1 ); - $resourceLoader->register( 'test', $module2 ); - $this->assertSame( $module2, $resourceLoader->getModule( 'test' ) ); + $resourceLoader->register( 'test', [ 'class' => ResourceLoaderSkinModule::class ] ); + $resourceLoader->register( 'test', [ 'class' => ResourceLoaderStartUpModule::class ] ); + $this->assertInstanceOf( + ResourceLoaderStartUpModule::class, + $resourceLoader->getModule( 'test' ), + 'last one wins' + ); } /** @@ -146,8 +141,8 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { public function testGetModuleNames() { // Use an empty one so that core and extension modules don't get in. $resourceLoader = new EmptyResourceLoader(); - $resourceLoader->register( 'test.foo', new ResourceLoaderTestModule() ); - $resourceLoader->register( 'test.bar', new ResourceLoaderTestModule() ); + $resourceLoader->register( 'test.foo', [] ); + $resourceLoader->register( 'test.bar', [] ); $this->assertEquals( [ 'startup', 'test.foo', 'test.bar' ], $resourceLoader->getModuleNames() @@ -155,15 +150,21 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { } public function provideTestIsFileModule() { - $fileModuleObj = $this->getMockBuilder( ResourceLoaderFileModule::class ) - ->disableOriginalConstructor() - ->getMock(); + $fileModuleObj = $this->createMock( ResourceLoaderFileModule::class ); return [ - 'object' => [ false, - new ResourceLoaderTestModule() + 'factory ignored' => [ false, + [ + 'factory' => function () { + return new ResourceLoaderTestModule(); + } + ] ], - 'FileModule object' => [ false, - $fileModuleObj + 'factory ignored (actual FileModule)' => [ false, + [ + 'factory' => function () use ( $fileModuleObj ) { + return $fileModuleObj; + } + ] ], 'simple empty' => [ true, [] @@ -214,7 +215,7 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { */ public function testIsModuleRegistered() { $rl = new EmptyResourceLoader(); - $rl->register( 'test', new ResourceLoaderTestModule() ); + $rl->register( 'test', [] ); $this->assertTrue( $rl->isModuleRegistered( 'test' ) ); $this->assertFalse( $rl->isModuleRegistered( 'test.unknown' ) ); } @@ -709,9 +710,13 @@ END // Disable log from outputErrorAndLog ->setMethods( [ 'outputErrorAndLog' ] )->getMock(); $rl->register( [ - 'foo' => self::getSimpleModuleMock(), - 'ferry' => self::getFailFerryMock(), - 'bar' => self::getSimpleModuleMock(), + 'foo' => [ 'class' => ResourceLoaderTestModule::class ], + 'ferry' => [ + 'factory' => function () { + return self::getFailFerryMock(); + } + ], + 'bar' => [ 'class' => ResourceLoaderTestModule::class ], ] ); $context = $this->getResourceLoaderContext( [], $rl ); @@ -800,7 +805,6 @@ END $modules = array_map( function ( $script ) { return self::getSimpleModuleMock( $script ); }, $scripts ); - $rl->register( $modules ); $context = $this->getResourceLoaderContext( [ @@ -845,7 +849,6 @@ END 'bar' => self::getSimpleModuleMock( 'bar();' ), ]; $rl = new EmptyResourceLoader(); - $rl->register( $modules ); $context = $this->getResourceLoaderContext( [ 'modules' => 'foo|ferry|bar', @@ -885,7 +888,6 @@ END 'bar' => self::getSimpleStyleModuleMock( '.bar{}' ), ]; $rl = new EmptyResourceLoader(); - $rl->register( $modules ); $context = $this->getResourceLoaderContext( [ 'modules' => 'foo|ferry|bar', @@ -922,9 +924,15 @@ END // provide the full Config object here. $rl = new EmptyResourceLoader( MediaWikiServices::getInstance()->getMainConfig() ); $rl->register( [ - 'foo' => self::getSimpleModuleMock( 'foo();' ), - 'ferry' => self::getFailFerryMock(), - 'bar' => self::getSimpleModuleMock( 'bar();' ), + 'foo' => [ 'factory' => function () { + return self::getSimpleModuleMock( 'foo();' ); + } ], + 'ferry' => [ 'factory' => function () { + return self::getFailFerryMock(); + } ], + 'bar' => [ 'factory' => function () { + return self::getSimpleModuleMock( 'bar();' ); + } ], ] ); $context = $this->getResourceLoaderContext( [ @@ -981,15 +989,12 @@ END ] ); $rl = new EmptyResourceLoader(); - $rl->register( [ - 'foo' => $module, - ] ); $context = $this->getResourceLoaderContext( [ 'modules' => 'foo', 'only' => 'scripts' ], $rl ); - $modules = [ 'foo' => $rl->getModule( 'foo' ) ]; + $modules = [ 'foo' => $module ]; $response = $rl->makeModuleResponse( $context, $modules ); $extraHeaders = TestingAccessWrapper::newFromObject( $rl )->extraHeaders; @@ -1022,13 +1027,12 @@ END ] ); $rl = new EmptyResourceLoader(); - $rl->register( [ 'foo' => $foo, 'bar' => $bar ] ); $context = $this->getResourceLoaderContext( [ 'modules' => 'foo|bar', 'only' => 'scripts' ], $rl ); - $modules = [ 'foo' => $rl->getModule( 'foo' ), 'bar' => $rl->getModule( 'bar' ) ]; + $modules = [ 'foo' => $foo, 'bar' => $bar ]; $response = $rl->makeModuleResponse( $context, $modules ); $extraHeaders = TestingAccessWrapper::newFromObject( $rl )->extraHeaders; $this->assertEquals( @@ -1073,7 +1077,11 @@ END 'makeModuleResponse', ] ) ->getMock(); - $rl->register( 'test', $module ); + $rl->register( 'test', [ + 'factory' => function () use ( $module ) { + return $module; + } + ] ); $context = $this->getResourceLoaderContext( [ 'modules' => 'test', 'only' => null ], $rl @@ -1102,7 +1110,11 @@ END 'sendResponseHeaders', ] ) ->getMock(); - $rl->register( 'test', $module ); + $rl->register( 'test', [ + 'factory' => function () use ( $module ) { + return $module; + } + ] ); $context = $this->getResourceLoaderContext( [ 'modules' => 'test' ], $rl ); // Disable logging from outputErrorAndLog $this->setLogger( 'exception', new Psr\Log\NullLogger() ); diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php index c1bdebec66..e8a0884b63 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php @@ -230,7 +230,6 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { $module::$returnFetchTitleInfo = $titleInfo; $rl = new EmptyResourceLoader(); - $rl->register( 'testmodule', $module ); $context = new ResourceLoaderContext( $rl, new FauxRequest() ); TestResourceLoaderWikiModule::invalidateModuleCache( @@ -253,20 +252,16 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { * @covers ResourceLoaderWikiModule::preloadTitleInfo */ public function testGetPreloadedBadTitle() { - // Mock values - $pages = [ - // Covers else branch for invalid page name - '[x]' => [ 'type' => 'styles' ], - ]; - $titleInfo = []; - - // Set up objects - $module = $this->getMockBuilder( TestResourceLoaderWikiModule::class ) - ->setMethods( [ 'getPages' ] )->getMock(); - $module->method( 'getPages' )->willReturn( $pages ); - $module::$returnFetchTitleInfo = $titleInfo; + // Set up + TestResourceLoaderWikiModule::$returnFetchTitleInfo = []; $rl = new EmptyResourceLoader(); - $rl->register( 'testmodule', $module ); + $rl->getConfig()->set( 'UseSiteJs', true ); + $rl->getConfig()->set( 'UseSiteCss', true ); + $rl->register( 'testmodule', [ + 'class' => TestResourceLoaderWikiModule::class, + // Covers preloadTitleInfo branch for invalid page name + 'styles' => [ '[x]' ], + ] ); $context = new ResourceLoaderContext( $rl, new FauxRequest() ); // Act @@ -277,8 +272,8 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { ); // Assert - $module = TestingAccessWrapper::newFromObject( $module ); - $this->assertEquals( $titleInfo, $module->getTitleInfo( $context ), 'Title info' ); + $module = TestingAccessWrapper::newFromObject( $rl->getModule( 'testmodule' ) ); + $this->assertSame( [], $module->getTitleInfo( $context ), 'Title info' ); } /** @@ -371,7 +366,6 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { $module->method( 'getPages' )->willReturn( $pages ); $rl = new EmptyResourceLoader(); - $rl->register( 'testmodule', $module ); $context = new DerivativeResourceLoaderContext( new ResourceLoaderContext( $rl, new FauxRequest() ) ); -- 2.20.1