Re: [Wikitech-l] How sort key works?
2009/8/11 Helder Geovane Gomes de Lima heldergeov...@gmail.com: 2009/8/10 Aryeh Gregor We could break ties by appending the page title to custom sort keys, if this is a problem. I think it would be good! =) I don’t (at least not in the way it is expressed). If you want to use the page title as a tiebreaker, then add it as a new column to the index (before the page_id), not (as I read the original sentence) by appending the title to the sort key. Otherwise, you’ll have to separate the sort key from the title with some control character under U+0020 (to ensure correct ordering of different-length sort keys – you need a separator which sorts before any valid character), which would be messy. But still, I don’t see the point in doing that. You don’t want a page called “Aaa” to come after a page called “Abc” when you set their sortkeys both to the same value? Don’t do that then. Set the sortkey accordingly to what you want. (OBTW, a different thing is that category paging is probably buggy in this tiebreaking aspect – even though the index is correctly defined to be unique, the page_id column is not included in the from= paging parameter. But this bug will probably appear only in extreme cases, like 300 articles with an identical sortkey.) -- [[cs:User:Mormegil | Petr Kadlec]] ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] How sort key works?
2009/8/11 Petr Kadlec petr.kad...@gmail.com: I don’t (at least not in the way it is expressed). If you want to use the page title as a tiebreaker, then add it as a new column to the index (before the page_id), not (as I read the original sentence) by appending the title to the sort key. The page title is not in the categorylinks table, so we can't add it to the index. Otherwise, you’ll have to separate the sort key from the title with some control character under U+0020 (to ensure correct ordering of different-length sort keys – you need a separator which sorts before any valid character), which would be messy. But still, I don’t see the point in doing that. You don’t want a page called “Aaa” to come after a page called “Abc” when you set their sortkeys both to the same value? Don’t do that then. Set the sortkey accordingly to what you want. Exactly. When using identical sortkeys, you shouldn't complain that MediaWiki doesn't magically know in which order you want to sort them. You can make it predictable by using a (more) unique sortkey. Roan Kattouw (Catrope) ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
--- On Mon, 8/10/09, Tim Starling tstarl...@wikimedia.org wrote:
No, the reason is because LocalSettings.php is in the
directory
pointed to by $IP, so you have to work out what $IP is
before you can
include it.
Web entry points need to locate WebStart.php, and command
line scripts
need to locate maintenance/commandLine.inc. Then either of
those two
entry scripts can locate the rest of MediaWiki.
Fair enough, but consider the following.
I did a global search over the phase3 directory and got these hits for the
string $IP = :
.../phase3/config/index.php:30: $IP = dirname( dirname( __FILE__ ) );
.../phase3/config/index.php:1876: \$IP = MW_INSTALL_PATH;
.../phase3/config/index.php:1878: \$IP = dirname( __FILE__ );
.../phase3/includes/WebStart.php:61: $IP = getenv( 'MW_INSTALL_PATH' );
.../phase3/includes/WebStart.php:63:$IP = realpath( '.' );
.../phase3/js2/mwEmbed/php/noMediaWikiConfig.php:11: $IP =
realpath(dirname(__FILE__).'/../');
.../phase3/LocalSettings.php:17:$IP = MW_INSTALL_PATH;
.../phase3/LocalSettings.php:19:$IP = dirname( __FILE__ );
.../phase3/maintenance/language/validate.php:16: $IP = dirname( __FILE__ ) .
'/../..';
.../phase3/maintenance/Maintenance.php:336: $IP = strval(
getenv('MW_INSTALL_PATH') ) !== ''
So, it appears that $IP computation is occurring in 6 files. In addition, $IP
is adjusted by the relative place of the file in the MW source tree (e.g., in
validate.php, $IP is set to dirname( __FILE__ ) . '/../..';) Adjusting paths
according to where a file exists in a source tree is fraught with danger. If
you ever move the file for some reason, the code breaks.
Why not isolate at least $IP computation in a single function? (Perhaps
breaking up LocalSettings.php into two parts is overkill, but certainly
cleaning up $IP computation isn't too radical an idea.) Of course, there is the
problem of locating the file of the function that does this. One approach is to
recognize that php.ini already requires potential modification for MW use.
Specifically, the path to PEAR must occur in 'include_path'. It would be a
simple matter to add another search directory for locating the initialization
code.
Or maybe there is a better way of locating MW initialization code. How its done
is an open issue. I am simply arguing that computing the value of $IP by
relying on the position of the php file in a source tree is not good software
architecture. Experience shows that this kind of thing almost always leads to
bugs.
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On Tue, Aug 11, 2009 at 10:59 AM, dan nessettdness...@yahoo.com wrote:
--- On Mon, 8/10/09, Tim Starling tstarl...@wikimedia.org wrote:
No, the reason is because LocalSettings.php is in the
directory
pointed to by $IP, so you have to work out what $IP is
before you can
include it.
Web entry points need to locate WebStart.php, and command
line scripts
need to locate maintenance/commandLine.inc. Then either of
those two
entry scripts can locate the rest of MediaWiki.
Fair enough, but consider the following.
I did a global search over the phase3 directory and got these hits for the
string $IP = :
.../phase3/config/index.php:30: $IP = dirname( dirname( __FILE__ ) );
.../phase3/config/index.php:1876: \$IP = MW_INSTALL_PATH;
.../phase3/config/index.php:1878: \$IP = dirname( __FILE__ );
.../phase3/includes/WebStart.php:61: $IP = getenv( 'MW_INSTALL_PATH' );
.../phase3/includes/WebStart.php:63: $IP = realpath( '.' );
.../phase3/js2/mwEmbed/php/noMediaWikiConfig.php:11: $IP =
realpath(dirname(__FILE__).'/../');
.../phase3/LocalSettings.php:17: $IP = MW_INSTALL_PATH;
.../phase3/LocalSettings.php:19: $IP = dirname( __FILE__ );
.../phase3/maintenance/language/validate.php:16: $IP = dirname( __FILE__ ) .
'/../..';
.../phase3/maintenance/Maintenance.php:336: $IP = strval(
getenv('MW_INSTALL_PATH') ) !== ''
So, it appears that $IP computation is occurring in 6 files. In addition, $IP
is adjusted by the relative place of the file in the MW source tree (e.g., in
validate.php, $IP is set to dirname( __FILE__ ) . '/../..';) Adjusting paths
according to where a file exists in a source tree is fraught with danger. If
you ever move the file for some reason, the code breaks.
Why not isolate at least $IP computation in a single function? (Perhaps
breaking up LocalSettings.php into two parts is overkill, but certainly
cleaning up $IP computation isn't too radical an idea.) Of course, there is
the problem of locating the file of the function that does this. One approach
is to recognize that php.ini already requires potential modification for MW
use. Specifically, the path to PEAR must occur in 'include_path'. It would be
a simple matter to add another search directory for locating the
initialization code.
Or maybe there is a better way of locating MW initialization code. How its
done is an open issue. I am simply arguing that computing the value of $IP by
relying on the position of the php file in a source tree is not good software
architecture. Experience shows that this kind of thing almost always leads to
bugs.
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
The problem with putting it in a single function is you still have
to find where that function is to begin with (I'd assume either
GlobalFunctions or install-utils would define this). At which point
you're back to the original problem: defining $IP yourself so you
can find this.
Yes, we should probably do this all a little more cleanly (at least
one unified style would be nice), but constructing it manually is
pretty much a given for anything trying to find an entry point, as
Tim points out.
-Chad
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] How sort key works?
On Tue, Aug 11, 2009 at 4:35 AM, Petr Kadlecpetr.kad...@gmail.com wrote: (OBTW, a different thing is that category paging is probably buggy in this tiebreaking aspect – even though the index is correctly defined to be unique, the page_id column is not included in the from= paging parameter. But this bug will probably appear only in extreme cases, like 300 articles with an identical sortkey.) It will return slightly wrong results whenever two articles with the same sort key happen to hit a page boundary. It's not a huge deal, since sortkeys are usually fairly unique, but it shouldn't be hard to fix if cl_from is already part of the sortkey index -- which it is, on trunk, although I can't say for sure whether that matches the deployed version. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
--- On Tue, 8/11/09, Chad innocentkil...@gmail.com wrote:
The problem with putting it in a single function is you
still have
to find where that function is to begin with (I'd assume
either
GlobalFunctions or install-utils would define this). At
which point
you're back to the original problem: defining $IP yourself
so you
can find this.
Yes, we should probably do this all a little more cleanly
(at least
one unified style would be nice), but constructing it
manually is
pretty much a given for anything trying to find an entry
point, as
Tim points out.
I'm probably missing something since I have only been programming in PHP for
about 4 weeks, but if you set include_path in php.ini so it includes the root
of the MW tree, put a php file at that level that has a function (or a method
in a class) that returns the MW root path, wouldn't that work? For example, if
you modified include_path in php.ini to include pathname to MW root, added
the file MWInit.php to the MW root directory and in MWInit.php put a function
MWInit() that computes and returns $IP, wouldn't that eliminate the necessity
of manually figuring out the value of $IP [each place where you now compute $IP
could require_once('MWInit.php') and call MWInit()]?
Of course, it may be considered dangerous for the MW installation software to
fool around with php.ini. But, even if you require setting the MW root manually
in php.ini::include_path (abusing the php namespace disambiguation operator
here) that would be an improvement. You should only have to do this once and
could upgrade MW without disturbing this binding.
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
* dan nessett dness...@yahoo.com [Tue, 11 Aug 2009 09:00:50 -0700
(PDT)]:
I'm probably missing something since I have only been programming in
PHP
for about 4 weeks, but if you set include_path in php.ini so it
includes
the root of the MW tree, put a php file at that level that has a
function (or a method in a class) that returns the MW root path,
wouldn't that work? For example, if you modified include_path in
php.ini
to include pathname to MW root, added the file MWInit.php to the MW
root directory and in MWInit.php put a function MWInit() that computes
and returns $IP, wouldn't that eliminate the necessity of manually
figuring out the value of $IP [each place where you now compute $IP
could require_once('MWInit.php') and call MWInit()]?
Of course, it may be considered dangerous for the MW installation
software to fool around with php.ini. But, even if you require setting
the MW root manually in php.ini::include_path (abusing the php
namespace
disambiguation operator here) that would be an improvement. You should
only have to do this once and could upgrade MW without disturbing this
binding.
Sorry for interrupting the conversation, but not everyone have root
rights to change php.ini freely. (MediaWiki can be used at shared
hosting sometimes).
I'd better define a shell variable for that ($MW_INSTALL_PATH).
Dmitriy
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On Tue, Aug 11, 2009 at 12:57 PM, dan nessettdness...@yahoo.com wrote: However, Dmitriy Sintsov makes the excellent point that not everyone can change php.ini. So, I then put MWInit.php into the PEAR directory and modified it as follows: Um, I'm assuming the PEAR directory is not writable to the user of an average shared webhost. Such users commonly have only FTP access to some subdirectory of the web root. Even if it was writable, it's not reasonable to require users to copy files into several different directories if there's not a clear benefit. You should be able to untar in your web root, run config/index.php, and have everything work. The question may arise, how do you get the value of the MW root into MWInit.php in the first place? This can occur on MW installation. Then you're doing almost exactly the same thing we're doing now, except with MWInit.php instead of LocalSettings.php. $IP is normally set in LocalSettings.php for most page views. Some places still must figure it out independently in either case, e.g., config/index.php. Of course, there may be a problem getting MWInit.php into the PEAR directory, but I assume that is an easier task than modifying php.ini. It's not, compared to not having to modify anything outside your web root. However, if even this is a problem, then we can do what Dmitry suggests, set an environmental variable to the MW root path. How? ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On 8/11/09 9:57 AM, dan nessett wrote: The question may arise, how do you get the value of the MW root into MWInit.php in the first place? This can occur on MW installation. Of course, there may be a problem getting MWInit.php into the PEAR directory, but I assume that is an easier task than modifying php.ini. However, if even this is a problem, then we can do what Dmitry suggests, set an environmental variable to the MW root path. You shouldn't assume you can set environment variables either. :) Really, MediaWiki is built to assume it's being run from the base directory where its files are found. I'm not sure there's a compelling reason to even have $IP set in LocalSettings.php anymore; the base include path should probably be autodetected in all cases, which is already being done in WebStart.php and commandLine.inc, the web and CLI initialization includes based on their locations in the file tree. Unless there's some reason to do otherwise, I'd recommend dropping the $IP from the autogen'd LocalSettings.php and pulling in DefaultSettings.php from the level above. (Keeping in mind that we should retain compat with existing LocalSettings.php files that are still pulling it.) Having $IP in there is a frequent source of upgrade problems as people move their filesystem paths around and then discover that their new MediaWiki install is trying to load files from the old one in another directory. -- brion ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
Brion Vibber br...@wikimedia.org wrote: Unless there's some reason to do otherwise, I'd recommend dropping the $IP from the autogen'd LocalSettings.php and pulling in DefaultSettings.php from the level above. (Keeping in mind that we should retain compat with existing LocalSettings.php files that are still pulling it.) Better, but what about /config/index.php, noMediaWikiConfig.php, validate.php and Maintenance.php? Having only two different places where $IP is computed is a definite improvement (assuming you fix the 4 files just mentioned), but it still means the code in WebStart.php and Command.inc is file position dependent. If this is the best that can be done, then that is that. However, it would really be better if all position dependent code could be eliminated. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On Tue, Aug 11, 2009 at 1:40 PM, dan nessettdness...@yahoo.com wrote: Brion Vibber br...@wikimedia.org wrote: Unless there's some reason to do otherwise, I'd recommend dropping the $IP from the autogen'd LocalSettings.php and pulling in DefaultSettings.php from the level above. (Keeping in mind that we should retain compat with existing LocalSettings.php files that are still pulling it.) Better, but what about /config/index.php, noMediaWikiConfig.php, validate.php and Maintenance.php? Having only two different places where $IP is computed is a definite improvement (assuming you fix the 4 files just mentioned), but it still means the code in WebStart.php and Command.inc is file position dependent. If this is the best that can be done, then that is that. However, it would really be better if all position dependent code could be eliminated. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l FWIW, commandLine.inc should be considered the same as Maintenance.php for these purposes, as the latter is slated the replace the former over time. -Chad ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
--- On Tue, 8/11/09, Aryeh Gregor simetrical+wikil...@gmail.com wrote: Then you're doing almost exactly the same thing we're doing now, except with MWInit.php instead of LocalSettings.php. $IP is normally set in LocalSettings.php for most page views. Some places still must figure it out independently in either case, e.g., config/index.php. I want to avoid seeming obsessed by this issue, but file position dependent code is a significant generator of bugs in other software. The difference between MWInit.php and LocalSettings.php is if you get the former into a directory that PHP uses for includes, you have a way of getting the root path of MW without the caller knowing anything about the relative structure of the code distribution tree hierarchy. As you pointed out previously, the reason you need to compute $IP before including/requiring LocalSettings is you don't know where it is. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On Tue, Aug 11, 2009 at 1:50 PM, dan nessettdness...@yahoo.com wrote: --- On Tue, 8/11/09, Aryeh Gregor simetrical+wikil...@gmail.com wrote: Then you're doing almost exactly the same thing we're doing now, except with MWInit.php instead of LocalSettings.php. $IP is normally set in LocalSettings.php for most page views. Some places still must figure it out independently in either case, e.g., config/index.php. I want to avoid seeming obsessed by this issue, but file position dependent code is a significant generator of bugs in other software. The difference between MWInit.php and LocalSettings.php is if you get the former into a directory that PHP uses for includes, you have a way of getting the root path of MW without the caller knowing anything about the relative structure of the code distribution tree hierarchy. As you pointed out previously, the reason you need to compute $IP before including/requiring LocalSettings is you don't know where it is. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l But you still have to figure out where MWInit is, which is basically the same problem. -Chad ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On Tue, 11 Aug 2009, dan nessett wrote: --- On Tue, 8/11/09, Aryeh Gregor simetrical+wikil...@gmail.com wrote: Then you're doing almost exactly the same thing we're doing now, except with MWInit.php instead of LocalSettings.php. $IP is normally set in LocalSettings.php for most page views. Some places still must figure it out independently in either case, e.g., config/index.php. I want to avoid seeming obsessed by this issue, but file position dependent code is a significant generator of bugs in other software. The difference between MWInit.php and LocalSettings.php is if you get the former into a directory that PHP uses for includes, you have a way of getting the root path of MW without the caller knowing anything about the relative structure of the code distribution tree hierarchy. As you pointed out previously, the reason you need to compute $IP before including/requiring LocalSettings is you don't know where it is. Dan Placing it in the include path could make it hard to run more than one version of the MW code on the same server, since both would probably find the same file and one of them would likely end up using the other one's $IP. Another way of putting it is, is it really better to hard-code the absolute position of the MW root rather than its position relative to the files in it? lw___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
--- On Tue, 8/11/09, Brion Vibber br...@wikimedia.org wrote: I'm not sure there's a compelling reason to even have $IP set in LocalSettings.php anymore; the base include path should probably be autodetected in all cases, which is already being done in WebStart.php and commandLine.inc, the web and CLI initialization includes based on their locations in the file tree. I started this thread because two of the fixes in the patch for bug ticket 20112 (those for Database.t and Global.t) move the require of LocalSettings.php before the require of AutoLoader.php. This is necessary because AutoLoader.php eventually executes: require_once($IP/js2/mwEmbed/php/jsAutoloadLocalClasses.php). This is a perfect example of how file position dependent code can introduce bugs. If $IP computation is eliminated from LocalSettings.php, then both of these tests will once again fail. The tests in phase3/t/inc are not executed as the result of a web request or through a command line execution path that includes maintenance/Command.inc. They normally are executed by typing at a terminal: prove t/inc -r or, e.g., prove t/inc/Global.t. prove is a TAP protocol consumer that digests and displays the results of the tests, which are TAP protocol producers. So, eliminating $IP computation from LocalSettings would require the development of new code for these tests. That would mean there would be 4 places where $IP is computed: WebStart.php, Command.inc, /config/index.php and the t test place. Not good. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On 8/11/09 11:33 AM, dan nessett wrote: --- On Tue, 8/11/09, Brion Vibberbr...@wikimedia.org wrote: I'm not sure there's a compelling reason to even have $IP set in LocalSettings.php anymore; the base include path should probably be autodetected in all cases, which is already being done in WebStart.php and commandLine.inc, the web and CLI initialization includes based on their locations in the file tree. I started this thread because two of the fixes in the patch for bug ticket 20112 (those for Database.t and Global.t) move the require of LocalSettings.php before the require of AutoLoader.php. This is necessary because AutoLoader.php eventually executes: require_once($IP/js2/mwEmbed/php/jsAutoloadLocalClasses.php). These scripts should simply be updated to initialize the framework properly instead of trying to half-ass it and load individual classes. -- brion ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
* lee worden won...@riseup.net [Tue, 11 Aug 2009 11:14:26 -0700 (PDT)]: Placing it in the include path could make it hard to run more than one version of the MW code on the same server, since both would probably find the same file and one of them would likely end up using the other one's $IP. Very true, I've had once such case. Another way of putting it is, is it really better to hard-code the absolute position of the MW root rather than its position relative to the files in it? I've heard that relative paths in require() and include() can be tricky for nested inclusions in PHP (it works correctly only for very few levels of nesting when paths go up and down). Dmitriy ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
[Wikitech-l] MW test infrastructure architecture
--- On Tue, 8/11/09, Brion Vibber br...@wikimedia.org wrote: These scripts should simply be updated to initialize the framework properly instead of trying to half-ass it and load individual classes. I agree, which is why I am trying to figure out how to consolidate the tests in /tests/ and /t/. [The example I gave was to illustrate how bugs can pop up when you use code that depends on the position of files in a distribution tree, not because I think the tests are in good shape. The bug fixes are only intended to make these tests available again, not to declare them finished.] I could use some help on test system architecture - you do wear the systems architect hat :-). It doesn't seem right to use WebStart.php to initialize the tests. For one thing, WebStart starts up profiling, which doesn't seem relevant for a test. That leaves Command.inc. However, the t tests stream TAP protocol text to prove, a PERL script that normally runs them. I have no way of running these tests through prove because my IDE doesn't support PERL, so if I changed the tests to require Command.inc, it would be hard to debug any problems. I researched other TAP consumers and didn't find anything other than prove. I was hoping that one written in PHP existed, but I haven't found anything. So, I am in kind of a bind. We could just dump the t tests, but at least one (Parser.t, which runs parserTests) is pretty useful. Furthermore, TAP has an IETF standardization effort and phpunit can produce TAP output. This suggests that TAP is a good candidate for test system infrastructure. So, what are your thoughts on this? Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
--- On Tue, 8/11/09, lee worden won...@riseup.net wrote: Placing it in the include path could make it hard to run more than one version of the MW code on the same server, since both would probably find the same file and one of them would likely end up using the other one's $IP. That is a good point. However, I don't think it is insurmountable. Callers to MWInit() could pass their path (which they can get calling realpath( '.' )). In a multi-MW environment MWInit() could disambiguate the root path by searching the provided path against those of all installed root paths. Another way of putting it is, is it really better to hard-code the absolute position of the MW root rather than its position relative to the files in it? Well, I think so. Hardcoding the absolute position of the MW root occurs at install time. Using file position dependent code is a development time dependency. Files are not moved around once installed, but could be moved around during the development process. So, bugs that depend on file position are normally not caused by installation activity, but rather by development activity. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
On 8/11/09 12:45 PM, dan nessett wrote: --- On Tue, 8/11/09, lee wordenwon...@riseup.net wrote: Placing it in the include path could make it hard to run more than one version of the MW code on the same server, since both would probably find the same file and one of them would likely end up using the other one's $IP. That is a good point. However, I don't think it is insurmountable. Callers to MWInit() could pass their path (which they can get calling realpath( '.' )). In a multi-MW environment MWInit() could disambiguate the root path by searching the provided path against those of all installed root paths. Another way of putting it is, is it really better to hard-code the absolute position of the MW root rather than its position relative to the files in it? Well, I think so. Hardcoding the absolute position of the MW root occurs at install time. Using file position dependent code is a development time dependency. Files are not moved around once installed, but could be moved around during the development process. So, bugs that depend on file position are normally not caused by installation activity, but rather by development activity. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l What seems to be being discussed here are particular offensive areas of MediaWiki - however if you really get to know MediaWiki you will likely find tons of these things everywhere... So are we proposing a specific change that will provide a solution to a problem or just nit-picking? I ask cause I'm wondering if I should ignore this thread or not (an others are probably wondering the same) - and I'm sort of feeling like this is becoming one of those threads where the people discussing things spend more time and effort battling each other than just fixing the code. - Trevor ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On Tue, Aug 11, 2009 at 3:35 PM, dan nessettdness...@yahoo.com wrote: --- On Tue, 8/11/09, Brion Vibber br...@wikimedia.org wrote: These scripts should simply be updated to initialize the framework properly instead of trying to half-ass it and load individual classes. I agree, which is why I am trying to figure out how to consolidate the tests in /tests/ and /t/. [The example I gave was to illustrate how bugs can pop up when you use code that depends on the position of files in a distribution tree, not because I think the tests are in good shape. The bug fixes are only intended to make these tests available again, not to declare them finished.] I could use some help on test system architecture - you do wear the systems architect hat :-). It doesn't seem right to use WebStart.php to initialize the tests. For one thing, WebStart starts up profiling, which doesn't seem relevant for a test. That leaves Command.inc. However, the t tests stream TAP protocol text to prove, a PERL script that normally runs them. I have no way of running these tests through prove because my IDE doesn't support PERL, so if I changed the tests to require Command.inc, it would be hard to debug any problems. I researched other TAP consumers and didn't find anything other than prove. I was hoping that one written in PHP existed, but I haven't found anything. So, I am in kind of a bind. We could just dump the t tests, but at least one (Parser.t, which runs parserTests) is pretty useful. Furthermore, TAP has an IETF standardization effort and phpunit can produce TAP output. This suggests that TAP is a good candidate for test system infrastructure. So, what are your thoughts on this? Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l To be perfectly honest, I'm of the opinion that tests/ and t/ should be scrapped and it should all be done over, properly. What we need is an easy and straightforward way to write test cases, so people are encouraged to write them. Right now, nobody understands wtf is going on in tests/ and t/, so they get ignored and the /vast/ majority of the code isn't tested. What we need is something similar to parser tests, where it's absurdly easy to pop new tests in with little to no coding required at all. Also, extensions having the ability to inject their own tests into the framework is a must IMHO. -Chad ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
Le 11 août 09 à 22:03, Chad a écrit : To be perfectly honest, I'm of the opinion that tests/ and t/ should be scrapped and it should all be done over, properly. What we need is an easy and straightforward way to write test cases, so people are encouraged to write them. Right now, nobody understands wtf is going on in tests/ and t/, so they get ignored and the /vast/ majority of the code isn't tested. What we need is something similar to parser tests, where it's absurdly easy to pop new tests in with little to no coding required at all. Also, extensions having the ability to inject their own tests into the framework is a must IMHO. -Chad +1, we could maybe write our own test system that can be based on the new Maintenance class, since we already some test scripts in / maintenance/ (cdb-test.php, fuzz-tester.php, parserTests.php, preprocessorFuzzTest.php and syntaxChecker.php). Porting tests such as parser to PHPUnit is a pain, since it has no native way to write a test suite that has a unknow number of tests to run. Alexandre Emsenhuber (ialex) ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] A potential land mine
--- On Tue, 8/11/09, Trevor Parscal tpars...@wikimedia.org wrote: Not to worry. I've given up on this issue, at least for the moment. Dan What seems to be being discussed here are particular offensive areas of MediaWiki - however if you really get to know MediaWiki you will likely find tons of these things everywhere... So are we proposing a specific change that will provide a solution to a problem or just nit-picking? I ask cause I'm wondering if I should ignore this thread or not (an others are probably wondering the same) - and I'm sort of feeling like this is becoming one of those threads where the people discussing things spend more time and effort battling each other than just fixing the code. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On 8/11/09 12:35 PM, dan nessett wrote: I could use some help on test system architecture - you do wear the systems architect hat :-). It doesn't seem right to use WebStart.php to initialize the tests. For one thing, WebStart starts up profiling, which doesn't seem relevant for a test. That leaves Command.inc. commandLine.inc would be correct, as they are command-line, not web scripts. -- brion ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
--- On Tue, 8/11/09, Chad innocentkil...@gmail.com wrote: To be perfectly honest, I'm of the opinion that tests/ and t/ should be scrapped and it should all be done over, properly. What we need is an easy and straightforward way to write test cases, so people are encouraged to write them. Right now, nobody understands wtf is going on in tests/ and t/, so they get ignored and the /vast/ majority of the code isn't tested. What we need is something similar to parser tests, where it's absurdly easy to pop new tests in with little to no coding required at all. Also, extensions having the ability to inject their own tests into the framework is a must IMHO. There is a way to easily add tests, but it requires some community discipline. phpunit has a --skeleton command (actually two variations of it) that automatically generates unit tests. (see http://www.phpunit.de/manual/current/en/skeleton-generator.html). All developers have to do is add assertions (which have the appearance of comments) to their code and call phpunit with the --skeleton flag. If you want even more hand holding, Netbeans will do it for you. This is all wonderful, but there are problems: * Who is going to go back and create all of the assertions for existing code? Not me (at least not alone). This is too big a job for one person. In order for this to work, you need buy in from at least a reasonable number of developers. So far, the number of developers expressing an interest in code quality and testing is pretty small. * What motivation is there for those creating new code to do the work to add assertions with good code coverage? So far I haven't seen anything in the MW code development process that would encourage a developer to do this. Without some carrots (and maybe a few sticks) this approach has failure written all over it. * Even if we get a bunch of Unit tests, how are they integrated into a useful whole? That requires some decisions on test infrastructure. This thread begins the discussion on that, but it really needs a lot more. * MW has a culture problem. Up to this point people just sling code into trunk and think they are done. As far as I can tell, very few feel they have any responsibility for ensuring their code won't break the product. [Perhaps I am being unkind on this. Without any testing tools available, it is quite possible that developers want to ensure the quality of their code, but don't have the means of doing so.] I realize these observations may make me unpopular. However, someone has to make them. If everyone just gets mad, it doesn't solve the problem. It just pushes it out to a time when it is even more serious. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On 8/11/09 1:03 PM, Chad wrote: To be perfectly honest, I'm of the opinion that tests/ and t/ should be scrapped and it should all be done over, properly. What we need is an easy and straightforward way to write test cases, so people are encouraged to write them. Right now, nobody understands wtf is going on in tests/ and t/, so they get ignored and the /vast/ majority of the code isn't tested. What we need is something similar to parser tests, where it's absurdly easy to pop new tests in with little to no coding required at all. Also, extensions having the ability to inject their own tests into the framework is a must IMHO. If we don't want to use straight PHPUnit or similar -- where you're writing PHP source code with a tiny bit of structure -- we could either adapt another surrounding structure or make up our own (scarrry!) Note that PHP's own test suite uses a file-per-test-case structure which is similar to the individual chunks of our parser test definition file, with delimited sections listing the name, some options, the source to run, and the expected output... php-5.2.5/tests/strings/bug22592.phpt: --TEST-- Bug #22592 (Cascading assignments to strings with curly braces broken) --FILE-- ?php $wrong = $correct = 'abcdef'; $t = $x[] = 'x'; var_dump($correct); var_dump($wrong); $correct[1] = '*'; $correct[3] = '*'; $correct[5] = '*'; // This produces the $wrong[1] = $wrong[3] = $wrong[5] = '*'; var_dump($correct); var_dump($wrong); ? --EXPECT-- string(6) abcdef string(6) abcdef string(6) a*c*e* string(6) a*c*e* It might be pretty handy actually to use a similar structure for all our tests, whether run through the parser tester or as PHP code; we could provide a standard setup/teardown environment (eg an implied commandLine.inc include and switch over to standard options and test database) so the code segment doesn't need to be a standalone PHP file. Note that some of the parser test cases rely on setup such as saving some pages into the test database; tests such as a search test could also require custom database setup or the use of mock objects, and we should consider this in planning out potential new test system requirements. -- brion ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On 8/11/09 1:39 PM, dan nessett wrote: * MW has a culture problem. Up to this point people just sling code into trunk and think they are done. As far as I can tell, very few feel they have any responsibility for ensuring their code won't break the product. [Perhaps I am being unkind on this. Without any testing tools available, it is quite possible that developers want to ensure the quality of their code, but don't have the means of doing so.] Starting about a week ago, parser test results are now included in code review listings for development trunk: http://www.mediawiki.org/w/index.php?title=Special:Code/MediaWiki/pathpath=%2Ftrunk%2Fphase3 Regressions are now quickly noted and fixed up within a few revisions -- something which didn't happen when they were only being run manually by a few folks here and there. Is this the sort of thing you're thinking of? -- brion ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
--- On Tue, 8/11/09, Alexandre Emsenhuber alex.emsenhu...@bluewin.ch wrote: +1, we could maybe write our own test system that can be based on the new Maintenance class, since we already some test scripts in / maintenance/ (cdb-test.php, fuzz-tester.php, parserTests.php, preprocessorFuzzTest.php and syntaxChecker.php). Porting tests such as parser to PHPUnit is a pain, since it has no native way to write a test suite that has a unknow number of tests to run. Rewriting parserTests as php unit tests would be a horrible waste of time. parserTests works and it provides a reasonable service. One problem, however, is how do we fix the parser? It seems it is a pretty complex code system (when I ran a MacGyver test on parserTests, 141 files were accessed, most of which are associated with the parser). I have been thinking about this, but those thoughts are not yet sufficiently clear to make public yet. On the other hand, taking the parserTests route and doing all of our own test infrastructure would also be a good deal of work. There are tools out there (phpuint and prove) that are useful. In my view creating a test infrastructure from scratch would unnecessarily waste time and resources. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
Hi, On 2009-08-11, dan nessett wrote: There is a way to easily add tests, but it requires some community discipline. phpunit has a --skeleton command (actually two variations of it) that automatically generates unit tests. (see http://www.phpunit.de/manual/current/en/skeleton-generator.html). All developers have to do is add assertions (which have the appearance of comments) to their code and call phpunit with the --skeleton flag. If you want even more hand holding, Netbeans will do it for you. This is all wonderful, but there are problems: * Who is going to go back and create all of the assertions for existing code? Not me (at least not alone). This is too big a job for one person. In order for this to work, you need buy in from at least a reasonable number of developers. So far, the number of developers expressing an interest in code quality and testing is pretty small. This doesn't need to be done straight away, providing all new code is has proper unit tests built for it the old stuff will get done eventually or when it breaks. * What motivation is there for those creating new code to do the work to add assertions with good code coverage? So far I haven't seen anything in the MW code development process that would encourage a developer to do this. Without some carrots (and maybe a few sticks) this approach has failure written all over it. We don't do unit tests properly yet, that's probably why. If there is a decent infrastructure in place then it might be possible to encourage developers to do this. * Even if we get a bunch of Unit tests, how are they integrated into a useful whole? That requires some decisions on test infrastructure. This thread begins the discussion on that, but it really needs a lot more. We have started integrating parser tests in to code review, so something like this could be done for the unit tests so they won't stagnate, etc. * MW has a culture problem. Up to this point people just sling code into trunk and think they are done. As far as I can tell, very few feel they have any responsibility for ensuring their code won't break the product. [Perhaps I am being unkind on this. Without any testing tools available, it is quite possible that developers want to ensure the quality of their code, but don't have the means of doing so.] I completely agree, blatant syntax errors are regularly being checked in - and now there is a post-commit review process very few developers actually get someone else to look over their patches before commiting them which is far from ideal IMO. I realize these observations may make me unpopular. However, someone has to make them. If everyone just gets mad, it doesn't solve the problem. It just pushes it out to a time when it is even more serious. I don't think you have anything to worry about, you bring up some valid points that are more often that not, ignored. Dan Please can you properly break your lines in e-mail though, to 73(?) characters per a line - should be possible to specify this in your client. -- Robert Leverington http://rhl.me.uk/ signature.asc Description: Digital signature ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On Tue, Aug 11, 2009 at 2:55 PM, Robert Leverington rob...@rhl.me.ukwrote: Please can you properly break your lines in e-mail though, to 73(?) characters per a line - should be possible to specify this in your client. That's a joke right? If you are using pine on a monitor built in the 1980s then you are fully expected to BYOLB (Bring Your Own Line Breaks). ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
2009/8/11 Brian brian.min...@colorado.edu: On Tue, Aug 11, 2009 at 2:55 PM, Robert Leverington rob...@rhl.me.ukwrote: Please can you properly break your lines in e-mail though, to 73(?) characters per a line - should be possible to specify this in your client. That's a joke right? If you are using pine on a monitor built in the 1980s then you are fully expected to BYOLB (Bring Your Own Line Breaks). ^X to word-wrap the para, isn't it? - d. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
Brion Vibber br...@wikimedia.org wrote: Starting about a week ago, parser test results are now included in code review listings for development trunk: http://www.mediawiki.org/w/index.php?title=Special:Code/MediaWiki/pathpath=%2 Ftrunk%2Fphase3 Regressions are now quickly noted and fixed up within a few revisions -- something which didn't happen when they were only being run manually by a few folks here and there. Is this the sort of thing you're thinking of? -- brion Yes. Absolutely. Sight is critical for action and running parserTests on each revision in the development trunk is a good first step on improving code quality. Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
--- On Tue, 8/11/09, Robert Leverington rob...@rhl.me.uk wrote: Please can you properly break your lines in e-mail though, to 73(?) characters per a line - should be possible to specify this in your client. I'm using the web interface provided by yahoo. If you can point me in the right direction for setting up yahoo to do this I'll be happy to (I've done this manually on this message). Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
Le 11 août 09 à 22:51, dan nessett a écrit : --- On Tue, 8/11/09, Alexandre Emsenhuber alex.emsenhu...@bluewin.ch wrote: +1, we could maybe write our own test system that can be based on the new Maintenance class, since we already some test scripts in / maintenance/ (cdb-test.php, fuzz-tester.php, parserTests.php, preprocessorFuzzTest.php and syntaxChecker.php). Porting tests such as parser to PHPUnit is a pain, since it has no native way to write a test suite that has a unknow number of tests to run. Rewriting parserTests as php unit tests would be a horrible waste of time. parserTests works and it provides a reasonable service. One problem, however, is how do we fix the parser? It seems it is a pretty complex code system (when I ran a MacGyver test on parserTests, 141 files were accessed, most of which are associated with the parser). I have been thinking about this, but those thoughts are not yet sufficiently clear to make public yet. On the other hand, taking the parserTests route and doing all of our own test infrastructure would also be a good deal of work. There are tools out there (phpuint and prove) that are useful. In my view creating a test infrastructure from scratch would unnecessarily waste time and resources. Dan My idea is the move the backend of ParserTest (parserTests.txt file processing, result reporting, ...) and the TestRecorder stuff to something like a MediaWikiTests class that extends Maintenance and move the rest in a file in /maintenance/tests/ (to be created) and re- use the backend to have files that have the same format, but test's input could be raw PHP code (a bit like PHP core's tests) with a new config variable that's like $wgParserTestFiles but for these kind of test. This mostly concerns the actual tests in /tests/ and /t/inc/). We can also port cdb-test.php, fuzz-tester.php, preprocessorFuzzTest.php and syntaxChecker.php to this new system and then create a script in /maintenance/ that runs all the tests in / maintenance/tests/. This allows to also upload all the tests to CodeReview, not only the parser tests. A benefit is that we can get ride of /tests/ and /t/. Alexandre Emsenhuber (ialex) ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
--- On Tue, 8/11/09, Alexandre Emsenhuber alex.emsenhu...@bluewin.ch wrote: My idea is the move the backend of ParserTest (parserTests.txt file processing, result reporting, ...) and the TestRecorder stuff to something like a MediaWikiTests class that extends Maintenance and move the rest in a file in /maintenance/tests/ (to be created) and re- use the backend to have files that have the same format, but test's input could be raw PHP code (a bit like PHP core's tests) with a new config variable that's like $wgParserTestFiles but for these kind of test. This mostly concerns the actual tests in /tests/ and /t/inc/). We can also port cdb-test.php, fuzz-tester.php, preprocessorFuzzTest.php and syntaxChecker.php to this new system and then create a script in /maintenance/ that runs all the tests in / maintenance/tests/. This allows to also upload all the tests to CodeReview, not only the parser tests. A benefit is that we can get ride of /tests/ and /t/. One of the beauties of open source code development is he who does the work wins the prize. Of course, I am sure senior developers have discretionary power on what goes into a release and what does not. But, if you want to do the work, go for it (says the guy [me] who just joined the group). However, I think you should consider the following: * parserTests is designed to test parsing, which is predominantly a text manipulation task. Other parts of MW do not necessarily provide text processing markers that can be used to decide whether they are working correctly or not. * Sometimes testing the action of a module requires determining whether a series of actions provide the correct behavior. As far as I am aware, parserTests has no facility to tie together a set of actions into a single test. For example, consider two MW files in phase3/includes: 1) AutoLoader.php and 2) Hooks.php. In AutoLoader, the method loadAllExtensions() loads all extensions specified in $wgAutoloadClasses. It takes no parameters and has no return value. It simply walks through the entries specified in $wgAutoloadClasses and if the class specified as the key exists, executes a require of the file specified in the value. I don't see how you would specify a test of this method using the syntax of parserTests.txt. In Hooks.php, there is a single function wfRunHooks(). It looks up hooks previously set and calls user code for them. It throws exceptions in certain error conditions and testing it requires setting a hook and seeing if it is called appropriately. I don't see how you could describe this behavior with parserTests.txt syntax. Of course, you could create new syntax and behavior for the parserTests software components, but that is a lot of work that other infrastructure has already done. For example, see the set of assertions for phpunit (http://www.phpunit.de/manual/2.3/en/api.html#api.assert.tables.assertions). Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On Tue, Aug 11, 2009 at 6:39 PM, dan nessettdness...@yahoo.com wrote: For example, consider two MW files in phase3/includes: 1) AutoLoader.php and 2) Hooks.php. In AutoLoader, the method loadAllExtensions() loads all extensions specified in $wgAutoloadClasses. It takes no parameters and has no return value. It simply walks through the entries specified in $wgAutoloadClasses and if the class specified as the key exists, executes a require of the file specified in the value. I don't see how you would specify a test of this method using the syntax of parserTests.txt. In Hooks.php, there is a single function wfRunHooks(). It looks up hooks previously set and calls user code for them. It throws exceptions in certain error conditions and testing it requires setting a hook and seeing if it is called appropriately. I don't see how you could describe this behavior with parserTests.txt syntax. Neither of these need to be tested directly. If AutoLoader breaks, then some other class won't load, and the tests for that class will fail. If wfRunHooks() fails, then some hook won't work, and any test of that hook will fail. I think what's needed for decent test usage for MediaWiki is: 1) Some test suite is picked. PHPUnit is probably fine, if it runs out of the box and doesn't need some extra module to be installed. 2) The test suite is integrated into CodeReview with nag e-mails for broken tests. 3) A moderate number of tests are written for the test suite. Existing parser tests could be autoconverted, possibly. Maybe someone paid could be assigned to spend a day or two on this. 4) A new policy requires that everyone write tests for all their bug fixes and enhancements. Commits that don't add enough tests will be flagged as fixme, and reverted if not fixed. (4) is critical here. While we're at it, it would be nice if we instituted some other iron-clad policies. Here's a proposal: * All new functions (including private helper functions, functions in JavaScript includes, whatever) must have function-level documentation that explains the purpose of the function and describes its parameters. The documentation must be enough that no MediaWiki developer should ever have to read the function's source code to be able to use it correctly. Exception: if a method is overridden which is already documented in the base class, it doesn't need to be documented again in the derived class, since the semantics should be the same. * All new classes must have high-level documentation that explains their purpose and structure, and how you should use them. The documentation must be sufficient that any MediaWiki developer could understand why they might want to use the class in another file, and how they could do so, without reading any of the source code. Of course, developers would still have to read the function documentation to learn how to use specific functions. There are no exceptions, but a derived class might only need very brief documentation. * All new config variables must have documentation explaining what they do in terms understandable to end-users. They must describe what values are accepted, and if the values are complicated (like associative arrays), must provide at least one example that can be copy-pasted. There are no exceptions. * If any code is changed so as to make a comment incorrect, the comment must be updated to match. There are no exceptions. Or whatever. We have *way* too few high-level comments in our code. We have entire files -- added quite recently, mind you, by established developers -- that have no or almost no documentation on either the class or function level. We can really do better than this! If we had a clear list of requirements for comments in new code, we could start fixme'ing commits that don't have adequate comments. I think that would be enough to get people to add sufficient comments, for the most part. Without clear rules, though, backed up by the threat of reverting, I don't think the situation will improve here. (Wow, this kind of turned into a thread hijack. :D) ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On Tue, Aug 11, 2009 at 8:10 PM, Aryeh Gregorsimetrical+wikil...@gmail.com wrote: On Tue, Aug 11, 2009 at 6:39 PM, dan nessettdness...@yahoo.com wrote: For example, consider two MW files in phase3/includes: 1) AutoLoader.php and 2) Hooks.php. In AutoLoader, the method loadAllExtensions() loads all extensions specified in $wgAutoloadClasses. It takes no parameters and has no return value. It simply walks through the entries specified in $wgAutoloadClasses and if the class specified as the key exists, executes a require of the file specified in the value. I don't see how you would specify a test of this method using the syntax of parserTests.txt. In Hooks.php, there is a single function wfRunHooks(). It looks up hooks previously set and calls user code for them. It throws exceptions in certain error conditions and testing it requires setting a hook and seeing if it is called appropriately. I don't see how you could describe this behavior with parserTests.txt syntax. Neither of these need to be tested directly. If AutoLoader breaks, then some other class won't load, and the tests for that class will fail. If wfRunHooks() fails, then some hook won't work, and any test of that hook will fail. I think what's needed for decent test usage for MediaWiki is: 1) Some test suite is picked. PHPUnit is probably fine, if it runs out of the box and doesn't need some extra module to be installed. 2) The test suite is integrated into CodeReview with nag e-mails for broken tests. 3) A moderate number of tests are written for the test suite. Existing parser tests could be autoconverted, possibly. Maybe someone paid could be assigned to spend a day or two on this. 4) A new policy requires that everyone write tests for all their bug fixes and enhancements. Commits that don't add enough tests will be flagged as fixme, and reverted if not fixed. (4) is critical here. While we're at it, it would be nice if we instituted some other iron-clad policies. Here's a proposal: * All new functions (including private helper functions, functions in JavaScript includes, whatever) must have function-level documentation that explains the purpose of the function and describes its parameters. The documentation must be enough that no MediaWiki developer should ever have to read the function's source code to be able to use it correctly. Exception: if a method is overridden which is already documented in the base class, it doesn't need to be documented again in the derived class, since the semantics should be the same. * All new classes must have high-level documentation that explains their purpose and structure, and how you should use them. The documentation must be sufficient that any MediaWiki developer could understand why they might want to use the class in another file, and how they could do so, without reading any of the source code. Of course, developers would still have to read the function documentation to learn how to use specific functions. There are no exceptions, but a derived class might only need very brief documentation. * All new config variables must have documentation explaining what they do in terms understandable to end-users. They must describe what values are accepted, and if the values are complicated (like associative arrays), must provide at least one example that can be copy-pasted. There are no exceptions. * If any code is changed so as to make a comment incorrect, the comment must be updated to match. There are no exceptions. Or whatever. We have *way* too few high-level comments in our code. We have entire files -- added quite recently, mind you, by established developers -- that have no or almost no documentation on either the class or function level. We can really do better than this! If we had a clear list of requirements for comments in new code, we could start fixme'ing commits that don't have adequate comments. I think that would be enough to get people to add sufficient comments, for the most part. Without clear rules, though, backed up by the threat of reverting, I don't think the situation will improve here. (Wow, this kind of turned into a thread hijack. :D) ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l On the whole new code front. Can we all /please/ remember that we're writing PHP5 here. Visibility on all new functions and variables should also be a must. -Chad ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
--- On Tue, 8/11/09, Chad innocentkil...@gmail.com wrote: Neither of these need to be tested directly. If AutoLoader breaks, then some other class won't load, and the tests for that class will fail. If wfRunHooks() fails, then some hook won't work, and any test of that hook will fail. I will pass on commenting about these for the moment because: I think what's needed for decent test usage for MediaWiki is: 1) Some test suite is picked. PHPUnit is probably fine, if it runs out of the box and doesn't need some extra module to be installed. 2) The test suite is integrated into CodeReview with nag e-mails for broken tests. 3) A moderate number of tests are written for the test suite. Existing parser tests could be autoconverted, possibly. Maybe someone paid could be assigned to spend a day or two on this. 4) A new policy requires that everyone write tests for all their bug fixes and enhancements. Commits that don't add enough tests will be flagged as fixme, and reverted if not fixed. (4) is critical here. All good stuff, especially (4) - applause :-D. While we're at it, it would be nice if we instituted some other iron-clad policies. Here's a proposal: * All new functions (including private helper functions, functions in JavaScript includes, whatever) must have function-level documentation that explains the purpose of the function and describes its parameters. The documentation must be enough that no MediaWiki developer should ever have to read the function's source code to be able to use it correctly. Exception: if a method is overridden which is already documented in the base class, it doesn't need to be documented again in the derived class, since the semantics should be the same. * All new classes must have high-level documentation that explains their purpose and structure, and how you should use them. The documentation must be sufficient that any MediaWiki developer could understand why they might want to use the class in another file, and how they could do so, without reading any of the source code. Of course, developers would still have to read the function documentation to learn how to use specific functions. There are no exceptions, but a derived class might only need very brief documentation. * All new config variables must have documentation explaining what they do in terms understandable to end-users. They must describe what values are accepted, and if the values are complicated (like associative arrays), must provide at least one example that can be copy-pasted. There are no exceptions. * If any code is changed so as to make a comment incorrect, the comment must be updated to match. There are no exceptions. Or whatever. We have *way* too few high-level comments in our code. We have entire files -- added quite recently, mind you, by established developers -- that have no or almost no documentation on either the class or function level. We can really do better than this! If we had a clear list of requirements for comments in new code, we could start fixme'ing commits that don't have adequate comments. I think that would be enough to get people to add sufficient comments, for the most part. Without clear rules, though, backed up by the threat of reverting, I don't think the situation will improve here. Wonderful stuff - more applause. (Wow, this kind of turned into a thread hijack. :D) Who cares. It needs to be said. On the whole new code front. Can we all /please/ remember that we're writing PHP5 here. Visibility on all new functions and variables should also be a must. OK, I must admit I didn't understand that, probably because I'm new to PHP. Can you make this more explicit? Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
On Tue, Aug 11, 2009 at 8:29 PM, dan nessettdness...@yahoo.com wrote: --- On Tue, 8/11/09, Chad innocentkil...@gmail.com wrote: Neither of these need to be tested directly. If AutoLoader breaks, then some other class won't load, and the tests for that class will fail. If wfRunHooks() fails, then some hook won't work, and any test of that hook will fail. I will pass on commenting about these for the moment because: I think what's needed for decent test usage for MediaWiki is: 1) Some test suite is picked. PHPUnit is probably fine, if it runs out of the box and doesn't need some extra module to be installed. 2) The test suite is integrated into CodeReview with nag e-mails for broken tests. 3) A moderate number of tests are written for the test suite. Existing parser tests could be autoconverted, possibly. Maybe someone paid could be assigned to spend a day or two on this. 4) A new policy requires that everyone write tests for all their bug fixes and enhancements. Commits that don't add enough tests will be flagged as fixme, and reverted if not fixed. (4) is critical here. All good stuff, especially (4) - applause :-D. While we're at it, it would be nice if we instituted some other iron-clad policies. Here's a proposal: * All new functions (including private helper functions, functions in JavaScript includes, whatever) must have function-level documentation that explains the purpose of the function and describes its parameters. The documentation must be enough that no MediaWiki developer should ever have to read the function's source code to be able to use it correctly. Exception: if a method is overridden which is already documented in the base class, it doesn't need to be documented again in the derived class, since the semantics should be the same. * All new classes must have high-level documentation that explains their purpose and structure, and how you should use them. The documentation must be sufficient that any MediaWiki developer could understand why they might want to use the class in another file, and how they could do so, without reading any of the source code. Of course, developers would still have to read the function documentation to learn how to use specific functions. There are no exceptions, but a derived class might only need very brief documentation. * All new config variables must have documentation explaining what they do in terms understandable to end-users. They must describe what values are accepted, and if the values are complicated (like associative arrays), must provide at least one example that can be copy-pasted. There are no exceptions. * If any code is changed so as to make a comment incorrect, the comment must be updated to match. There are no exceptions. Or whatever. We have *way* too few high-level comments in our code. We have entire files -- added quite recently, mind you, by established developers -- that have no or almost no documentation on either the class or function level. We can really do better than this! If we had a clear list of requirements for comments in new code, we could start fixme'ing commits that don't have adequate comments. I think that would be enough to get people to add sufficient comments, for the most part. Without clear rules, though, backed up by the threat of reverting, I don't think the situation will improve here. Wonderful stuff - more applause. (Wow, this kind of turned into a thread hijack. :D) Who cares. It needs to be said. On the whole new code front. Can we all /please/ remember that we're writing PHP5 here. Visibility on all new functions and variables should also be a must. OK, I must admit I didn't understand that, probably because I'm new to PHP. Can you make this more explicit? Dan ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l This is slightly OT and I don't want to hijack the main point (getting our shit together for testing), but... PHP4 did not have proper object oriented support. There was no concept of visibility--all member variables and functions were public. PHP5 fixed this by adding the familiar public/protected/private structure, as well as a bunch of other things that a proper OO language should be able to do. My point is that not declaring visibility on new code (ie: leaving everything public) is poor form. It usually doesn't take very long to make the decision about what scope to put a function or variable in, and it can always be changed later if the choice was wrong. -Chad ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
