Last modified: 2009-11-08 11:07:51 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T22112, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 20112 - Tests in /phase3/t/inc no longer work. Fix provided.
Tests in /phase3/t/inc no longer work. Fix provided.
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Maintenance scripts (Other open bugs)
1.16.x
All All
: Normal normal (vote)
: ---
Assigned To: Brion Vibber
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-07 18:26 UTC by Dan Nessett
Modified: 2009-11-08 11:07 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
fixes 8 bugs in /phase3/t/ tests (5.21 KB, patch)
2009-08-07 18:26 UTC, Dan Nessett
Details

Description Dan Nessett 2009-08-07 18:26:24 UTC
Created attachment 6438 [details]
fixes 8 bugs in /phase3/t/ tests

The tests in /phase3/t/inc have been broken for a while. The attached patch (merged, diffed and tested against r54593) fixes all of the tests in that directory. The patch is relative to /phase3/t/. Getting all tests to work required fixing 8 bugs. Note: the test file Parser.t still returns one failure. That is because Parser.t simply involkes parserTests and parserTests currently reports one failed test. So, this failure is not a bug in Parser.t.

The following files are affected by this patch:

* fixed bug in t/Search.inc: the database prefix was incorrect. Added a "_" suffix so Search.t works.

* fixed by in t/inc/Database.t: Moved up require of LocalSettings.php, because its definitions are now required by Autoloader.php.

* fixed bug in t/inc/Global.t: Moved up require of LocalSettings.php, because its definitions are now required by Autoloader.php.

* fixed bug in IP.t: increased plan number from 1010 to 1120.

* fixed bug in Language.t: changed data constants in array initialization from floats to strings. Specifying them as floats was causing round-off error problems. In addition userAdjust expects its arguments to be strings.

* fixed bug in LocalFile.t: changed expected result from getThumbVirtualUrl() to reflect new directory organization for filerepos (specifically, virtual thumbs are in /test/thumb not /test/public/thumb.

* fixed bug in Revision.t: added require of LocalSettings.php and require_once of LocalisationCache.php. LocalisationCache.php requires some of the definitions in LocalSettings.php. LocalisationCache.php is required so Language::factory( 'en' ) works.

* fixed bug in Title.t: deleted the ">" character in the strpos() call so ">" tests as legal in page titles. The addition of ">" to $wgLegalTitleChars in DefaultSettings.php occured in r53667.

To verify that the tests now run correctly, cd to /phase3/ and execute 'prove t/inc -r' (assumes tester has the prove application installed).
Comment 1 Alexandre Emsenhuber [IAlex] 2009-08-09 20:53:56 UTC
(In reply to comment #0)
> Moved up require of LocalSettings.php, because its definitions are now
> required by Autoloader.php.

No, that's the opposite, AutoLoader.php is supposed be included before LocalSettings.php (as in includes/WebRequest.php).
Comment 2 Alexandre Emsenhuber [IAlex] 2009-08-09 20:54:42 UTC
sorry, WebStart.php not WebRequest.php.
Comment 3 Dan Nessett 2009-08-10 16:24:43 UTC
(In reply to comment #2)
> sorry, WebStart.php not WebRequest.php.
> 

Following the execution path of Database.t. - in the current version it performs a require 'includes/AutoLoader.php' on line 9. This takes it into Autoloader.php, which goes through function define logic until it reaches line 622 (line number relative to r54724). There it executes require_once("$IP/js2/mwEmbed/php/jsAutoloadLocalClasses.php"). Note the requirement that $IP is defined at this point. Since LocalSettings.php has not yet been required, $IP is undefined and the Autoloader require_once() fails.
Comment 4 Dan Nessett 2009-08-10 20:12:52 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > Moved up require of LocalSettings.php, because its definitions are now
> > required by Autoloader.php.
> 
> No, that's the opposite, AutoLoader.php is supposed be included before
> LocalSettings.php (as in includes/WebRequest.php).
> 

It appears that includes/WebRequest.php tests for the existence of $IP and if it is missing defines it (lines 61-63). I'm not sure what is going on here. For some reason Webstart wants to define $IP independently of its definition in LocalSettings.php. Maybe there is a good reason for this, but it's kind of clumsy (not to mention a potential source of bugs, since it means $IP is defined in two different place).
Comment 5 Dan Nessett 2009-08-10 20:15:04 UTC
(In reply to comment #4)

Made the same mistake you did (in my case due to inattentive copying of text). That should be "includes/WebStart.php."
Comment 6 Dan Nessett 2009-08-11 00:02:36 UTC
Poking around for another purpose, I have found another file that defines $IP - maintanence/Command.inc. It seems to me this is only looking for trouble. If there is some reason why require_once('LocalSettings.php') will cause problems in this file and WebStart.php, then LocalSettings.php should be broken into two parts: 1) LocalSettingsCore.php and LocalSettingsNonCore.php (I am sure someone can come up with better names than these). LocalSettingsCore.php should contain definitions that any core MW php file can tolerate, such as the definition of $IP. LocalSettingsNoneCore.php should contain those definitions that may cause problems with one or more core MW files. Any or all core MW files can then require LocalSettingsCore.php. I will post this proposal to wikitech.
Comment 7 Ævar Arnfjörð Bjarmason 2009-11-08 11:07:51 UTC
Applied this patch with some fixes. RESOLVED FIXED in trunk. 

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links