r91270 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r91269‎ | r91270 | r91271 >
Date:16:34, 1 July 2011
Author:demon
Status:ok (Comments)
Tags:
Comment:
Merge ApiTestSetup into ApiTestCase and update all subclasses. The amount of duplication here was nasty, and also lets us get rid of a bunch of useless require_once()s.

./phpunit.php --filter Api currently gives me: Tests: 24, Assertions: 107, Incomplete: 1, Skipped: 2.
Modified paths:
  • /trunk/phase3/tests/TestsAutoLoader.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiBlockTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiPurgeTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiQueryTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiSetup.php (deleted) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiTestCase.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiWatchTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/format/ApiFormatPhpTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/api/format/ApiFormatTestBase.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/upload/UploadFromUrlTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/upload/UploadFromUrlTest.php
——@@ -1,12 +1,10 @@
22 <?php
33
4 -require_once dirname( dirname( __FILE__ ) ) . '/api/ApiSetup.php';
5 -
64 /**
75 * @group Broken
86 * @group Upload
97 */
10 -class UploadFromUrlTest extends ApiTestSetup {
 8+class UploadFromUrlTest extends ApiTestCase {
119
1210 public function setUp() {
1311 global $wgEnableUploads, $wgAllowCopyUploads, $wgAllowAsyncCopyUploads;
Index: trunk/phase3/tests/phpunit/includes/api/ApiSetup.php
——@@ -1,96 +0,0 @@
2 -<?php
3 -
4 -abstract class ApiTestSetup extends MediaWikiLangTestCase {
5 - protected $user;
6 - protected $sysopUser;
7 - protected static $apiUrl;
8 -
9 - function setUp() {
10 - global $wgServer, $wgContLang, $wgAuth, $wgMemc, $wgRequest;
11 -
12 - parent::setUp();
13 - self::$apiUrl = $wgServer . wfScript( 'api' );
14 -
15 - $wgMemc = new EmptyBagOStuff;
16 - $wgContLang = Language::factory( 'en' );
17 - $wgAuth = new StubObject( 'wgAuth', 'AuthPlugin' );
18 - $wgRequest = new FauxRequest( array() );
19 - $this->setupUser();
20 - }
21 -
22 - protected function doApiRequest( $params, $data = null, $appendModule = false ) {
23 - $_SESSION = isset( $data[2] ) ? $data[2] : array();
24 -
25 - $req = new FauxRequest( $params, true, $_SESSION );
26 - $module = new ApiMain( $req, true );
27 - $module->execute();
28 -
29 - $data[0] = $module->getResultData();
30 - $data[1] = $req;
31 - $data[2] = $_SESSION;
32 -
33 - if( $appendModule ) $data[3] = $module;
34 -
35 - return $data;
36 - }
37 -
38 - function setupUser() {
39 - if ( $this->user == null || $this->sysopUser == null ) {
40 - $this->user = new UserWrapper( 'User for MediaWiki automated tests', User::randomPassword() );
41 - $this->sysopUser = new UserWrapper( 'Sysop for MediaWiki automated tests', User::randomPassword(), 'sysop' );
42 - }
43 -
44 - $GLOBALS['wgUser'] = $this->sysopUser->user;
45 - }
46 -
47 - function doLogin() {
48 - $data = $this->doApiRequest( array(
49 - 'action' => 'login',
50 - 'lgname' => $this->sysopUser->userName,
51 - 'lgpassword' => $this->sysopUser->password ) );
52 -
53 - $token = $data[0]['login']['token'];
54 -
55 - $data = $this->doApiRequest( array(
56 - 'action' => 'login',
57 - "lgtoken" => $token,
58 - "lgname" => $this->sysopUser->userName,
59 - "lgpassword" => $this->sysopUser->password ), $data );
60 -
61 - return $data;
62 - }
63 -
64 - function getTokenList( $user ) {
65 - $GLOBALS['wgUser'] = $user->user;
66 - $data = $this->doApiRequest( array(
67 - 'action' => 'query',
68 - 'titles' => 'Main Page',
69 - 'intoken' => 'edit|delete|protect|move|block|unblock',
70 - 'prop' => 'info' ) );
71 - return $data;
72 - }
73 -
74 -}
75 -
76 -class UserWrapper {
77 - public $userName, $password, $user;
78 -
79 - public function __construct( $userName, $password, $group = '' ) {
80 - $this->userName = $userName;
81 - $this->password = $password;
82 -
83 - $this->user = User::newFromName( $this->userName );
84 - if ( !$this->user->getID() ) {
85 - $this->user = User::createNew( $this->userName, array(
86 - "email" => "test@example.com",
87 - "real_name" => "Test User" ) );
88 - }
89 - $this->user->setPassword( $this->password );
90 -
91 - if ( $group !== '' ) {
92 - $this->user->addGroup( $group );
93 - }
94 - $this->user->saveSettings();
95 - }
96 -}
97 -
Index: trunk/phase3/tests/phpunit/includes/api/ApiTest.php
——@@ -1,25 +1,10 @@
22 <?php
33
4 -class MockApi extends ApiBase {
5 - public function execute() { }
6 - public function getVersion() { }
7 -
8 - public function __construct() { }
9 -
10 - public function getAllowedParams() {
11 - return array(
12 - 'filename' => null,
13 - 'enablechunks' => false,
14 - 'sessionkey' => null,
15 - );
16 - }
17 -}
18 -
194 /**
205 * @group Database
216 * @group Destructive
227 */
23 -class ApiTest extends ApiTestSetup {
 8+class ApiTest extends ApiTestCase {
249
2510 function testRequireOnlyOneParameterDefault() {
2611 $mock = new MockApi();
——@@ -78,7 +63,7 @@
7964 */
8065 function testApiLoginNoName() {
8166 $data = $this->doApiRequest( array( 'action' => 'login',
82 - 'lgname' => '', 'lgpassword' => $this->user->password,
 67+ 'lgname' => '', 'lgpassword' => self::$users['sysop']->password,
8368 ) );
8469 $this->assertEquals( 'NoName', $data[0]['login']['result'] );
8570 }
——@@ -86,14 +71,15 @@
8772 function testApiLoginBadPass() {
8873 global $wgServer;
8974
90 - $user = $this->user;
 75+ $user = self::$users['sysop'];
 76+ $user->user->logOut();
9177
9278 if ( !isset( $wgServer ) ) {
9379 $this->markTestIncomplete( 'This test needs $wgServer to be set in LocalSettings.php' );
9480 }
9581 $ret = $this->doApiRequest( array(
9682 "action" => "login",
97 - "lgname" => $user->userName,
 83+ "lgname" => $user->username,
9884 "lgpassword" => "bad",
9985 )
10086 );
——@@ -109,8 +95,8 @@
11096 $ret = $this->doApiRequest( array(
11197 "action" => "login",
11298 "lgtoken" => $token,
113 - "lgname" => $user->userName,
114 - "lgpassword" => "bad",
 99+ "lgname" => $user->username,
 100+ "lgpassword" => "badnowayinhell",
115101 )
116102 );
117103
——@@ -129,11 +115,12 @@
130116 $this->markTestIncomplete( 'This test needs $wgServer to be set in LocalSettings.php' );
131117 }
132118
133 - $user = $this->user;
 119+ $user = self::$users['sysop'];
 120+ $user->user->logOut();
134121
135122 $ret = $this->doApiRequest( array(
136123 "action" => "login",
137 - "lgname" => $user->userName,
 124+ "lgname" => $user->username,
138125 "lgpassword" => $user->password,
139126 )
140127 );
——@@ -149,7 +136,7 @@
150137 $ret = $this->doApiRequest( array(
151138 "action" => "login",
152139 "lgtoken" => $token,
153 - "lgname" => $user->userName,
 140+ "lgname" => $user->username,
154141 "lgpassword" => $user->password,
155142 )
156143 );
——@@ -170,11 +157,13 @@
171158 if ( !isset( $wgServer ) ) {
172159 $this->markTestIncomplete( 'This test needs $wgServer to be set in LocalSettings.php' );
173160 }
 161+ $user = self::$users['sysop'];
 162+
174163 $req = MWHttpRequest::factory( self::$apiUrl . "?action=login&format=xml",
175164 array( "method" => "POST",
176165 "postData" => array(
177 - "lgname" => $this->user->userName,
178 - "lgpassword" => $this->user->password ) ) );
 166+ "lgname" => $user->username,
 167+ "lgpassword" => $user->password ) ) );
179168 $req->execute();
180169
181170 libxml_use_internal_errors( true );
——@@ -189,8 +178,8 @@
190179
191180 $req->setData( array(
192181 "lgtoken" => $token,
193 - "lgname" => $this->user->userName,
194 - "lgpassword" => $this->user->password ) );
 182+ "lgname" => $user->username,
 183+ "lgpassword" => $user->password ) );
195184 $req->execute();
196185
197186 $cj = $req->getCookieJar();
——@@ -198,7 +187,7 @@
199188 $this->assertNotEquals( false, $serverName );
200189 $serializedCookie = $cj->serializeToHttpRequest( $wgScriptPath, $serverName );
201190 $this->assertNotEquals( '', $serializedCookie );
202 - $this->assertRegexp( '/_session=[^;]*; .*UserID=[0-9]*; .*UserName=' . $this->user->userName . '; .*Token=/', $serializedCookie );
 191+ $this->assertRegexp( '/_session=[^;]*; .*UserID=[0-9]*; .*UserName=' . $user->userName . '; .*Token=/', $serializedCookie );
203192
204193 return $cj;
205194 }
——@@ -225,10 +214,11 @@
226215 }
227216
228217 function testRunLogin() {
 218+ $sysopUser = self::$users['sysop'];
229219 $data = $this->doApiRequest( array(
230220 'action' => 'login',
231 - 'lgname' => $this->sysopUser->userName,
232 - 'lgpassword' => $this->sysopUser->password ) );
 221+ 'lgname' => $sysopUser->username,
 222+ 'lgpassword' => $sysopUser->password ) );
