Re: [Wikitech-l] MW test infrastructure architecture
Bryan Tong Minh bryan.tongm...@gmail.com wrote in message news:fd5886130908120137h13b0aad9xdd2f05c8a362c...@mail.gmail.com... On Wed, Aug 12, 2009 at 2:38 AM, Chadinnocentkil...@gmail.com wrote: 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. Well, you can always go up (privateprotectedpublic), but not necessarily go down without possibly breaking things you are unaware of. True - and not specifying a scope means the visibility defaults to public. Therefore requiring a scope to be declared can only be a good thing. - Mark Clements (HappyDog) ___ 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 12/08/2009, at 1:16 AM, Chad wrote: 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. I honestly don't think this is that important. We should err on the side of public, not private or protected. Very frequently, I've had to change visibility on functions because somebody hasn't anticipated a case where a class function might be usefully called from outside that class. -- Andrew Garrett agarr...@wikimedia.org http://werdn.us/ ___ 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 Wed, Aug 12, 2009 at 8:31 AM, Andrew Garrettagarr...@wikimedia.org wrote: On 12/08/2009, at 1:16 AM, Chad wrote: 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. I honestly don't think this is that important. We should err on the side of public, not private or protected. Very frequently, I've had to change visibility on functions because somebody hasn't anticipated a case where a class function might be usefully called from outside that class. -- Andrew Garrett agarr...@wikimedia.org http://werdn.us/ ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l When writing maintenance, I tried to make very concious decisions about what visibility to put on each method. For clearly defined public interfaces, I made the methods public. Some of the setup functions don't need accessing and I probably don't want children implementing them, so I made them private. I think making careful decisions when coding can lead to well-written classes with proper visibility. If a particular method/variable needs to become public at a later point, it can easily be changed. My main point is not so much make things private but at least look like we're writing code in the last 4 years. -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 Wed, Aug 12, 2009 at 8:31 AM, Andrew Garrettagarr...@wikimedia.org wrote:
I honestly don't think this is that important. We should err on the
side of public, not private or protected.
Very frequently, I've had to change visibility on functions because
somebody hasn't anticipated a case where a class function might be
usefully called from outside that class.
On the other hand, I've often wanted to refactor code but been unable
to because the functions were public or protected and I didn't want to
break any third-party extensions that might call them. If a function
is private and could usefully be public, then just make it public, no
problem.
The only people who are harmed by too many private functions are
people without commit access, who can't just make the functions
public. Since these are also the only people whose code could be
broken by changing a public function, though -- if the code is
committed, then the one who changes the public function can just
change the caller -- you can make a good case that it's better to make
everything public, and let extension authors rely on the public
functions if they want without any guarantee that the functions will
remain stable. They'd probably prefer that to having the function
private and not being able to even try to use it.
Generally speaking, I always make my functions private if I don't
explicitly intend to make them part of the class interface for
third-party use. But I can see the other side too, so I don't know if
a firm policy is the best course of action.
On Wed, Aug 12, 2009 at 9:24 AM, Victor Vasilievvasi...@gmail.com wrote:
What comment would you except to public function getArticleText()?
Well, I can't write a very good comment since I've never read that
function or even any significant part of that class until now. (I'm
looking at RawPage::getArticleText() here.) But maybe something like:
/**
* Returns the parsed HTML corresponding to this page for action=raw output,
* and outputs some HTTP headers.
*
* @return string Parsed HTML suitable for echoing
*/
private function getArticleText() {
The documentation is cursory because it's really just a helper method
that's designed to be called in exactly one place, so anyone who wants
to use it is already making changes to RawPage and will as likely as
not want to read and/or change the source of the method.
If you don't want to make it private for some reason, then I'd add
@private to the documentation and put a note saying what method you
actually want to call. It has various side effects and
RawPage-specific logic that make it very unlikely to be useful to any
other classes.
If you were referring to a hypothetical public method in, say,
Revision, then I'd expect the comment to include all information that
callers might like to know, such as:
* Is the text parsed or unparsed? If parsed, what method should be
used to get unparsed text? If unparsed, what method to get parsed
text?
* What's the performance impact like? Does this do any batching or
caching? Does every call result in a database query? Does it call
the parser? Does it use memcached? Is it possible that this will
return outdated text, or will it definitely be up-to-date?
* Is any kind of permissions checking done? Will this retrieve text
for revisions that are deleted, oversighted, rev_deleted, or otherwise
hidden? Does it check whether the current user has read permission?
* What happens if there's an error, like the revision doesn't exist?
Is an exception thrown, or some magic value returned? What sorts of
errors should we expect to see? Can we assume there are no errors?
E.g., a low-level function that always returns the text even if
deleted, and where the revision object was created with a constructor
that verified that the revision was legit, might never return an error
unless the database couldn't be reached or was manually tampered with
(in which case the request should probably just die anyway).
The answers don't have to be lengthy, and they can refer to
documentation of other methods within reason. But they *should* be
there, in a form where most MediaWiki developers can understand them.
I think all those questions could be answered in about three or four
lines, say, depending on what the answers are. I really have to go
now, or else I'd commit a good comment for Revision::get(Raw)Text() to
show you what I mean.
In fact, Revision::getText() already has 11 lines of comments.
Revision::getRawText() has only 2. I think both could be improved a
bit.
By the way, is there a policy which describes where should we use
get*/set* methods and where should we use public fields?
I always use private member variables. Some other developers always
use public member variables. It's pretty much the same story as with
public/private methods.
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] MW test infrastructure architecture
Chad innocentkil...@gmail.com wrote: [...] 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. [...] Is it really so much more difficult to write | TestAddArticle (Main Page, blah blah); than | !! article | Main Page | !! text | blah blah | !! endarticle or | TestParser (External links: trail, Linktrails should not work for external links: [http://example.com link]s, pLinktrails should not work for external links: a href=\http://example.com\; class=\external text\ rel=\nofollow\link/as\n/p); than | !! test | External links: trail | !! input | Linktrails should not work for external links: [http://example.com link]s | !! result | pLinktrails should not work for external links: a href=http://example.com; class=external text rel=nofollowlink/as | /p | !! end I think the motivation for using standard techniques is to lower the bar for newcomers to the code and not have them to master another (fairly self-explanatory) syntax. Tim ___ 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 Wed, Aug 12, 2009 at 6:40 PM, Tim Landscheidtt...@tim-landscheidt.de wrote: Chad innocentkil...@gmail.com wrote: [...] 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. [...] Is it really so much more difficult to write | TestAddArticle (Main Page, blah blah); than | !! article | Main Page | !! text | blah blah | !! endarticle or | TestParser (External links: trail, Linktrails should not work for external links: [http://example.com link]s, pLinktrails should not work for external links: a href=\http://example.com\; class=\external text\ rel=\nofollow\link/as\n/p); than | !! test | External links: trail | !! input | Linktrails should not work for external links: [http://example.com link]s | !! result | pLinktrails should not work for external links: a href=http://example.com; class=external text rel=nofollowlink/as | /p | !! end I think the motivation for using standard techniques is to lower the bar for newcomers to the code and not have them to master another (fairly self-explanatory) syntax. Tim ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l I don't disagree here. I don't really care what the particular format is. My main point (on which I think we agree) was that it needs to be standardized, easy to learn and use, and very flexible to a variety of tests we could potentially want. -Chad ___ 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] 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] 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
