Avoid constructing Title objects in data providers
authordaniel <daniel.kinzler@wikimedia.de>
Fri, 31 Aug 2018 04:56:42 +0000 (14:56 +1000)
committerDaniel Kinzler <daniel.kinzler@wikimedia.de>
Mon, 3 Sep 2018 16:36:49 +0000 (16:36 +0000)
Bug: T202641
Change-Id: I34efa0b9329e740bcb292b2529ec8f7f925dc346

tests/phpunit/includes/RevisionDbTestBase.php
tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
tests/phpunit/includes/Storage/RevisionRecordTests.php
tests/phpunit/includes/api/ApiQuerySearchTest.php
tests/phpunit/includes/diff/DifferenceEngineTest.php
tests/phpunit/mocks/search/MockSearchEngine.php
tests/phpunit/mocks/search/MockSearchResultSet.php

index ff4c198..c760b41 100644 (file)
@@ -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',
                        [
index 0e0d609..189b79b 100644 (file)
@@ -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' );
index 30dacdb..eb048a7 100644 (file)
@@ -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 )
index 0700cf7..708ebc5 100644 (file)
@@ -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;
+               };
        }
 
 }
index 3336235..07d02dd 100644 (file)
@@ -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 );
index 2b7ea47..207ac28 100644 (file)
@@ -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;
        }
 
        /**
index 20e2a9f..38f6731 100644 (file)
@@ -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];