233223
234224 $this->assertArrayHasKey( "login", $data[0] );
235225 $this->assertArrayHasKey( "result", $data[0]['login'] );
——@@ -238,8 +228,8 @@
239229 $data = $this->doApiRequest( array(
240230 'action' => 'login',
241231 "lgtoken" => $token,
242 - "lgname" => $this->sysopUser->userName,
243 - "lgpassword" => $this->sysopUser->password ), $data );
 232+ "lgname" => $sysopUser->username,
 233+ "lgpassword" => $sysopUser->password ), $data );
244234
245235 $this->assertArrayHasKey( "login", $data[0] );
246236 $this->assertArrayHasKey( "result", $data[0]['login'] );
——@@ -250,7 +240,7 @@
251241 }
252242
253243 function testGettingToken() {
254 - foreach ( array( $this->user, $this->sysopUser ) as $user ) {
 244+ foreach ( self::$users as $user ) {
255245 $this->runTokenTest( $user );
256246 }
257247 }
Index: trunk/phase3/tests/phpunit/includes/api/ApiPurgeTest.php
——@@ -1,11 +1,9 @@
22 <?php
33
4 -require_once dirname( __FILE__ ) . '/ApiSetup.php';
5 -
64 /**
75 * @group Database
86 */
9 -class ApiPurgeTest extends ApiTestSetup {
 7+class ApiPurgeTest extends ApiTestCase {
108
119 function setUp() {
1210 parent::setUp();
Index: trunk/phase3/tests/phpunit/includes/api/ApiQueryTest.php
——@@ -1,11 +1,9 @@
22 <?php
33
4 -require_once dirname( __FILE__ ) . '/ApiSetup.php';
5 -
64 /**
75 * @group Database
86 */
9 -class ApiQueryTest extends ApiTestSetup {
 7+class ApiQueryTest extends ApiTestCase {
108
119 function setUp() {
1210 parent::setUp();
Index: trunk/phase3/tests/phpunit/includes/api/ApiWatchTest.php
——@@ -1,13 +1,11 @@
22 <?php
33
4 -require_once dirname( __FILE__ ) . '/ApiSetup.php';
5 -
64 /**
75 * @group Database
86 * @group Destructive
97 * @todo This test suite is severly broken and need a full review
108 */
11 -class ApiWatchTest extends ApiTestSetup {
 9+class ApiWatchTest extends ApiTestCase {
1210
1311 function setUp() {
1412 parent::setUp();
——@@ -15,7 +13,7 @@
1614 }
1715
1816 function getTokens() {
19 - return $this->getTokenList( $this->sysopUser );
 17+ return $this->getTokenList( self::$users['sysop'] );
2018 }
2119
2220 /**
Index: trunk/phase3/tests/phpunit/includes/api/format/ApiFormatPhpTest.php
——@@ -1,19 +1,10 @@
22 <?php
33
4 -require_once dirname( __FILE__ ) . '/ApiFormatTestBase.php';
5 -
64 /**
75 * @group API
86 * @group Database
97 */
108 class ApiFormatPhpTest extends ApiFormatTestBase {
11 -
12 - /*function setUp() {
13 - parent::setUp();
14 - $this->doLogin();
15 - }*/
16 -
17 -
189 function testValidPHPSyntax() {
1910
2011 $data = $this->apiRequest( 'php', array( 'action' => 'query', 'meta' => 'siteinfo' ) );
Index: trunk/phase3/tests/phpunit/includes/api/format/ApiFormatTestBase.php
——@@ -1,13 +1,9 @@
22 <?php
33
4 -require_once dirname( dirname( __FILE__ ) ) . '/ApiSetup.php';
5 -
6 -abstract class ApiFormatTestBase extends ApiTestSetup {
7 -
 4+abstract class ApiFormatTestBase extends ApiTestCase {
85 protected function apiRequest( $format, $params, $data = null ) {
9 -
106 $data = parent::doApiRequest( $params, $data, true );
11 -
 7+
128 $module = $data[3];
139
1410 $printer = $module->createPrinterByName( $format );
——@@ -23,9 +19,4 @@
2420
2521 return $out;
2622 }
27 -
28 - function setupUser() {
29 - /* Do not setup a user here */
30 - }
3123 }
32 -
Index: trunk/phase3/tests/phpunit/includes/api/ApiBlockTest.php
——@@ -1,12 +1,10 @@
22 <?php
33
4 -require_once dirname( __FILE__ ) . '/ApiSetup.php';
5 -
64 /**
75 * @group Database
86 * @group Destructive
97 */
10 -class ApiBlockTest extends ApiTestSetup {
 8+class ApiBlockTest extends ApiTestCase {
119
1210 function setUp() {
1311 parent::setUp();
——@@ -14,7 +12,7 @@
1513 }
1614
1715 function getTokens() {
18 - return $this->getTokenList( $this->sysopUser );
 16+ return $this->getTokenList( self::$users['sysop'] );
1917 }
2018
2119 function addDBData() {
Index: trunk/phase3/tests/phpunit/includes/api/ApiTestCase.php
——@@ -1,12 +1,17 @@
22 <?php
33
44 abstract class ApiTestCase extends MediaWikiLangTestCase {
 5+ /**
 6+ * @var Array of ApiTestUser
 7+ */
58 public static $users;
 9+ protected static $apiUrl;
610
711 function setUp() {
8 - global $wgContLang, $wgAuth, $wgMemc, $wgRequest, $wgUser;
 12+ global $wgContLang, $wgAuth, $wgMemc, $wgRequest, $wgUser, $wgServer;
913
1014 parent::setUp();
 15+ self::$apiUrl = $wgServer . wfScript( 'api' );
1116 $wgMemc = new EmptyBagOStuff();
1217 $wgContLang = Language::factory( 'en' );
1318 $wgAuth = new StubObject( 'wgAuth', 'AuthPlugin' );
——@@ -35,12 +40,18 @@
3641 if ( is_null( $session ) ) {
3742 $session = array();
3843 }
 44+ $_SESSION = $session; // paranoia
3945
4046 $request = new FauxRequest( $params, true, $session );
4147 $module = new ApiMain( $request, true );
4248 $module->execute();
4349
44 - return array( $module->getResultData(), $request, $request->getSessionArray() );
 50+ $results = array( $module->getResultData(), $request, $request->getSessionArray() );
 51+ if( $appendModule ) {
 52+ $results[] = $module;
 53+ }
 54+
 55+ return $results;
4556 }
4657
4758 /**
——@@ -62,4 +73,68 @@
6374 }
6475 }
6576
 77+ protected function doLogin() {
 78+ $data = $this->doApiRequest( array(
 79+ 'action' => 'login',
 80+ 'lgname' => self::$users['sysop']->username,
 81+ 'lgpassword' => self::$users['sysop']->password ) );
 82+
 83+ $token = $data[0]['login']['token'];
 84+
 85+ $data = $this->doApiRequest( array(
 86+ 'action' => 'login',
 87+ 'lgtoken' => $token,
 88+ 'lgname' => self::$users['sysop']->username,
 89+ 'lgpassword' => self::$users['sysop']->password
 90+ ), $data );
 91+
 92+ return $data;
 93+ }
 94+
 95+ protected function getTokenList( $user ) {
 96+ $GLOBALS['wgUser'] = $user->user;
 97+ $data = $this->doApiRequest( array(
 98+ 'action' => 'query',
 99+ 'titles' => 'Main Page',
 100+ 'intoken' => 'edit|delete|protect|move|block|unblock',
 101+ 'prop' => 'info' ) );
 102+ return $data;
 103+ }
66104 }
 105+
 106+class UserWrapper {
 107+ public $userName, $password, $user;
 108+
 109+ public function __construct( $userName, $password, $group = '' ) {
 110+ $this->userName = $userName;
 111+ $this->password = $password;
 112+
 113+ $this->user = User::newFromName( $this->userName );
 114+ if ( !$this->user->getID() ) {
 115+ $this->user = User::createNew( $this->userName, array(
 116+ "email" => "test@example.com",
 117+ "real_name" => "Test User" ) );
 118+ }
 119+ $this->user->setPassword( $this->password );
 120+
 121+ if ( $group !== '' ) {
 122+ $this->user->addGroup( $group );
 123+ }
 124+ $this->user->saveSettings();
 125+ }
 126+}
 127+
 128+class MockApi extends ApiBase {
 129+ public function execute() { }
 130+ public function getVersion() { }
 131+
 132+ public function __construct() { }
 133+
 134+ public function getAllowedParams() {
 135+ return array(
 136+ 'filename' => null,
 137+ 'enablechunks' => false,
 138+ 'sessionkey' => null,
 139+ );
 140+ }
 141+}
Index: trunk/phase3/tests/TestsAutoLoader.php
——@@ -14,10 +14,12 @@
1515 'BlockTest' => "$testFolder/phpunit/includes/BlockTest.php",
1616
1717 //API
18 - 'ApiTestSetup' => "$testFolder/phpunit/includes/api/ApiSetup.php",
 18+ 'ApiFormatTestBase' => "$testFolder/phpunit/includes/api/format/ApiFormatTestBase.php",
1919 'ApiTestCase' => "$testFolder/phpunit/includes/api/ApiTestCase.php",
2020 'ApiTestUser' => "$testFolder/phpunit/includes/api/ApiTestUser.php",
 21+ 'MockApi' => "$testFolder/phpunit/includes/api/ApiTestCase.php",
2122 'RandomImageGenerator' => "$testFolder/phpunit/includes/api/RandomImageGenerator.php",
 23+ 'UserWrapper' => "$testFolder/phpunit/includes/api/ApiTestCase.php",
2224
2325 //Parser
2426 'ParserTestFileIterator' => "$testFolder/phpunit/includes/parser/NewParserHelpers.php",

Follow-up revisions

RevisionCommit summaryAuthorDate
r91563Followup r91270: didn't need to be so paranoid about $_SESSION, and actually ...demon16:56, 6 July 2011

Comments

#Comment by Hashar (talk | contribs)   21:11, 1 July 2011

This commit has issue on CruiseControl with messages like: 

Skipping numeric key X. In Unknown line 0.

Where X is a number.

According to some googling, it seems related to $_SESSION being passed a numeric key. To catch the issue, we would need to add a strack trace logging before any $_SESSION assignment and try to find the culprit.

#Comment by 😂 (talk | contribs)   16:42, 6 July 2011

Related

Status & tagging log