Test that classes use all their ServiceOptions

Classes that use ServiceOptions need to declare a list of keys that they
use, typically in self::$constructorOptions. If keys are missing from
that list that are supposed to be present, an exception will be thrown
when someone tries to access them. If keys are present on the list that
are never used, no error is flagged. This means if a dependency on a
given configuration option is removed and nobody updates the list, the
service will keep thinking it depends on that option when it doesn't.
This is messy at best, like an unused variable.

A new and easy-to-use TestAllServiceOptionsUsed trait fixes that
problem. It will log all the ServiceOptions accesses while the test
class runs and raise an error if there are any keys that were never
accessed.

In retrospect, it was probably not worth the time to write this, but
it's a sunk cost now.

Change-Id: Idcaf9a0e2f687069869e6f8057908ffee7dd5f11

diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php
index c35e80f..4911c4a 100644 (file)
--- a/tests/common/TestsAutoLoader.php
+++ b/tests/common/TestsAutoLoader.php
@@ -103,6 +103,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();
+       }
 }
 
 /**