From: daniel Date: Wed, 22 Aug 2018 16:02:49 +0000 (+0200) Subject: Don't reset name tables between test runs. X-Git-Tag: 1.34.0-rc.0~4322^2 X-Git-Url: http://git.cyclocoop.org/%28%28?a=commitdiff_plain;h=2c7e4adcea9e8f77acbfb86b822e1cea6134528b;p=lhc%2Fweb%2Fwiklou.git Don't reset name tables between test runs. 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 --- diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php index 52e8f5b993..6c7919d254 100644 --- a/includes/Storage/NameTableStore.php +++ b/includes/Storage/NameTableStore.php @@ -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. diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php index 69721655ce..34f93ad7a1 100644 --- a/tests/phpunit/MediaWikiTestCase.php +++ b/tests/phpunit/MediaWikiTestCase.php @@ -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 ) ); } diff --git a/tests/phpunit/includes/Storage/NameTableStoreTest.php b/tests/phpunit/includes/Storage/NameTableStoreTest.php index b5b2e0dd45..1517964471 100644 --- a/tests/phpunit/includes/Storage/NameTableStoreTest.php +++ b/tests/phpunit/includes/Storage/NameTableStoreTest.php @@ -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 ); diff --git a/tests/phpunit/includes/Storage/PageUpdaterTest.php b/tests/phpunit/includes/Storage/PageUpdaterTest.php index f01c6bac24..758537d35a 100644 --- a/tests/phpunit/includes/Storage/PageUpdaterTest.php +++ b/tests/phpunit/includes/Storage/PageUpdaterTest.php @@ -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 ); } diff --git a/tests/phpunit/includes/api/ApiBlockTest.php b/tests/phpunit/includes/api/ApiBlockTest.php index 374ea3cd6f..65adedcff8 100644 --- a/tests/phpunit/includes/api/ApiBlockTest.php +++ b/tests/phpunit/includes/api/ApiBlockTest.php @@ -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' ] ]