Lucas Werkmeister (WMDE) has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/406050 )
Change subject: Split up and test CheckConstraints::newFromGlobalState ...................................................................... Split up and test CheckConstraints::newFromGlobalState CheckConstraints::newFromGlobalState was one giant method that initialized the dependency injection for everything else. This commit splits it up into several methods and adds tests for all of them. I’ll be the first to admit that this still isn’t great, and that the tests don’t do very much, but at least we’ll now get test failures if one of the classes instantiated here gains a parameter, which previously would only be detected by manual tests. Bug: T183373 Change-Id: Ie075eade0f9155128898564874bea9f38a86f443 --- M src/Api/CheckConstraints.php M tests/phpunit/Api/CheckConstraintsTest.php 2 files changed, 221 insertions(+), 72 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseQualityConstraints refs/changes/50/406050/1 diff --git a/src/Api/CheckConstraints.php b/src/Api/CheckConstraints.php index 46b5303..6d092ba 100644 --- a/src/Api/CheckConstraints.php +++ b/src/Api/CheckConstraints.php @@ -4,9 +4,11 @@ use ApiBase; use ApiMain; +use Config; use IBufferingStatsdDataFactory; use MediaWiki\MediaWikiServices; use RequestContext; +use TitleParser; use ValueFormatters\FormatterOptions; use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\EntityIdParser; @@ -19,6 +21,7 @@ use Wikibase\Repo\Api\ResultBuilder; use Wikibase\Repo\EntityIdLabelFormatterFactory; use Wikibase\Repo\WikibaseRepo; +use WikibaseQuality\ConstraintReport\ConstraintCheck\DelegatingConstraintChecker; use WikibaseQuality\ConstraintReport\ConstraintCheck\Helper\ConstraintParameterParser; use WikibaseQuality\ConstraintReport\ConstraintCheck\Result\CheckResult; use WikibaseQuality\ConstraintReport\ConstraintParameterRenderer; @@ -75,32 +78,91 @@ * @param string $prefix * * @return self + * + * @codeCoverageIgnore hard to test and only delegates to other methods */ public static function newFromGlobalState( ApiMain $main, $name, $prefix = '' ) { - $repo = WikibaseRepo::getDefaultInstance(); + return self::getCheckConstraints( + MediaWikiServices::getInstance(), + WikibaseRepo::getDefaultInstance(), + $main, + $name, + $prefix + ); + } + public static function getCheckConstraints( + MediaWikiServices $services, + WikibaseRepo $repo, + ApiMain $main, + $name, + $prefix + ) { + $config = $services->getMainConfig(); + $dataFactory = $services->getStatsdDataFactory(); + $titleParser = $services->getTitleParser(); + + $constraintParameterRenderer = self::getConstraintParameterRenderer( + $config, + $repo + ); + $constraintReportFactory = self::getConstraintReportFactory( + $config, + $dataFactory, + $titleParser, + $repo, + $constraintParameterRenderer + ); + $resultsBuilder = self::getResultsBuilder( + $config, + $dataFactory, + $repo, + $constraintParameterRenderer, + $constraintReportFactory->getConstraintChecker() + ); + + return new CheckConstraints( + $main, + $name, + $prefix, + $repo->getEntityIdParser(), + $repo->getStatementGuidValidator(), + $repo->getApiHelperFactory( RequestContext::getMain() ), + $resultsBuilder, + $dataFactory + ); + } + + public static function getConstraintParameterRenderer( + Config $config, + WikibaseRepo $repo + ) { $language = $repo->getUserLanguage(); - $formatterOptions = new FormatterOptions(); - $formatterOptions->setOption( SnakFormatter::OPT_LANG, $language->getCode() ); - $valueFormatterFactory = $repo->getValueFormatterFactory(); - $valueFormatter = $valueFormatterFactory->getValueFormatter( SnakFormatter::FORMAT_HTML, $formatterOptions ); - - $languageFallbackLabelDescriptionLookupFactory = $repo->getLanguageFallbackLabelDescriptionLookupFactory(); - $labelDescriptionLookup = $languageFallbackLabelDescriptionLookupFactory->newLabelDescriptionLookup( $language ); - $entityIdHtmlLinkFormatterFactory = $repo->getEntityIdHtmlLinkFormatterFactory(); - $entityIdHtmlLinkFormatter = $entityIdHtmlLinkFormatterFactory->getEntityIdFormatter( $labelDescriptionLookup ); - $entityIdLabelFormatterFactory = new EntityIdLabelFormatterFactory(); - $entityIdLabelFormatter = $entityIdLabelFormatterFactory->getEntityIdFormatter( $labelDescriptionLookup ); - $config = MediaWikiServices::getInstance()->getMainConfig(); - $titleParser = MediaWikiServices::getInstance()->getTitleParser(); - $unitConverter = $repo->getUnitConverter(); - $dataFactory = MediaWikiServices::getInstance()->getStatsdDataFactory(); - $constraintParameterRenderer = new ConstraintParameterRenderer( + $entityIdHtmlLinkFormatter = $repo->getEntityIdHtmlLinkFormatterFactory() + ->getEntityIdFormatter( + $repo->getLanguageFallbackLabelDescriptionLookupFactory() + ->newLabelDescriptionLookup( $language ) + ); + $valueFormatter = $repo->getValueFormatterFactory() + ->getValueFormatter( + SnakFormatter::FORMAT_HTML, + new FormatterOptions( [ SnakFormatter::OPT_LANG => $language->getCode() ] ) + ); + return new ConstraintParameterRenderer( $entityIdHtmlLinkFormatter, $valueFormatter, $config ); - $constraintReportFactory = new ConstraintReportFactory( + } + + public static function getConstraintReportFactory( + Config $config, + IBufferingStatsdDataFactory $dataFactory, + TitleParser $titleParser, + WikibaseRepo $repo, + ConstraintParameterRenderer $constraintParameterRenderer + ) { + return new ConstraintReportFactory( $repo->getEntityLookup(), $repo->getPropertyDataTypeLookup(), $repo->getStatementGuidParser(), @@ -114,28 +176,40 @@ $repo->getRdfVocabulary(), $repo->getEntityIdParser(), $titleParser, - $unitConverter, + $repo->getUnitConverter(), $dataFactory ); + } + public static function getResultsBuilder( + Config $config, + IBufferingStatsdDataFactory $dataFactory, + WikibaseRepo $repo, + ConstraintParameterRenderer $constraintParameterRenderer, + DelegatingConstraintChecker $delegatingConstraintChecker + ) { + $entityIdLabelFormatterFactory = new EntityIdLabelFormatterFactory(); + $entityIdLabelFormatter = $entityIdLabelFormatterFactory->getEntityIdFormatter( + $repo->getLanguageFallbackLabelDescriptionLookupFactory() + ->newLabelDescriptionLookup( $repo->getUserLanguage() ) + ); $resultsBuilder = new CheckingResultsBuilder( - $constraintReportFactory->getConstraintChecker(), + $delegatingConstraintChecker, $repo->getEntityTitleLookup(), $entityIdLabelFormatter, $constraintParameterRenderer, $config ); + if ( $config->get( 'WBQualityConstraintsCacheCheckConstraintsResults' ) ) { $wikiPageEntityMetaDataAccessor = new WikiPageEntityMetaDataLookup( $repo->getEntityNamespaceLookup() ); - $entityRevisionLookup = $repo->getEntityRevisionLookup(); - $entityIdParser = $repo->getEntityIdParser(); $resultsBuilder = new CachingResultsBuilder( $resultsBuilder, ResultsCache::getDefaultInstance(), $wikiPageEntityMetaDataAccessor, - $entityIdParser, + $repo->getEntityIdParser(), $config->get( 'WBQualityConstraintsCacheCheckConstraintsTTLSeconds' ), [ $config->get( 'WBQualityConstraintsCommonsLinkConstraintId' ), @@ -147,16 +221,7 @@ ); } - return new CheckConstraints( - $main, - $name, - $prefix, - $repo->getEntityIdParser(), - $repo->getStatementGuidValidator(), - $repo->getApiHelperFactory( RequestContext::getMain() ), - $resultsBuilder, - $dataFactory - ); + return $resultsBuilder; } /** diff --git a/tests/phpunit/Api/CheckConstraintsTest.php b/tests/phpunit/Api/CheckConstraintsTest.php index 4ae94a2..5890a19 100644 --- a/tests/phpunit/Api/CheckConstraintsTest.php +++ b/tests/phpunit/Api/CheckConstraintsTest.php @@ -6,6 +6,7 @@ use DataValues\UnknownValue; use HashConfig; use MediaWiki\Logger\LoggerFactory; +use MediaWiki\MediaWikiServices; use NullStatsdDataFactory; use RequestContext; use Wikibase\DataModel\Entity\Item; @@ -21,6 +22,7 @@ use Wikibase\Repo\Tests\NewItem; use Wikibase\Repo\Tests\NewStatement; use Wikibase\Repo\WikibaseRepo; +use WikibaseQuality\ConstraintReport\Api\CachingResultsBuilder; use WikibaseQuality\ConstraintReport\Api\CheckConstraints; use WikibaseQuality\ConstraintReport\Constraint; use WikibaseQuality\ConstraintReport\ConstraintCheck\ConstraintChecker; @@ -29,6 +31,7 @@ use WikibaseQuality\ConstraintReport\ConstraintCheck\Helper\LoggingHelper; use WikibaseQuality\ConstraintReport\Api\CheckingResultsBuilder; use WikibaseQuality\ConstraintReport\ConstraintCheck\Result\CheckResult; +use WikibaseQuality\ConstraintReport\ConstraintReportFactory; use WikibaseQuality\ConstraintReport\Tests\Fake\FakeChecker; use WikibaseQuality\ConstraintReport\Tests\Fake\InMemoryConstraintLookup; use Wikibase\LanguageFallbackChainFactory; @@ -39,6 +42,7 @@ use Wikimedia\Assert\Assert; use Language; use WikibaseQuality\ConstraintReport\ConstraintParameterRenderer; +use Wikimedia\TestingAccessWrapper; /** * @covers WikibaseQuality\ConstraintReport\Api\CheckConstraints @@ -83,30 +87,6 @@ $wgAPIModules['wbcheckconstraints']['factory'] = function ( $main, $name ) { $repo = WikibaseRepo::getDefaultInstance(); - $factory = new EntityIdLabelFormatterFactory(); - $termLookup = $repo->getTermLookup(); - $termBuffer = $repo->getTermBuffer(); - $languageFallbackChainFactory = new LanguageFallbackChainFactory(); - $fallbackLabelDescLookupFactory = new LanguageFallbackLabelDescriptionLookupFactory( - $languageFallbackChainFactory, - $termLookup, - $termBuffer - ); - $language = new Language(); - $labelLookup = $fallbackLabelDescLookupFactory->newLabelDescriptionLookup( $language ); - $entityIdFormatter = $factory->getEntityIdFormatter( $labelLookup ); - - $formatterOptions = new FormatterOptions(); - $factoryFunctions = []; - Assert::parameterElementType( 'callable', $factoryFunctions, '$factoryFunctions' ); - $formatterOptions->setOption( SnakFormatter::OPT_LANG, $language->getCode() ); - $valueFormatterFactory = new OutputFormatValueFormatterFactory( - $factoryFunctions, - $language, - $languageFallbackChainFactory - ); - $valueFormatter = $valueFormatterFactory->getValueFormatter( SnakFormatter::FORMAT_HTML, $formatterOptions ); - // we can’t use the DefaultConfig trait because we’re in a static method $config = new HashConfig( [ 'WBQualityConstraintsPropertyConstraintId' => 'P1', @@ -119,19 +99,19 @@ 'WBQualityConstraintsCheckDurationInfoSeconds' => 1.0, 'WBQualityConstraintsCheckDurationWarningSeconds' => 10.0, 'WBQualityConstraintsIncludeDetailInApi' => true, + 'WBQualityConstraintsCacheCheckConstraintsResults' => false, ] ); - $entityIdParser = new ItemIdParser(); - $constraintParameterRenderer = new ConstraintParameterRenderer( - $entityIdFormatter, - $valueFormatter, - $config + $dataFactory = new NullStatsdDataFactory(); + + $constraintParameterRenderer = CheckConstraints::getConstraintParameterRenderer( + $config, + $repo ); $constraintParameterParser = new ConstraintParameterParser( $config, $repo->getBaseDataModelDeserializerFactory(), $constraintParameterRenderer ); - $dataFactory = new NullStatsdDataFactory(); $constraintChecker = new DelegatingConstraintChecker( self::$entityLookup, self::$checkerMap, @@ -147,21 +127,22 @@ false, [] ); + $resultsBuilder = CheckConstraints::getResultsBuilder( + $config, + $dataFactory, + $repo, + $constraintParameterRenderer, + $constraintChecker + ); return new CheckConstraints( $main, $name, '', - $entityIdParser, - new StatementGuidValidator( $entityIdParser ), + $repo->getEntityIdParser(), + $repo->getStatementGuidValidator(), $repo->getApiHelperFactory( RequestContext::getMain() ), - new CheckingResultsBuilder( - $constraintChecker, - $repo->getEntityTitleLookup(), - $entityIdFormatter, - $constraintParameterRenderer, - $config - ), + $resultsBuilder, $dataFactory ); }; @@ -180,6 +161,109 @@ parent::tearDownAfterClass(); } + public function testGetResultsBuilder_WithoutCaching() { + $constraintParameterRenderer = $this->getMockBuilder( ConstraintParameterRenderer::class ) + ->disableOriginalConstructor() + ->getMock(); + $delegatingConstraintChecker = $this->getMockBuilder( DelegatingConstraintChecker::class ) + ->disableOriginalConstructor() + ->getMock(); + $resultsBuilder = CheckConstraints::getResultsBuilder( + new HashConfig( [ + 'WBQualityConstraintsCacheCheckConstraintsResults' => false, + ] ), + new NullStatsdDataFactory(), + WikibaseRepo::getDefaultInstance(), + $constraintParameterRenderer, + $delegatingConstraintChecker + ); + + $this->assertInstanceOf( CheckingResultsBuilder::class, $resultsBuilder ); + } + + public function testGetResultsBuilder_WithCaching() { + $constraintParameterRenderer = $this->getMockBuilder( ConstraintParameterRenderer::class ) + ->disableOriginalConstructor() + ->getMock(); + $delegatingConstraintChecker = $this->getMockBuilder( DelegatingConstraintChecker::class ) + ->disableOriginalConstructor() + ->getMock(); + $resultsBuilder = CheckConstraints::getResultsBuilder( + new HashConfig( [ + 'WBQualityConstraintsCacheCheckConstraintsResults' => true, + 'WBQualityConstraintsCacheCheckConstraintsTTLSeconds' => 10, + 'WBQualityConstraintsCommonsLinkConstraintId' => 'Q1', + 'WBQualityConstraintsTypeConstraintId' => 'Q2', + 'WBQualityConstraintsValueTypeConstraintId' => 'Q3', + 'WBQualityConstraintsDistinctValuesConstraintId' => 'Q4', + ] ), + new NullStatsdDataFactory(), + WikibaseRepo::getDefaultInstance(), + $constraintParameterRenderer, + $delegatingConstraintChecker + ); + + $this->assertInstanceOf( CachingResultsBuilder::class, $resultsBuilder ); + $resultsBuilder = TestingAccessWrapper::newFromObject( $resultsBuilder ); + $this->assertSame( 10, $resultsBuilder->ttlInSeconds ); + $this->assertSame( [ 'Q1', 'Q2', 'Q3', 'Q4' ], $resultsBuilder->possiblyStaleConstraintTypes ); + $this->assertInstanceOf( CheckingResultsBuilder::class, $resultsBuilder->resultsBuilder ); + } + + public function testGetConstraintReportFactory() { + $constraintParameterRenderer = $this->getMockBuilder( ConstraintParameterRenderer::class ) + ->disableOriginalConstructor() + ->getMock(); + $constraintReportFactory = CheckConstraints::getConstraintReportFactory( + new HashConfig(), + new NullStatsdDataFactory(), + $this->getMock( \TitleParser::class ), + WikibaseRepo::getDefaultInstance(), + $constraintParameterRenderer + ); + + $this->assertInstanceOf( ConstraintReportFactory::class, $constraintReportFactory ); + } + + public function testGetConstraintParameterRenderer() { + $constraintParameterRenderer = CheckConstraints::getConstraintParameterRenderer( + new HashConfig(), + WikibaseRepo::getDefaultInstance() + ); + + $this->assertInstanceOf( ConstraintParameterRenderer::class, $constraintParameterRenderer ); + } + + public function testGetCheckConstraints() { + $config = $this->getMock( \Config::class ); + $config->method( 'get' )->willReturnCallback( function ( $key ) { + switch ( $key ) { + case 'WBQualityConstraintsPropertiesWithViolatingQualifiers': + return []; + default: + return null; + } + } ); + $dataFactory = new NullStatsdDataFactory(); + $titleParser = $this->getMock( \TitleParser::class ); + $services = $this->getMockBuilder( MediaWikiServices::class ) + ->disableOriginalConstructor() + ->getMock(); + $services->expects( $this->once() )->method( 'getMainConfig' )->willReturn( $config ); + $services->expects( $this->once() )->method( 'getStatsdDataFactory' )->willReturn( $dataFactory ); + $services->expects( $this->once() )->method( 'getTitleParser' )->willReturn( $titleParser ); + + $checkConstraints = CheckConstraints::getCheckConstraints( + $services, + WikibaseRepo::getDefaultInstance(), + new \ApiMain(), + 'wbcheckconstraints', + '' + ); + + $this->assertInstanceOf( CheckConstraints::class, $checkConstraints ); + } + public function testReportForNonexistentItemIsEmpty() { $result = $this->doRequest( [ CheckConstraints::PARAM_ID => self::NONEXISTENT_ITEM ] -- To view, visit https://gerrit.wikimedia.org/r/406050 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie075eade0f9155128898564874bea9f38a86f443 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/WikibaseQualityConstraints Gerrit-Branch: master Gerrit-Owner: Lucas Werkmeister (WMDE) <lucas.werkmeis...@wikimedia.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits