Don't reset name tables between test runs.
authordaniel <daniel.kinzler@wikimedia.de>
Wed, 22 Aug 2018 16:02:49 +0000 (18:02 +0200)
committerdaniel <daniel.kinzler@wikimedia.de>
Thu, 23 Aug 2018 17:15:33 +0000 (19:15 +0200)
Resetting the content_model and slot_role tables between test runs
requires the corresponding NameTabelStore instances to be reset
as well. We may however have many of them, buried in various services.
There is no easy way to reset them consistently.

Letting information in these tables persist between tests seems
harmless. Tests that need these tables reset can simply add them
to the tablesUsed array.

This is needed for unit tests to work with the new MCR schema.

Bug: T198561
Change-Id: I63e61e1ab74e00c20930a83d3a3f5df53092a197

includes/Storage/NameTableStore.php
tests/phpunit/MediaWikiTestCase.php
tests/phpunit/includes/Storage/NameTableStoreTest.php
tests/phpunit/includes/Storage/PageUpdaterTest.php
tests/phpunit/includes/api/ApiBlockTest.php

index 52e8f5b..6c7919d 100644 (file)
@@ -27,7 +27,6 @@ use Wikimedia\Assert\Assert;
 use Wikimedia\Rdbms\Database;
 use Wikimedia\Rdbms\IDatabase;
 use Wikimedia\Rdbms\ILoadBalancer;
