Page MenuHomePhabricator

Tests in /phase3/t/inc no longer work. Fix provided.
Closed, ResolvedPublic

Description

Author: dnessett

Description:
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).


Version: 1.16.x
Severity: normal

Attached:

Details

Reference
bz20112

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:53 PM
bzimport set Reference to bz20112.

(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).

sorry, WebStart.php not WebRequest.php.

dnessett wrote:

(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.

dnessett wrote:

(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).

dnessett wrote:

(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."

dnessett wrote:

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.

avarab wrote:

Applied this patch with some fixes. RESOLVED FIXED in trunk.