From b058a0e562ff2ee1ae47517e52c7f1a83d6ce068 Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Wed, 14 Aug 2019 15:58:53 +0300 Subject: [PATCH] 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 --- tests/common/TestsAutoLoader.php | 4 ++ .../includes/block/BlockManagerTest.php | 11 ++++- .../includes/config/LoggedServiceOptions.php | 36 ++++++++++++++ .../config/TestAllServiceOptionsUsed.php | 48 +++++++++++++++++++ .../DefaultPreferencesFactoryTest.php | 12 ++++- .../includes/title/NamespaceInfoTest.php | 16 ++++++- 6 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 tests/phpunit/includes/config/LoggedServiceOptions.php create mode 100644 tests/phpunit/includes/config/TestAllServiceOptionsUsed.php diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index c35e80fada..4911c4a094 100644 --- 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 f42777c503..71f76e760f 100644 --- 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 index 0000000000..41fdf24b20 --- /dev/null +++ b/tests/phpunit/includes/config/LoggedServiceOptions.php @@ -0,0 +1,36 @@ +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 index 0000000000..618472b526 --- /dev/null +++ b/tests/phpunit/includes/config/TestAllServiceOptionsUsed.php @@ -0,0 +1,48 @@ +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 a00eb3fcd0..e7f7067adb 100644 --- a/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php +++ b/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php @@ -1,7 +1,6 @@ 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 c1e258dac0..028c43823d 100644 --- 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(); + } } /** -- 2.20.1