-use Wikimedia\Rdbms\LoadBalancer;
 
 /**
  * @author Addshore
@@ -35,7 +34,7 @@ use Wikimedia\Rdbms\LoadBalancer;
  */
 class NameTableStore {
 
-       /** @var LoadBalancer */
+       /** @var ILoadBalancer */
        private $loadBalancer;
 
        /** @var WANObjectCache */
@@ -159,11 +158,13 @@ class NameTableStore {
                if ( $searchResult === false ) {
                        $id = $this->store( $name );
                        if ( $id === null ) {
-                               // RACE: $name was already in the db, probably just inserted, so load from master
-                               // Use DBO_TRX to avoid missing inserts due to other threads or REPEATABLE-READs
-                               $table = $this->loadTable(
-                                       $this->getDBConnection( DB_MASTER, LoadBalancer::CONN_TRX_AUTOCOMMIT )
-                               );
+                               // RACE: $name was already in the db, probably just inserted, so load from master.
+                               // Use DBO_TRX to avoid missing inserts due to other threads or REPEATABLE-READs.
+                               // ...but not during unit tests, because we need the fake DB tables of the default
+                               // connection.
+                               $connFlags = defined( 'MW_PHPUNIT_TEST' ) ? 0 : ILoadBalancer::CONN_TRX_AUTOCOMMIT;
+                               $table = $this->reloadMap( $connFlags );
+
                                $searchResult = array_search( $name, $table, true );
                                if ( $searchResult === false ) {
                                        // Insert failed due to IGNORE flag, but DB_MASTER didn't give us the data
@@ -172,14 +173,15 @@ class NameTableStore {
                                        $this->logger->error( $m );
                                        throw new NameTableAccessException( $m );
                                }
-                               $this->purgeWANCache(
-                                       function () {
-                                               $this->cache->reap( $this->getCacheKey(), INF );
-                                       }
-                               );
+                       } elseif ( isset( $table[$id] ) ) {
+                               throw new NameTableAccessException(
+                                       "Expected unused ID from database insert for '$name' "
+                                       . " into '{$this->table}', but ID $id is already associated with"
+                                       . " the name '{$table[$id]}'! This may indicate database corruption!" );
                        } else {
                                $table[$id] = $name;
                                $searchResult = $id;
+
                                // As store returned an ID we know we inserted so delete from WAN cache
                                $this->purgeWANCache(
                                        function () {
@@ -193,6 +195,31 @@ class NameTableStore {
                return $searchResult;
        }
 
+       /**
+        * Reloads the name table from the master database, and purges the WAN cache entry.
+        *
+        * @note This should only be called in situations where the local cache has been detected
+        * to be out of sync with the database. There should be no reason to call this method
+        * from outside the NameTabelStore during normal operation. This method may however be
+        * useful in unit tests.
+        *
+        * @param int $connFlags ILoadBalancer::CONN_XXX flags. Optional.
+        *
+        * @return \string[] The freshly reloaded name map
+        */
+       public function reloadMap( $connFlags = 0 ) {
+               $this->tableCache = $this->loadTable(
+                       $this->getDBConnection( DB_MASTER, $connFlags )
+               );
+               $this->purgeWANCache(
+                       function () {
+                               $this->cache->reap( $this->getCacheKey(), INF );
+                       }
+               );
+
+               return $this->tableCache;
+       }
+
        /**
         * Get the id of the given name.
         * If the name doesn't exist this will throw.
index 6972165..34f93ad 100644 (file)
@@ -8,6 +8,7 @@ use Psr\Log\LoggerInterface;
 use Wikimedia\Rdbms\IDatabase;
 use Wikimedia\Rdbms\IMaintainableDatabase;
 use Wikimedia\Rdbms\Database;
+use Wikimedia\Rdbms\IResultWrapper;
 use Wikimedia\Rdbms\LBFactory;
 use Wikimedia\TestingAccessWrapper;
 
@@ -1732,10 +1733,14 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
         */
        private function resetDB( $db, $tablesUsed ) {
                if ( $db ) {
+                       // NOTE: Do not reset the slot_roles and content_models tables, but let them
+                       // leak across tests. Resetting them would require to reset all NamedTableStore
+                       // instances for these tables, of which there may be several beyond the ones
+                       // known to MediaWikiServices. See T202641.
                        $userTables = [ 'user', 'user_groups', 'user_properties', 'actor' ];
                        $pageTables = [
                                'page', 'revision', 'ip_changes', 'revision_comment_temp', 'comment', 'archive',
-                               'revision_actor_temp', 'slots', 'content', 'content_models', 'slot_roles',
+                               'revision_actor_temp', 'slots', 'content',
                        ];
                        $coreDBDataTables = array_merge( $userTables, $pageTables );
 
@@ -1761,41 +1766,8 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                                }
                        }
 
-                       $truncate = in_array( $db->getType(), [ 'oracle', 'mysql' ] );
                        foreach ( $tablesUsed as $tbl ) {
-                               if ( !$db->tableExists( $tbl ) ) {
-                                       continue;
-                               }
-
-                               if ( $truncate ) {
-                                       $db->query( 'TRUNCATE TABLE ' . $db->tableName( $tbl ), __METHOD__ );
-                               } else {
-                                       $db->delete( $tbl, '*', __METHOD__ );
-                               }
-
-                               if ( in_array( $db->getType(), [ 'postgres', 'sqlite' ], true ) ) {
-                                       // Reset the table's sequence too.
-                                       $db->resetSequenceForTable( $tbl, __METHOD__ );
-                               }
-
-                               if ( $tbl === 'interwiki' ) {
-                                       if ( !$this->interwikiTable ) {
-                                               // @todo We should probably throw here, but this causes test failures that I
-                                               // can't figure out, so for now we silently continue.
-                                               continue;
-                                       }
-                                       $db->insert(
-                                               'interwiki',
-                                               array_values( array_map( 'get_object_vars', iterator_to_array( $this->interwikiTable ) ) ),
-                                               __METHOD__
-                                       );
-                               }
-
-                               if ( $tbl === 'page' ) {
-                                       // Forget about the pages since they don't
-                                       // exist in the DB.
-                                       MediaWikiServices::getInstance()->getLinkCache()->clear();
-                               }
+                               $this->truncateTable( $tbl, $db );
                        }
 
                        if ( array_intersect( $tablesUsed, $coreDBDataTables ) ) {
@@ -1805,6 +1777,56 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
                }
        }
 
+       /**
+        * Empties the given table and resets any auto-increment counters.
+        * Will also purge caches associated with some well known tables.
+        * If the table is not know, this method just returns.
+        *
+        * @param string $tableName
+        * @param IDatabase|null $db
+        */
+       protected function truncateTable( $tableName, IDatabase $db = null ) {
+               if ( !$db ) {
+                       $db = $this->db;
+               }
+
+               if ( !$db->tableExists( $tableName ) ) {
+                       return;
+               }
+
+               $truncate = in_array( $db->getType(), [ 'oracle', 'mysql' ] );
+
+               if ( $truncate ) {
+                       $db->query( 'TRUNCATE TABLE ' . $db->tableName( $tableName ), __METHOD__ );
+               } else {
+                       $db->delete( $tableName, '*', __METHOD__ );
+               }
+
+               if ( in_array( $db->getType(), [ 'postgres', 'sqlite' ], true ) ) {
+                       // Reset the table's sequence too.
+                       $db->resetSequenceForTable( $tableName, __METHOD__ );
+               }
+
+               if ( $tableName === 'interwiki' ) {
+                       if ( !$this->interwikiTable ) {
+                               // @todo We should probably throw here, but this causes test failures that I
+                               // can't figure out, so for now we silently continue.
+                               return;
+                       }
+                       $db->insert(
+                               'interwiki',
+                               array_values( array_map( 'get_object_vars', iterator_to_array( $this->interwikiTable ) ) ),
+                               __METHOD__
+                       );
+               }
+
+               if ( $tableName === 'page' ) {
+                       // Forget about the pages since they don't
+                       // exist in the DB.
+                       MediaWikiServices::getInstance()->getLinkCache()->clear();
+               }
+       }
+
        private static function unprefixTable( &$tableName, $ind, $prefix ) {
                $tableName = substr( $tableName, strlen( $prefix ) );
        }
index b5b2e0d..1517964 100644 (file)
@@ -26,6 +26,10 @@ class NameTableStoreTest extends MediaWikiTestCase {
                parent::setUp();
        }
 
+       protected function addCoreDBData() {
+               // The default implementation causes the slot_roles to already have content. Skip that.
+       }
+
        private function populateTable( $values ) {
                $insertValues = [];
                foreach ( $values as $name ) {
@@ -139,6 +143,9 @@ class NameTableStoreTest extends MediaWikiTestCase {
                $name,
                $expectedId
        ) {
+               // Make sure the table is empty!
+               $this->truncateTable( 'slot_roles' );
+
                $this->populateTable( $existingValues );
                $store = $this->getNameTableSqlStore( $cacheBag, (int)$needsInsert, $selectCalls );
 
@@ -266,6 +273,21 @@ class NameTableStoreTest extends MediaWikiTestCase {
                $this->assertSame( $expected, TestingAccessWrapper::newFromObject( $store )->tableCache );
        }
 
+       public function testReloadMap() {
+               $this->populateTable( [ 'foo' ] );
+               $store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 2 );
+
+               // force load
+               $this->assertCount( 1, $store->getMap() );
+
+               // add more stuff to the table, so the cache gets out of sync
+               $this->populateTable( [ 'bar' ] );
+
+               $expected = [ 1 => 'foo', 2 => 'bar' ];
+               $this->assertSame( $expected, $store->reloadMap() );
+               $this->assertSame( $expected, $store->getMap() );
+       }
+
        public function testCacheRaceCondition() {
                $wanHashBag = new HashBagOStuff();
                $store1 = $this->getNameTableSqlStore( $wanHashBag, 1, 1 );
index f01c6ba..758537d 100644 (file)
@@ -19,6 +19,13 @@ use WikiPage;
  */
 class PageUpdaterTest extends MediaWikiTestCase {
 
+       public static function setUpBeforeClass() {
+               parent::setUpBeforeClass();
+
+               // force service reset!
+               MediaWikiServices::getInstance()->resetServiceForTesting( 'RevisionStore' );
+       }
+
        private function getDummyTitle( $method ) {
                return Title::newFromText( $method, $this->getDefaultWikitextNS() );
        }
@@ -217,6 +224,7 @@ class PageUpdaterTest extends MediaWikiTestCase {
 
                // check site stats - this asserts that derived data updates where run.
                $stats = $this->db->selectRow( 'site_stats', '*', '1=1' );
+               $this->assertNotNull( $stats, 'site_stats' );
                $this->assertSame( $oldStats->ss_total_pages + 0, (int)$stats->ss_total_pages );
                $this->assertSame( $oldStats->ss_total_edits + 2, (int)$stats->ss_total_edits );
        }
index 374ea3c..65adedc 100644 (file)
@@ -125,10 +125,10 @@ class ApiBlockTest extends ApiTestCase {
                $this->doBlock( [ 'tags' => 'custom tag' ] );
 
                $dbw = wfGetDB( DB_MASTER );
-               $this->assertSame( 'custom tag', $dbw->selectField(
+               $this->assertSame( 1, (int)$dbw->selectField(
                        [ 'change_tag', 'logging' ],
-                       'ct_tag',
-                       [ 'log_type' => 'block' ],
+                       'COUNT(*)',
+                       [ 'log_type' => 'block', 'ct_tag' => 'custom tag' ],
                        __METHOD__,
                        [],
                        [ 'change_tag' => [ 'INNER JOIN', 'ct_log_id = log_id' ] ]