Merge "Test that classes use all their ServiceOptions"

diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php
index 65ba495..fd418f7 100644 (file)
--- a/tests/common/TestsAutoLoader.php
+++ b/tests/common/TestsAutoLoader.php
@@ -104,6 +104,10 @@ $wgAutoloadClasses += [
        # tests/phpunit/includes/changes
        'TestRecentChangesHelper' => "$testDir/phpunit/includes/changes/TestRecentChangesHelper.php",
 
+       # tests/phpunit/includes/config
+       'TestAllServiceOptionsUsed' => "$testDir/phpunit/includes/config/TestAllServiceOptionsUsed.php",
+       'LoggedServiceOptions' => "$testDir/phpunit/includes/config/LoggedServiceOptions.php",
+
        # tests/phpunit/includes/content
        'DummyContentHandlerForTesting' =>
                "$testDir/phpunit/mocks/content/DummyContentHandlerForTesting.php",

diff --git a/tests/phpunit/includes/block/BlockManagerTest.php b/tests/phpunit/includes/block/BlockManagerTest.php
index f42777c..71f76e7 100644 (file)
--- a/tests/phpunit/includes/block/BlockManagerTest.php
+++ b/tests/phpunit/includes/block/BlockManagerTest.php
@@ -4,7 +4,6 @@ use MediaWiki\Block\BlockManager;
 use MediaWiki\Block\DatabaseBlock;
 use MediaWiki\Block\CompositeBlock;
 use MediaWiki\Block\SystemBlock;
-use MediaWiki\Config\ServiceOptions;
 use MediaWiki\MediaWikiServices;
 use Wikimedia\TestingAccessWrapper;
 
@@ -14,6 +13,7 @@ use Wikimedia\TestingAccessWrapper;
  * @coversDefaultClass \MediaWiki\Block\BlockManager
  */
 class BlockManagerTest extends MediaWikiTestCase {
+       use TestAllServiceOptionsUsed;
 
        /** @var User */
        protected $user;
@@ -50,7 +50,8 @@ class BlockManagerTest extends MediaWikiTestCase {
                $this->setMwGlobals( $blockManagerConfig );
                $this->overrideMwServices();
                return [
-                       new ServiceOptions(
+                       new LoggedServiceOptions(
+                               self::$serviceOptionsAccessLog,
                                BlockManager::$constructorOptions,
                                MediaWikiServices::getInstance()->getMainConfig()
                        ),
@@ -680,4 +681,10 @@ class BlockManagerTest extends MediaWikiTestCase {
                ];
        }
 
+       /**
+        * @coversNothing
+        */
+       public function testAllServiceOptionsUsed() {
+               $this->assertAllServiceOptionsUsed( [ 'ApplyIpBlocksToXff', 'SoftBlockRanges' ] );
+       }
 }

diff --git a/tests/phpunit/includes/config/LoggedServiceOptions.php b/tests/phpunit/includes/config/LoggedServiceOptions.php
new file mode 100644 (file)
index 0000000..41fdf24
--- /dev/null
+++ b/tests/phpunit/includes/config/LoggedServiceOptions.php
@@ -0,0 +1,36 @@
+<?php
+
+use MediaWiki\Config\ServiceOptions;
+
+/**
+ * Helper for TestAllServiceOptionsUsed.
+ */
+class LoggedServiceOptions extends ServiceOptions {
+       /** @var array */
+       private $accessLog;
+
+       /**
+        * @param array &$accessLog Pass self::$serviceOptionsAccessLog from the class implementing
+        *   TestAllServiceOptionsUsed.
+        * @param string[] $keys
+        * @param mixed ...$args Forwarded to parent as-is.
+        */
+       public function __construct( array &$accessLog, array $keys, ...$args ) {
+               $this->accessLog = &$accessLog;
+               if ( !$accessLog ) {
+                       $accessLog = [ $keys, [] ];
+               }
+
+               parent::__construct( $keys, ...$args );
+       }
+
+       /**
+        * @param string $key
+        * @return mixed
+        */
+       public function get( $key ) {
+               $this->accessLog[1][$key] = true;
+
+               return parent::get( $key );
+       }
+}

diff --git a/tests/phpunit/includes/config/TestAllServiceOptionsUsed.php b/tests/phpunit/includes/config/TestAllServiceOptionsUsed.php
new file mode 100644 (file)
index 0000000..618472b
--- /dev/null
+++ b/tests/phpunit/includes/config/TestAllServiceOptionsUsed.php
@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * Use this trait to check that code run by tests accesses every key declared for this class'
+ * ServiceOptions, e.g., in a $constructorOptions member variable. To use this trait, you need to do
+ * two things (other than use-ing it):
+ *
+ * 1) Don't use the regular ServiceOptions when constructing your objects, but rather
+ * LoggedServiceOptions. These are used the same as ServiceOptions, except in the constructor, pass
+ * self::$serviceOptionsAccessLog before the regular arguments.
+ *
+ * 2) Make a test that calls assertAllServiceOptionsUsed(). If some ServiceOptions keys are not yet
+ * accessed in tests but actually are used by the class, pass their names as an argument.
+ *
+ * Currently we support only one ServiceOptions per test class.
+ */
+trait TestAllServiceOptionsUsed {
+       /** @var array [ expected keys (as list), keys accessed so far (as dictionary) ] */
+       private static $serviceOptionsAccessLog = [];
+
+       /**
+        * @param string[] $expectedUnused Options that we know are not yet tested
+        */
+       public function assertAllServiceOptionsUsed( array $expectedUnused = [] ) {
+               $this->assertNotEmpty( self::$serviceOptionsAccessLog,
+                       'You need to pass LoggedServiceOptions to your class instead of ServiceOptions ' .
+                       'for TestAllServiceOptionsUsed to work.'
+               );
+
+               list( $expected, $actual ) = self::$serviceOptionsAccessLog;
+
+               $expected = array_diff( $expected, $expectedUnused );
+
+               $this->assertSame(
+                       [],
+                       array_diff( $expected, array_keys( $actual ) ),
+                       "Some ServiceOptions keys were not accessed in tests. If they really aren't used, " .
+                       "remove them from the class' option list. If they are used, add tests to cover them, " .
+                       "or ignore the problem for now by passing them to assertAllServiceOptionsUsed() in " .
+                       "its \$expectedUnused argument."
+               );
+
+               if ( $expectedUnused ) {
+                       $this->markTestIncomplete( 'Some ServiceOptions keys are not yet accessed by tests: ' .
+                               implode( ', ', $expectedUnused ) );
+               }
+       }
+}

diff --git a/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php b/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php
index a00eb3f..e7f7067 100644 (file)
--- a/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php
+++ b/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php
@@ -1,7 +1,6 @@
 <?php
 
 use MediaWiki\Auth\AuthManager;
-use MediaWiki\Config\ServiceOptions;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Preferences\DefaultPreferencesFactory;
 use Wikimedia\TestingAccessWrapper;
@@ -29,6 +28,7 @@ use Wikimedia\TestingAccessWrapper;
  * @group Preferences
  */
 class DefaultPreferencesFactoryTest extends \MediaWikiTestCase {
+       use TestAllServiceOptionsUsed;
 
        /** @var IContextSource */
        protected $context;
@@ -60,7 +60,8 @@ class DefaultPreferencesFactoryTest extends \MediaWikiTestCase {
                        ->method( $this->anythingBut( 'getValidNamespaces', '__destruct' ) );
 
                return new DefaultPreferencesFactory(
-                       new ServiceOptions( DefaultPreferencesFactory::$constructorOptions, $this->config ),
+                       new LoggedServiceOptions( self::$serviceOptionsAccessLog,
+                               DefaultPreferencesFactory::$constructorOptions, $this->config ),
                        new Language(),
                        AuthManager::singleton(),
                        MediaWikiServices::getInstance()->getLinkRenderer(),
@@ -237,4 +238,11 @@ class DefaultPreferencesFactoryTest extends \MediaWikiTestCase {
                $form->trySubmit();
                $this->assertEquals( 12, $user->getOption( 'rclimit' ) );
        }
+
+       /**
+        * @coversNothing
+        */
+       public function testAllServiceOptionsUsed() {
+               $this->assertAllServiceOptionsUsed( [ 'EnotifMinorEdits', 'EnotifRevealEditorAddress' ] );
+       }
 }

diff --git a/tests/phpunit/includes/title/NamespaceInfoTest.php b/tests/phpunit/includes/title/NamespaceInfoTest.php
index c1e258d..028c438 100644 (file)
--- a/tests/phpunit/includes/title/NamespaceInfoTest.php
+++ b/tests/phpunit/includes/title/NamespaceInfoTest.php
@@ -9,6 +9,8 @@ use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Linker\LinkTarget;
 
 class NamespaceInfoTest extends MediaWikiTestCase {
+       use TestAllServiceOptionsUsed;
+
        /**********************************************************************************************
         * Shared code
         * %{
@@ -63,8 +65,11 @@ class NamespaceInfoTest extends MediaWikiTestCase {
        ];
 
        private function newObj( array $options = [] ) : NamespaceInfo {
-               return new NamespaceInfo( new ServiceOptions( NamespaceInfo::$constructorOptions,
-                       $options, self::$defaultOptions ) );
+               return new NamespaceInfo( new LoggedServiceOptions(
+                       self::$serviceOptionsAccessLog,
+                       NamespaceInfo::$constructorOptions,
+                       $options, self::$defaultOptions
+               ) );
        }
 
        // %} End shared code
@@ -1342,6 +1347,13 @@ class NamespaceInfoTest extends MediaWikiTestCase {
        }
 
        // %} End restriction levels
+
+       /**
+        * @coversNothing
+        */
+       public function testAllServiceOptionsUsed() {
+               $this->assertAllServiceOptionsUsed();
+       }
 }
 
 /**