r105154 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r105153‎ | r105154 | r105155 >
Date:07:22, 5 December 2011
Author:aaron
Status:resolved (Comments)
Tags:
Comment:
* Made resetDB() actually clear out the tables rather than leave the contents
* Removed destroyDB(); unused and broken (does not use prefix)
Modified paths:
  • /trunk/phase3/tests/phpunit/MediaWikiTestCase.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/MediaWikiTestCase.php
——@@ -13,6 +13,7 @@
1414 protected $useTemporaryTables = true;
1515 protected $reuseDB = false;
1616 private static $dbSetup = false;
 17+ private static $tablesCloned = array();
1718
1819 /**
1920 * Table name prefixes. Oracle likes it shorter.
——@@ -54,7 +55,7 @@
5556 $this->oldTablePrefix = $wgDBprefix;
5657
5758 if( !self::$dbSetup ) {
58 - $this->initDB();
 59+ self::$tablesCloned = $this->initDB();
5960 self::$dbSetup = true;
6061 }
6162
——@@ -140,7 +141,8 @@
141142 throw new MWException( 'Cannot run unit tests, the database prefix is already "unittest_"' );
142143 }
143144
144 - $dbClone = new CloneDatabase( $this->db, $this->listTables(), $this->dbPrefix() );
 145+ $tablesCloned = $this->listTables();
 146+ $dbClone = new CloneDatabase( $this->db, $tablesCloned, $this->dbPrefix() );
145147 $dbClone->useTemporaryTables( $this->useTemporaryTables );
146148
147149 if ( ( $this->db->getType() == 'oracle' || !$this->useTemporaryTables ) && $this->reuseDB ) {
——@@ -154,6 +156,8 @@
155157 if ( $this->db->getType() == 'oracle' ) {
156158 $this->db->query( 'BEGIN FILL_WIKI_INFO; END;' );
157159 }
 160+
 161+ return $tablesCloned;
158162 }
159163
160164 /**
——@@ -166,44 +170,20 @@
167171 wfGetLB()->closeAll();
168172 $this->db = wfGetDB( DB_MASTER );
169173 } else {
170 - foreach( $this->listTables() as $tbl ) {
 174+ foreach( self::$tablesCloned as $tbl ) {
171175 if( $tbl == 'interwiki') continue;
172176 $this->db->query( 'TRUNCATE TABLE '.$this->db->tableName($tbl), __METHOD__ );
173177 }
174178 }
175179 } else {
176 - foreach( $this->listTables() as $tbl ) {
 180+ foreach( self::$tablesCloned as $tbl ) {
177181 if( $tbl == 'interwiki' || $tbl == 'user' ) continue;
178 - $this->db->delete( $tbl, '*', __METHOD__ );
 182+ $this->db->delete( $tbl, '*', __METHOD__ );
179183 }
180184 }
181185 }
182186 }
183187
184 - protected function destroyDB() {
185 - if ( is_null( $this->db ) ||
186 - ( $this->useTemporaryTables && $this->db->getType() != 'oracle' ) ||
187 - ( $this->reuseDB ) ) {
188 - # Don't need to do anything
189 - return;
190 - }
191 -
192 - $tables = $this->db->listTables( $this->dbPrefix(), __METHOD__ );
193 -
194 - foreach ( $tables as $table ) {
195 - try {
196 - $sql = $this->db->getType() == 'oracle' ? "DROP TABLE $table CASCADE CONSTRAINTS PURGE" : "DROP TABLE `$table`";
197 - $this->db->query( $sql, __METHOD__ );
198 - } catch( MWException $mwe ) {}
199 - }
200 -
201 - if ( $this->db->getType() == 'oracle' )
202 - $this->db->query( 'BEGIN FILL_WIKI_INFO; END;', __METHOD__ );
203 -
204 - CloneDatabase::changePrefix( $this->oldTablePrefix );
205 - }
206 -
207 -
208188 function __call( $func, $args ) {
209189 static $compatibility = array(
210190 'assertInternalType' => 'assertType',

Sign-offs

UserFlagDate
Nikerabbitinspected16:16, 5 December 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r105752FU r105154: only clear tables that tests specify as written to. This reduces ...aaron05:47, 10 December 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   07:38, 5 December 2011

To clarify, the "foreach( self::$tablesCloned as $tbl ) {" was iterating over any empty array so nothing was cleared.

#Comment by Brion VIBBER (talk | contribs)   18:55, 6 December 2011

that line appears to be in new code, not old code. Was this the foreach($tables as $table) in destroyDB()?

It kinda looks like that's only needed on certain database types, and normally we'd use temporary tables which drop themselves... was that code even being reached on MySQL / SQLite? Or did we stop using temporary tables?

#Comment by Aaron Schulz (talk | contribs)   19:01, 6 December 2011

Opps, I mean the old line in resetDB(), "foreach( $this->listTables() as $tbl ) {".

#Comment by Platonides (talk | contribs)   19:15, 9 December 2011

This produces an important slowdown in my main checkout:

r105153r105154
make databaseless0m10.755s / 0m9.516s / 0m0.750s0m10.938s / 0m9.733s / 0m0.740s
make database9m12.853s / 7m37.064s / 0m36.374s51m7.904s / 19m54.749s / 1m55.319s
make destructive8m59.431s / 7m40.353s / 0m12.339s52m38.923s / 19m48.659s / 0m40.247s[1]


  1. ↑ I also had slower results, such as 71m40.636s / 22m0.081s / 0m49.227s in r105292, but there could be other factors there, such as a cold cache
#Comment by Aaron Schulz (talk | contribs)   22:26, 9 December 2011

I just fixed the code not doing what it what it claimed to. I was trying not to get too sucked into this mess.

It would help to detect (or make tests list out) the table changed and only delete those ones.

Status & tagging log