From 2d5b2f0f6a835446aab05c2009f785c430d25391 Mon Sep 17 00:00:00 2001 From: daniel Date: Fri, 31 Aug 2018 14:56:42 +1000 Subject: [PATCH] Avoid constructing Title objects in data providers Bug: T202641 Change-Id: I34efa0b9329e740bcb292b2529ec8f7f925dc346 --- tests/phpunit/includes/RevisionDbTestBase.php | 6 +- .../Storage/DerivedPageDataUpdaterTest.php | 20 +++- .../includes/Storage/RevisionRecordTests.php | 99 ++++++++++--------- .../includes/api/ApiQuerySearchTest.php | 43 ++++++-- .../includes/diff/DifferenceEngineTest.php | 20 +++- .../phpunit/mocks/search/MockSearchEngine.php | 9 +- .../mocks/search/MockSearchResultSet.php | 17 +++- 7 files changed, 152 insertions(+), 62 deletions(-) diff --git a/tests/phpunit/includes/RevisionDbTestBase.php b/tests/phpunit/includes/RevisionDbTestBase.php index ff4c198aec..c760b41e05 100644 --- a/tests/phpunit/includes/RevisionDbTestBase.php +++ b/tests/phpunit/includes/RevisionDbTestBase.php @@ -1452,14 +1452,14 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase { Revision::DELETED_TEXT, Revision::DELETED_TEXT, [ 'sysop' ], - Title::newFromText( __METHOD__ ), + __METHOD__, true, ]; yield [ Revision::DELETED_TEXT, Revision::DELETED_TEXT, [], - Title::newFromText( __METHOD__ ), + __METHOD__, false, ]; } @@ -1469,6 +1469,8 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase { * @covers Revision::userCanBitfield */ public function testUserCanBitfield( $bitField, $field, $userGroups, $title, $expected ) { + $title = Title::newFromText( $title ); + $this->setMwGlobals( 'wgGroupPermissions', [ diff --git a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php index 0e0d609a6e..189b79b880 100644 --- a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php +++ b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php @@ -530,8 +530,26 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase { return $rev; } + /** + * @param int $id + * @return Title + */ + private function getMockTitle( $id = 23 ) { + $mock = $this->getMockBuilder( Title::class ) + ->disableOriginalConstructor() + ->getMock(); + $mock->expects( $this->any() ) + ->method( 'getDBkey' ) + ->will( $this->returnValue( __CLASS__ ) ); + $mock->expects( $this->any() ) + ->method( 'getArticleID' ) + ->will( $this->returnValue( $id ) ); + + return $mock; + } + public function provideIsReusableFor() { - $title = Title::makeTitleSafe( NS_MAIN, __METHOD__ ); + $title = $this->getMockTitle(); $user1 = User::newFromName( 'Alice' ); $user2 = User::newFromName( 'Bob' ); diff --git a/tests/phpunit/includes/Storage/RevisionRecordTests.php b/tests/phpunit/includes/Storage/RevisionRecordTests.php index 30dacdb7ac..eb048a7edc 100644 --- a/tests/phpunit/includes/Storage/RevisionRecordTests.php +++ b/tests/phpunit/includes/Storage/RevisionRecordTests.php @@ -343,14 +343,14 @@ trait RevisionRecordTests { RevisionRecord::DELETED_TEXT, RevisionRecord::DELETED_TEXT, [ 'sysop' ], - Title::newFromText( __METHOD__ ), + __METHOD__, true, ]; yield [ RevisionRecord::DELETED_TEXT, RevisionRecord::DELETED_TEXT, [], - Title::newFromText( __METHOD__ ), + __METHOD__, false, ]; } @@ -360,6 +360,11 @@ trait RevisionRecordTests { * @covers \MediaWiki\Storage\RevisionRecord::userCanBitfield */ public function testUserCanBitfield( $bitField, $field, $userGroups, $title, $expected ) { + if ( is_string( $title ) ) { + // NOTE: Data providers cannot instantiate Title objects! See T202641. + $title = Title::newFromText( $title ); + } + $this->forceStandardPermissions(); $user = $this->getTestUser( $userGroups )->getUser(); @@ -371,72 +376,75 @@ trait RevisionRecordTests { } public function provideHasSameContent() { - /** - * @param SlotRecord[] $slots - * @param int $revId - * @return RevisionStoreRecord - */ - $recordCreator = function ( array $slots, $revId ) { - $title = Title::newFromText( 'provideHasSameContent' ); - $title->resetArticleID( 19 ); - $slots = new RevisionSlots( $slots ); - - return new RevisionStoreRecord( - $title, - new UserIdentityValue( 11, __METHOD__, 0 ), - CommentStoreComment::newUnsavedComment( __METHOD__ ), - (object)[ - 'rev_id' => strval( $revId ), - 'rev_page' => strval( $title->getArticleID() ), - 'rev_timestamp' => '20200101000000', - 'rev_deleted' => 0, - 'rev_minor_edit' => 0, - 'rev_parent_id' => '5', - 'rev_len' => $slots->computeSize(), - 'rev_sha1' => $slots->computeSha1(), - 'page_latest' => '18', - ], - $slots - ); - }; - // Create some slots with content $mainA = SlotRecord::newUnsaved( 'main', new TextContent( 'A' ) ); $mainB = SlotRecord::newUnsaved( 'main', new TextContent( 'B' ) ); $auxA = SlotRecord::newUnsaved( 'aux', new TextContent( 'A' ) ); $auxB = SlotRecord::newUnsaved( 'aux', new TextContent( 'A' ) ); - $initialRecord = $recordCreator( [ $mainA ], 12 ); + $initialRecordSpec = [ [ $mainA ], 12 ]; return [ 'same record object' => [ true, - $initialRecord, - $initialRecord, + $initialRecordSpec, + $initialRecordSpec, ], 'same record content, different object' => [ true, - $recordCreator( [ $mainA ], 12 ), - $recordCreator( [ $mainA ], 13 ), + [ [ $mainA ], 12 ], + [ [ $mainA ], 13 ], ], 'same record content, aux slot, different object' => [ true, - $recordCreator( [ $auxA ], 12 ), - $recordCreator( [ $auxB ], 13 ), + [ [ $auxA ], 12 ], + [ [ $auxB ], 13 ], ], 'different content' => [ false, - $recordCreator( [ $mainA ], 12 ), - $recordCreator( [ $mainB ], 13 ), + [ [ $mainA ], 12 ], + [ [ $mainB ], 13 ], ], 'different content and number of slots' => [ false, - $recordCreator( [ $mainA ], 12 ), - $recordCreator( [ $mainA, $mainB ], 13 ), + [ [ $mainA ], 12 ], + [ [ $mainA, $mainB ], 13 ], ], ]; } + /** + * @note Do not call directly from a data provider! Data providers cannot instantiate + * Title objects! See T202641. + * + * @param SlotRecord[] $slots + * @param int $revId + * @return RevisionStoreRecord + */ + private function makeHasSameContentTestRecord( array $slots, $revId ) { + $title = Title::newFromText( 'provideHasSameContent' ); + $title->resetArticleID( 19 ); + $slots = new RevisionSlots( $slots ); + + return new RevisionStoreRecord( + $title, + new UserIdentityValue( 11, __METHOD__, 0 ), + CommentStoreComment::newUnsavedComment( __METHOD__ ), + (object)[ + 'rev_id' => strval( $revId ), + 'rev_page' => strval( $title->getArticleID() ), + 'rev_timestamp' => '20200101000000', + 'rev_deleted' => 0, + 'rev_minor_edit' => 0, + 'rev_parent_id' => '5', + 'rev_len' => $slots->computeSize(), + 'rev_sha1' => $slots->computeSha1(), + 'page_latest' => '18', + ], + $slots + ); + } + /** * @dataProvider provideHasSameContent * @covers \MediaWiki\Storage\RevisionRecord::hasSameContent @@ -444,9 +452,12 @@ trait RevisionRecordTests { */ public function testHasSameContent( $expected, - RevisionRecord $record1, - RevisionRecord $record2 + $recordSpec1, + $recordSpec2 ) { + $record1 = $this->makeHasSameContentTestRecord( ...$recordSpec1 ); + $record2 = $this->makeHasSameContentTestRecord( ...$recordSpec2 ); + $this->assertSame( $expected, $record1->hasSameContent( $record2 ) diff --git a/tests/phpunit/includes/api/ApiQuerySearchTest.php b/tests/phpunit/includes/api/ApiQuerySearchTest.php index 0700cf77de..708ebc52b8 100644 --- a/tests/phpunit/includes/api/ApiQuerySearchTest.php +++ b/tests/phpunit/includes/api/ApiQuerySearchTest.php @@ -10,22 +10,22 @@ class ApiQuerySearchTest extends ApiTestCase { 'empty search result' => [ [], [] ], 'has search results' => [ [ 'Zomg' ], - [ $this->mockResult( 'Zomg' ) ], + [ $this->mockResultClosure( 'Zomg' ) ], ], 'filters broken search results' => [ [ 'A', 'B' ], [ - $this->mockResult( 'a' ), - $this->mockResult( 'Zomg' )->setBrokenTitle( true ), - $this->mockResult( 'b' ), + $this->mockResultClosure( 'a' ), + $this->mockResultClosure( 'Zomg', [ 'setBrokenTitle' => true ] ), + $this->mockResultClosure( 'b' ), ], ], 'filters results with missing revision' => [ [ 'B', 'A' ], [ - $this->mockResult( 'Zomg' )->setMissingRevision( true ), - $this->mockResult( 'b' ), - $this->mockResult( 'a' ), + $this->mockResultClosure( 'Zomg', [ 'setMissingRevision' => true ] ), + $this->mockResultClosure( 'b' ), + $this->mockResultClosure( 'a' ), ], ], ]; @@ -56,7 +56,10 @@ class ApiQuerySearchTest extends ApiTestCase { [ SearchResultSet::SECONDARY_RESULTS => [ 'utwiki' => new MockSearchResultSet( [ - $this->mockResult( 'Qwerty' )->setInterwikiPrefix( 'utwiki' ), + $this->mockResultClosure( + 'Qwerty', + [ 'setInterwikiPrefix' => 'utwiki' ] + ), ] ), ], ] @@ -102,8 +105,28 @@ class ApiQuerySearchTest extends ApiTestCase { ] ); } - private function mockResult( $title ) { - return MockSearchResult::newFromtitle( Title::newFromText( $title ) ); + /** + * Returns a closure that evaluates to a MockSearchResult, to be resolved by + * MockSearchEngine::addMockResults() or MockresultSet::extractResults(). + * + * This is needed because MockSearchResults cannot be instantiated in a data provider, + * since they load revisions. This would hit the "real" database instead of the mock + * database, which in turn may cause cache pollution and other inconsistencies, see T202641. + * + * @param string $title + * @param array $setters + * @return callable function(): MockSearchResult + */ + private function mockResultClosure( $title, $setters = [] ) { + return function () use ( $title, $setters ){ + $result = MockSearchResult::newFromTitle( Title::newFromText( $title ) ); + + foreach ( $setters as $method => $param ) { + $result->$method( $param ); + } + + return $result; + }; } } diff --git a/tests/phpunit/includes/diff/DifferenceEngineTest.php b/tests/phpunit/includes/diff/DifferenceEngineTest.php index 333623562d..07d02dd24a 100644 --- a/tests/phpunit/includes/diff/DifferenceEngineTest.php +++ b/tests/phpunit/includes/diff/DifferenceEngineTest.php @@ -343,12 +343,30 @@ class DifferenceEngineTest extends MediaWikiTestCase { trim( strip_tags( $diff ), "\n" ) ); } + /** + * @param int $id + * @return Title + */ + private function getMockTitle( $id = 23 ) { + $mock = $this->getMockBuilder( Title::class ) + ->disableOriginalConstructor() + ->getMock(); + $mock->expects( $this->any() ) + ->method( 'getDBkey' ) + ->will( $this->returnValue( __CLASS__ ) ); + $mock->expects( $this->any() ) + ->method( 'getArticleID' ) + ->will( $this->returnValue( $id ) ); + + return $mock; + } + /** * @param SlotRecord[] $slots * @return MutableRevisionRecord */ private function getRevisionRecord( ...$slots ) { - $title = Title::newFromText( 'Foo' ); + $title = $this->getMockTitle(); $revision = new MutableRevisionRecord( $title ); foreach ( $slots as $slot ) { $revision->setSlot( $slot ); diff --git a/tests/phpunit/mocks/search/MockSearchEngine.php b/tests/phpunit/mocks/search/MockSearchEngine.php index 2b7ea4760a..207ac28dc4 100644 --- a/tests/phpunit/mocks/search/MockSearchEngine.php +++ b/tests/phpunit/mocks/search/MockSearchEngine.php @@ -19,12 +19,17 @@ class MockSearchEngine extends SearchEngine { * @param SearchResult[] $results The results to return for $query */ public static function addMockResults( $query, array $results ) { - self::$results[$query] = $results; $lc = MediaWikiServices::getInstance()->getLinkCache(); - foreach ( $results as $result ) { + foreach ( $results as &$result ) { + // Resolve deferred results; needed to work around T203279 + if ( is_callable( $result ) ) { + $result = $result(); + } + // TODO: better page ids? Does it matter? $lc->addGoodLinkObj( mt_rand(), $result->getTitle() ); } + self::$results[$query] = $results; } /** diff --git a/tests/phpunit/mocks/search/MockSearchResultSet.php b/tests/phpunit/mocks/search/MockSearchResultSet.php index 20e2a9fb68..38f6731e5b 100644 --- a/tests/phpunit/mocks/search/MockSearchResultSet.php +++ b/tests/phpunit/mocks/search/MockSearchResultSet.php @@ -8,8 +8,8 @@ class MockSearchResultSet extends SearchResultSet { private $interwikiResults; /** - * @param SearchResult[] $results - * @param SearchResultSet[][] $interwikiResults Map from result type + * @param SearchResult[]|callable[] $results + * @param SearchResultSet[][]|callable[][] $interwikiResults Map from result type * to list of results for that type. */ public function __construct( array $results, array $interwikiResults = [] ) { @@ -27,6 +27,19 @@ class MockSearchResultSet extends SearchResultSet { count( $this->interwikiResults[$type] ) > 0; } + public function extractResults() { + $results = parent::extractResults(); + + foreach ( $results as &$result ) { + // Resolve deferred results; needed to work around T203279 + if ( is_callable( $result ) ) { + $result = $result(); + } + } + + return $results; + } + public function getInterwikiResults( $type = self::SECONDARY_RESULTS ) { if ( $this->hasInterwikiResults( $type ) ) { return $this->interwikiResults[$type]; -- 2.20.1