r76617 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r76616‎ | r76617 | r76618 >
Date:08:32, 13 November 2010
Author:maxsem
Status:deferred (Comments)
Tags:
Comment:
Tests: removed assignments of null to globals that are usually not null in tearDown() methods. This practice leads to other tests having to play whack-a-mole by initializing random variables they don't need directly just to avoid crashing.
Modified paths:
  • /trunk/phase3/maintenance/tests/phpunit/includes/ExtraParserTest.php (modified) (history)
  • /trunk/phase3/maintenance/tests/phpunit/includes/LanguageConverterTest.php (modified) (history)
  • /trunk/phase3/maintenance/tests/phpunit/includes/TitlePermissionTest.php (modified) (history)
  • /trunk/phase3/maintenance/tests/phpunit/includes/api/ApiSetup.php (modified) (history)
  • /trunk/phase3/maintenance/tests/phpunit/includes/api/ApiUploadTest.php (modified) (history)
  • /trunk/phase3/maintenance/tests/phpunit/includes/search/SearchUpdateTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/phpunit/includes/TitlePermissionTest.php
——@@ -50,11 +50,6 @@
5151 }
5252 }
5353
54 - function tearDown() {
55 - global $wgContLang, $wgLang;
56 - $wgContLang = $wgLang = null;
57 - }
58 -
5954 function setUserPerm( $perm ) {
6055 if ( is_array( $perm ) ) {
6156 self::$user->mRights = $perm;
Index: trunk/phase3/maintenance/tests/phpunit/includes/ExtraParserTest.php
——@@ -15,12 +15,6 @@
1616 $wgMemc = new FakeMemCachedClient;
1717 }
1818
19 - function tearDown() {
20 - global $wgMemc;
21 -
22 - $wgMemc = null;
23 - }
24 -
2519 // Bug 8689 - Long numeric lines kill the parser
2620 function testBug8689() {
2721 global $wgLang;
Index: trunk/phase3/maintenance/tests/phpunit/includes/LanguageConverterTest.php
——@@ -21,7 +21,7 @@
2222 unset( $wgMemc );
2323 unset( $this->lc );
2424 unset( $this->lang );
25 - $wgContLang = null;
 25+ $wgContLang = Language::factory( 'en' );
2626 }
2727
2828 function testGetPreferredVariantDefaults() {
Index: trunk/phase3/maintenance/tests/phpunit/includes/search/SearchUpdateTest.php
——@@ -91,7 +91,6 @@
9292 $wgDBtype = self::$dbtype;
9393 $wgLBFactoryConf = self::$factoryconf;
9494 $wgDBservers = self::$dbservers;
95 - $wgContLang = null;
9695 }
9796
9897 function testUpdateText() {
Index: trunk/phase3/maintenance/tests/phpunit/includes/api/ApiSetup.php
——@@ -39,11 +39,6 @@
4040
4141 $GLOBALS['wgUser'] = self::$sysopUser->user;
4242 }
43 -
44 - function tearDown() {
45 - global $wgMemc;
46 - $wgMemc = null;
47 - }
4843 }
4944
5045 class UserWrapper {
Index: trunk/phase3/maintenance/tests/phpunit/includes/api/ApiUploadTest.php
——@@ -108,11 +108,6 @@
109109
110110 }
111111
112 - function tearDown() {
113 - global $wgMemc;
114 - $wgMemc = null;
115 - }
116 -
117112 protected function doApiRequest( $params, $session = null ) {
118113 $_SESSION = isset( $session ) ? $session : array();
119114

Follow-up revisions

RevisionCommit summaryAuthorDate
r76805Fixed SearchUpdateTest, it doesn't mess with database anymore. More fixes in ...maxsem16:06, 16 November 2010

Comments

#Comment by Platonides (talk | contribs)   18:11, 15 November 2010

I disagree. It forces the other tests to initialise the globals they use, so that they don't end up working only if run after tests A and B.

#Comment by MaxSem (talk | contribs)   18:19, 15 November 2010

No, with this scheme tests need to initialize only variables they need to change for testing. And they should behave nicely to restore them to default values in tearDown(). Your proposal is problematic because in our architecture there are lots of cross-dependant variables/functions so if we don't care about corrupt globals we'll end up initializing dozens of globals for every test, and things will keep breaking frequently anyway.

This commit actually fixed several tests that were failing on CC.

#Comment by Platonides (talk | contribs)   20:55, 15 November 2010

How are you going to assert that it changes what they modify in the tests?

The only way is to ensure that the objects are reloaded. See for instance r76703 which I just found when looking for examples. Despite changing $wgDBtype & co. the database handler was deeply cached. There are hidden dependancies everywhere. As another example, take the double unstubbing of $wgContLang from r74932. It shouldn't have happened and nobody foresaw it, but for months LocalisationCache was being recursively called.

I very much prefer that each test group initialises its globals, so they are in fresh state. Freeing them on teardown guarantees that next tests can't abuse those initializations.

Status & tagging log