r74646 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r74645‎ \| r74646 \| r74647 >
Date:	22:50, 11 October 2010
Author:	mah
Status:	deferred (Comments)
Tags:
Comment:	NOTE THAT THIS COMMIT REVEALS FAILING PARSER TESTS WHEN THEY ARE RUN IN PHPUnit. THE PHPUnit HARNESS NEEDS TO BE FIXED IN THIS CASE. Running parser tests by the traditional “php parserTests.php” verified to work without failures. Committing this to get the parser tests running under PHPUnit on the whole. I expect to find fixes for the PHPUnit harness shortly. * Construct ParserOptions with a user in ExtraParserTest and set up wgLang if it gets set to null — which was happening PHPUnit. Note that this probably shows a bug that I wasn't able to track down. * Adjust phpunit tests so that they execute and report Parser test failures properly. Attempt to make parser test titles the test titles PHPUnit sees. * Add E_STRICT reporting to PHPUnit testing. * Provide PHPUnitParserTest as an exension of ParserTest to handle necessary PHPUnit calls for running parser tests under PHPUnit. * Use dirname() instead of relative paths (i.e. “..”) where SearchEngineTest.php looks for bootstrap.php. * Sprinkle teardownGlobals() in exit points for teardownDatabase() * Make ParserTest::chomp() static.
Modified paths:	/trunk/phase3/maintenance/tests/parser/parserTest.inc (modified) (history) /trunk/phase3/maintenance/tests/phpunit/bootstrap.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/includes/ExtraParserTest.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/includes/parser/MediaWikiParserTest.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/includes/parser/ParserHelpers.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/includes/search/SearchEngineTest.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/suite.xml (modified) (history) /trunk/phase3/maintenance/tests/testHelpers.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/phpunit/bootstrap.php
—	—	@@ -31,34 +31,25 @@
32	32	/* Classes */
33	33
34	34	abstract class MediaWikiTestSetup extends PHPUnit_Framework_TestCase {
	35	+ protected $suite;
	36	+ public $regex = '';
	37	+ public $runDisabled = false;
35	38
36		~~- protected function buildTestDatabase( $tables ) {~~
37		~~- global $wgDBprefix;~~
38	39
39		~~- $db = wfGetDB( DB_MASTER );~~
40		~~- $oldTableNames = array();~~
41		~~- foreach ( $tables as $table )~~
42		~~- $oldTableNames[$table] = $db->tableName( $table );~~
43		~~- if ( $db->getType() == 'oracle' ) {~~
44		~~- $wgDBprefix = 'pt_';~~
45		~~- } else {~~
46		~~- $wgDBprefix = 'parsertest_';~~
	40	+ function __construct( PHPUnit_Framework_TestSuite $suite = null ) {
	41	+ if ( null !== $suite ) {
	42	+ $this->suite = $suite;
47	43	}
48		-
49		~~- $db->tablePrefix( $wgDBprefix );~~
50		-
51		~~- if ( $db->isOpen() ) {~~
52		~~- foreach ( $tables as $tbl ) {~~
53		~~- $newTableName = $db->tableName( $tbl );~~
54		~~- $tableName = $oldTableNames[$tbl];~~
55		~~- $db->query( "DROP TABLE IF EXISTS $newTableName", __METHOD__ );~~
56		~~- $db->duplicateTableStructure( $tableName, $newTableName, __METHOD__ );~~
57	44	}
58		~~- return $db;~~
	45	+
	46	+ function __call( $func, $args ) {
	47	+ if ( method_exists( $this->suite, $func ) ) {
	48	+ return call_user_func_array( array( $tohis->suite, $func ), $args);
59	49	} else {
60		~~- // Something amiss~~
61		~~- return null;~~
	50	+ throw new MWException( "Called non-existant $func method on "
	51	+ . get_class( $this ) );
62	52	}
	53	+ return false;
63	54	}
64	55	}
65	56
Index: trunk/phase3/maintenance/tests/phpunit/includes/ExtraParserTest.php
—	—	@@ -23,11 +23,14 @@
24	24
25	25	// Bug 8689 - Long numeric lines kill the parser
26	26	function testBug8689() {
	27	+ global $wgLang;
	28	+ global $wgUser;
27	29	$longLine = '1.' . str_repeat( '1234567890', 100000 ) . "\n";
28	30
	31	+ if ( $wgLang === null ) $wgLang = new Language;
29	32	$parser = new Parser();
30	33	$t = Title::newFromText( 'Unit test' );
31		~~- $options = new ParserOptions();~~
	34	+ $options = ParserOptions::newFromUser( $wgUser );
32	35	$this->assertEquals( "<p>$longLine</p>",
33	36	$parser->parse( $longLine, $t, $options )->getText() );
34	37	}
Index: trunk/phase3/maintenance/tests/phpunit/includes/parser/ParserHelpers.php
—	—	@@ -1,5 +1,21 @@
2	2	<?php
3	3
	4	+class PHPUnitParserTest extends ParserTest {
	5	+ function showTesting( $desc ) {
	6	+ /* Do nothing since we don't want to show info during PHPUnit testing. */
	7	+ }
	8	+
	9	+ public function showSuccess( $desc ) {
	10	+ PHPUnit_Framework_Assert::assertTrue( true, $desc );
	11	+ return true;
	12	+ }
	13	+
	14	+ public function showFailure( $desc, $expected, $got ) {
	15	+ PHPUnit_Framework_Assert::assertEquals( $expected, $got, $desc );
	16	+ return false;
	17	+ }
	18	+}
	19	+
4	20	class ParserUnitTest extends PHPUnit_Framework_TestCase {
5	21	private $test = "";
6	22	private $suite;
—	—	@@ -17,9 +33,9 @@
18	34	$result = new PHPUnit_Framework_TestResult;
19	35	}
20	36
21		~~- $backend = $this->suite->getBackend();~~
	37	+ $backend = new ParserTestSuiteBackend;
22	38	$result->startTest( $this );
23		-
	39	+
24	40	// Support the transition to PHPUnit 3.5 where PHPUnit_Util_Timer is replaced with PHP_Timer
25	41	if ( class_exists( 'PHP_Timer' ) ) {
26	42	PHP_Timer::start();
—	—	@@ -32,16 +48,13 @@
33	49	# Run the test.
34	50	# On failure, the subclassed backend will throw an exception with
35	51	# the details.
36		~~- $r = $backend->runTest(~~
37		~~- $this->test['test'],~~
38		~~- $this->test['input'],~~
39		~~- $this->test['result'],~~
40		~~- $this->test['options'],~~
41		~~- $this->test['config']~~
	52	+ $pt = new PHPUnitParserTest;
	53	+ $r = $pt->runTest( $this->test['test'], $this->test['input'],
	54	+ $this->test['result'], $this->test['options'], $this->test['config']
42	55	);
43	56	}
44	57	catch ( PHPUnit_Framework_AssertionFailedError $e ) {
45		-
	58	+
46	59	// PHPUnit_Util_Timer -> PHP_Timer support (see above)
47	60	if ( class_exists( 'PHP_Timer' ) ) {
48	61	$result->addFailure( $this, $e, PHP_Timer::stop() );
—	—	@@ -77,20 +90,29 @@
78	91
79	92	}
80	93
81		~~-class ParserTestSuiteBackend extends ParserTest {~~
	94	+class ParserTestSuiteBackend extends PHPUnit_FrameWork_TestSuite {
	95	+ public $recorder;
	96	+ public $term;
	97	+ static $usePHPUnit = false;
	98	+
	99	+ function __construct() {
	100	+ parent::__construct();
	101	+ $this->setupRecorder(null);
	102	+ self::$usePHPUnit = method_exists('PHPUnit_Framework_Assert', 'assertEquals');
	103	+ }
	104	+
82	105	function showTesting( $desc ) {
83	106	}
84	107
85	108	function showRunFile( $path ) {
86	109	}
87	110
88		~~- function showSuccess( $desc ) {~~
89		~~- PHPUnit_Framework_Assert::assertTrue( true, $desc );~~
90		~~- return true;~~
	111	+ function showTestResult( $desc, $result, $out ) {
	112	+ if ( $result === $out ) {
	113	+ return self::showSuccess( $desc, $result, $out );
	114	+ } else {
	115	+ return self::showFailure( $desc, $result, $out );
91	116	}
92		-
93		~~- function showFailure( $desc, $expected, $got ) {~~
94		~~- PHPUnit_Framework_Assert::assertEquals( $expected, $got, $desc );~~
95	117	}
96	118
97	119	public function setupRecorder( $options ) {
—	—	@@ -107,4 +129,3 @@
108	130
109	131	function reportPercentage( $success, $total ) { }
110	132	}
111		-
Index: trunk/phase3/maintenance/tests/phpunit/includes/parser/MediaWikiParserTest.php
—	—	@@ -1,41 +1,34 @@
2	2	<?php
3	3
4	4	require_once( dirname( __FILE__ ) . '/ParserHelpers.php' );
	5	+require_once( dirname(dirname(dirname( __FILE__ ))) . '/bootstrap.php' );
5	6
6		~~-class MediaWikiParserTest extends PHPUnit_Framework_TestSuite {~~
7		~~- private $count;~~
	7	+class MediaWikiParserTest extends MediaWikiTestSetup {
	8	+ public $count;
8	9	public $backend;
9	10
10		~~- public static function suite() {~~
11		~~- return new self;~~
12		~~- }~~
13		-
14	11	public function __construct() {
	12	+ $suite = new PHPUnit_Framework_TestSuite('Parser Tests');
	13	+ parent::__construct($suite);
15	14	$this->backend = new ParserTestSuiteBackend;
16		~~- parent::__construct();~~
17	15	$this->setName( 'Parser tests' );
18	16	}
19	17
20		~~- public function run( PHPUnit_Framework_TestResult $result = null, $filter = false,~~
21		~~- array $groups = array(), array $excludeGroups = array(), $processIsolation = false~~
22		~~- ) {~~
23		~~- global $IP, $wgContLang, $wgMemc;~~
24		~~- $wgContLang = Language::factory( 'en' );~~
25		~~- $wgMemc = new FakeMemCachedClient;~~
26		~~- $this->backend->setupDatabase();~~
	18	+ public static function suite() {
	19	+ global $IP;
27	20
	21	+ $tester = new self;
	22	+
28	23	$iter = new TestFileIterator( "$IP/maintenance/tests/parser/parserTests.txt" );
29		~~- $iter->setParser( $this->backend );~~
30		~~- $this->count = 0;~~
	24	+ $iter->setParser( $tester );
	25	+ $tester->count = 0;
31	26
32	27	foreach ( $iter as $test ) {
33		~~- $this->addTest( new ParserUnitTest( $this, $test ) );~~
34		~~- $this->count++;~~
	28	+ $tester->suite->addTest( new ParserUnitTest( $tester, $test ) );
	29	+ $tester->count++;
35	30	}
36	31
37		~~- parent::run( $result, $filter, $groups, $excludeGroups, $processIsolation );~~
38		-
39		~~- $this->backend->teardownDatabase();~~
	32	+ return $tester->suite;
40	33	}
41	34
42	35	public function count() {
Index: trunk/phase3/maintenance/tests/phpunit/includes/search/SearchEngineTest.php
—	—	@@ -1,6 +1,6 @@
2	2	<?php
3	3
4		~~-require_once dirname(__FILE__) . '/../../bootstrap.php';~~
	4	+require_once dirname(dirname(dirname(__FILE__))). '/bootstrap.php';
5	5
6	6	/**
7	7	* @group Stub
Index: trunk/phase3/maintenance/tests/phpunit/suite.xml
—	—	@@ -6,7 +6,8 @@
7	7	convertErrorsToExceptions="true"
8	8	convertNoticesToExceptions="true"
9	9	convertWarningsToExceptions="true"
10		~~- stopOnFailure="false">~~
	10	+ stopOnFailure="false"
	11	+ strict="true">
11	12	<testsuites>
12	13	<testsuite name="includes">
13	14	<directory>./includes</directory>
—	—	@@ -26,6 +27,7 @@
27	28	</testsuites>
28	29	<groups>
29	30	<exclude>
	31	+ <group>Utility</group>
30	32	<group>Broken</group>
31	33	<group>Stub</group>
32	34	</exclude>
Index: trunk/phase3/maintenance/tests/testHelpers.inc
—	—	@@ -478,7 +478,7 @@
479	479	$this->lineNum = $this->index = 0;
480	480	}
481	481
482		~~- function setParser( ParserTest $parser ) {~~
	482	+ function setParser( MediaWikiParserTest $parser ) {
483	483	$this->parser = $parser;
484	484	}
485	485
—	—	@@ -536,11 +536,10 @@
537	537	wfDie( "'endarticle' without 'article' at line {$this->lineNum} of $this->file\n" );
538	538	}
539	539
540		~~- if ( $this->parser ) {~~
541		~~- $this->parser->addArticle( $this->parser->chomp( $data['article'] ), $this->parser->chomp( $data['text'] ),~~
542		~~- $this->lineNum );~~
543		~~- }~~
544		-
	540	+ $title = Title::newFromText( ParserTest::chomp( $data['article'] ) );
	541	+ $aid = $title->getArticleID( Title::GAID_FOR_UPDATE );
	542	+ if ( $aid == 0 )
	543	+ ParserTest::addArticle( $data['article'], $data['text'], $this->lineNum );
545	544	$data = array();
546	545	$section = null;
547	546
—	—	@@ -632,11 +631,11 @@
633	632
634	633	if ( $this->parser ) {
635	634	$this->test = array(
636		~~- 'test' => $this->parser->chomp( $data['test'] ),~~
637		~~- 'input' => $this->parser->chomp( $data['input'] ),~~
638		~~- 'result' => $this->parser->chomp( $data['result'] ),~~
639		~~- 'options' => $this->parser->chomp( $data['options'] ),~~
640		~~- 'config' => $this->parser->chomp( $data['config'] ) );~~
	635	+ 'test' => ParserTest::chomp( $data['test'] ),
	636	+ 'input' => ParserTest::chomp( $data['input'] ),
	637	+ 'result' => ParserTest::chomp( $data['result'] ),
	638	+ 'options' => ParserTest::chomp( $data['options'] ),
	639	+ 'config' => ParserTest::chomp( $data['config'] ) );
641	640	} else {
642	641	$this->test['test'] = $data['test'];
643	642	}
Index: trunk/phase3/maintenance/tests/parser/parserTest.inc
—	—	@@ -57,6 +57,8 @@
58	58	private $fuzzSeed = 0;
59	59	private $memoryLimit = 50;
60	60
	61	+ public $regex = "";
	62	+ private $savedGlobals = array();
61	63	/**
62	64	* Sets terminal colorization and diff/quick modes depending on OS and
63	65	* command-line options (--color and --quick).
—	—	@@ -113,8 +115,59 @@
114	116
115	117	$this->hooks = array();
116	118	$this->functionHooks = array();
	119	+ self::setUp();
117	120	}
118	121
	122	+ static function setUp() {
	123	+ global $wgParser, $wgParserConf, $IP, $messageMemc, $wgMemc, $wgDeferredUpdateList,
	124	+ $wgUser, $wgLang, $wgOut, $wgRequest, $wgStyleDirectory, $wgEnableParserCache,
	125	+ $wgMessageCache, $wgUseDatabaseMessages, $wgMsgCacheExpiry, $parserMemc,
	126	+ $wgNamespaceAliases, $wgNamespaceProtection, $wgLocalFileRepo,
	127	+ $wgThumbnailScriptPath, $wgScriptPath,
	128	+ $wgArticlePath, $wgStyleSheetPath, $wgScript, $wgStylePath;
	129	+
	130	+ $wgScript = '/index.php';
	131	+ $wgScriptPath = '/';
	132	+ $wgArticlePath = '/wiki/$1';
	133	+ $wgStyleSheetPath = '/skins';
	134	+ $wgStylePath = '/skins';
	135	+ $wgThumbnailScriptPath = false;
	136	+ $wgLocalFileRepo = array(
	137	+ 'class' => 'LocalRepo',
	138	+ 'name' => 'local',
	139	+ 'directory' => wfTempDir() . '/test-repo',
	140	+ 'url' => 'http://example.com/images',
	141	+ 'deletedDir' => wfTempDir() . '/test-repo/delete',
	142	+ 'hashLevels' => 2,
	143	+ 'transformVia404' => false,
	144	+ );
	145	+ $wgNamespaceProtection[NS_MEDIAWIKI] = 'editinterface';
	146	+ $wgNamespaceAliases['Image'] = NS_FILE;
	147	+ $wgNamespaceAliases['Image_talk'] = NS_FILE_TALK;
	148	+
	149	+
	150	+ $wgEnableParserCache = false;
	151	+ $wgDeferredUpdateList = array();
	152	+ $wgMemc =& wfGetMainCache();
	153	+ $messageMemc =& wfGetMessageCacheStorage();
	154	+ $parserMemc =& wfGetParserCacheStorage();
	155	+
	156	+ // $wgContLang = new StubContLang;
	157	+ $wgUser = new User;
	158	+ $wgLang = new StubUserLang;
	159	+ $wgOut = new StubObject( 'wgOut', 'OutputPage' );
	160	+ $wgParser = new StubObject( 'wgParser', $wgParserConf['class'], array( $wgParserConf ) );
	161	+ $wgRequest = new WebRequest;
	162	+
	163	+ $wgMessageCache = new StubObject( 'wgMessageCache', 'MessageCache',
	164	+ array( $messageMemc, $wgUseDatabaseMessages,
	165	+ $wgMsgCacheExpiry ) );
	166	+ if ( $wgStyleDirectory === false ) {
	167	+ $wgStyleDirectory = "$IP/skins";
	168	+ }
	169	+
	170	+ }
	171	+
119	172	public function setupRecorder ( $options ) {
120	173	if ( isset( $options['record'] ) ) {
121	174	$this->recorder = new DbTestRecorder( $this );
—	—	@@ -131,8 +184,9 @@
132	185
133	186	/**
134	187	* Remove last character if it is a newline
	188	+ * @group utility
135	189	*/
136		~~- public function chomp( $s ) {~~
	190	+ static public function chomp( $s ) {
137	191	if ( substr( $s, -1 ) === "\n" ) {
138	192	return substr( $s, 0, -1 );
139	193	}
—	—	@@ -421,13 +475,20 @@
422	476	$result = $this->tidy( $result );
423	477	}
424	478
425		-
426	479	$this->teardownGlobals();
	480	+ return $this->showTestResult( $desc, $result, $out );
	481	+ }
427	482
428		~~- if ( $result === $out && ( $noxml === true \|\| $this->wellFormed( $out ) ) ) {~~
429		~~- return $this->showSuccess( $desc );~~
	483	+ /**
	484	+ *
	485	+ */
	486	+ function showTestResult( $desc, $result, $out ) {
	487	+ if ( $result === $out ) {
	488	+ $this->showSuccess( $desc );
	489	+ return true;
430	490	} else {
431		~~- return $this->showFailure( $desc, $result, $out );~~
	491	+ $this->showFailure( $desc, $result, $out );
	492	+ return false;
432	493	}
433	494	}
434	495
—	—	@@ -820,6 +881,7 @@
821	882	global $wgDBtype;
822	883
823	884	if ( !$this->databaseSetupDone ) {
	885	+ $this->teardownGlobals();
824	886	return;
825	887	}
826	888	$this->teardownUploadDir( $this->uploadDir );
—	—	@@ -829,6 +891,7 @@
830	892
831	893	if ( $this->useTemporaryTables ) {
832	894	# Don't need to do anything
	895	+ $this->teardownGlobals();
833	896	return;
834	897	}
835	898
—	—	@@ -842,6 +905,8 @@
843	906
844	907	if ( $wgDBtype == 'oracle' )
845	908	$db->query( 'BEGIN FILL_WIKI_INFO; END;' );
	909	+
	910	+ $this->teardownGlobals();
846	911	}
847	912
848	913	/**
—	—	@@ -1085,20 +1150,26 @@
1086	1151	* @param $text String: the article text
1087	1152	* @param $line Integer: the input line number, for reporting errors
1088	1153	*/
1089		~~- public function addArticle( $name, $text, $line ) {~~
	1154	+ static public function addArticle( $name, $text, $line ) {
1090	1155	global $wgCapitalLinks;
	1156	+
	1157	+ $text = self::chomp($text);
	1158	+
1091	1159	$oldCapitalLinks = $wgCapitalLinks;
1092	1160	$wgCapitalLinks = true; // We only need this from SetupGlobals() See r70917#c8637
1093	1161
1094		~~- $title = Title::newFromText( $name );~~
	1162	+ $title = Title::newFromText( self::chomp($name) );
1095	1163
1096	1164	if ( is_null( $title ) ) {
1097		~~- wfDie( "invalid title at line $line\n" );~~
	1165	+ wfDie( "invalid title ('$name' => '$title') at line $line\n" );
1098	1166	}
1099	1167
1100	1168	$aid = $title->getArticleID( Title::GAID_FOR_UPDATE );
1101	1169
1102	1170	if ( $aid != 0 ) {
	1171	+ foreach(debug_backtrace() as $bt) {
	1172	+ echo "{$bt['file']}::{$bt['line']}\n";
	1173	+ }
1103	1174	wfDie( "duplicate article '$name' at line $line\n" );
1104	1175	}
1105	1176

Follow-up revisions

Revision	Commit summary	Author	Date
r74747	Fixup r74646...	reedy	23:43, 13 October 2010
r75420	Marking parsertests as broken until r74646 is fixed.	platonides	13:56, 26 October 2010
r76134	Follow up r75810. The code for the parser tests modified the main database ev...	platonides	20:03, 5 November 2010

Comments

#Comment by Platonides (talk | contribs) 18:44, 25 October 2010

Marking as fixme since 57 parser tests fail under phpunit.

mah, look at the image setup, I don't think you are adding the images. That should fix a lot of them.

#Comment by MarkAHershberger (talk | contribs) 20:02, 6 January 2011

X! has rewritten this post-branch.

#Comment by Reedy (talk | contribs) 20:04, 6 January 2011

Marking as old then (as in it has been superceeded).

Not a real issue for 1.17 itself, tests are known to be dodgy

#Comment by Platonides (talk | contribs) 22:04, 6 January 2011

I disagree. Our releases should have the tests working, and end users should be able to run them and verify they indeed pass. We shouldn't perform releases with broken tests, such as 1.15.4 whose tests could have been trivially fixed (r67101).

That said, given that parserTests.php work and this revision had been broken for months, and were disabled by default in phpunit, I agree it's not worth trying to ship this fixed in 1.17.

Status & tagging log

00:20, 14 September 2011 Meno25 (talk | contribs) changed the status of r74646 [removed: old added: deferred]
20:04, 6 January 2011 Reedy (talk | contribs) changed the status of r74646 [removed: new added: old]
20:02, 6 January 2011 MarkAHershberger (talk | contribs) changed the status of r74646 [removed: fixme added: new]
18:44, 25 October 2010 Platonides (talk | contribs) changed the status of r74646 [removed: new added: fixme]