Re: [Wikitech-l] MW test infrastructure architecture

[Mark Clements (HappyDog)]

"Bryan Tong Minh"  wrote in message 
news:fd5886130908120137h13b0aad9xdd2f05c8a362c...@mail.gmail.com...
> On Wed, Aug 12, 2009 at 2:38 AM, Chad 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 (private>protected>public), 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

[Chad]

On Wed, Aug 12, 2009 at 6:40 PM, Tim Landscheidt wrote:
> Chad  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", "Linktrails should not work 
> for external links: http://example.com\"; class=\"external text\" 
> rel=\"nofollow\">links\n");
>
> than
>
> | !! test
> | External links: trail
> | !! input
> | Linktrails should not work for external links: [http://example.com link]s
> | !! result
> | Linktrails should not work for external links:  href="http://example.com"; class="external text" rel="nofollow">links
> | 
> | !! 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

Re: [Wikitech-l] MW test infrastructure architecture

[Tim Landscheidt]

Chad  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", "Linktrails should not work for external 
links: http://example.com\"; class=\"external text\" 
rel=\"nofollow\">links\n");

than

| !! test
| External links: trail
| !! input
| Linktrails should not work for external links: [http://example.com link]s
| !! result
| Linktrails should not work for external links: http://example.com"; class="external text" rel="nofollow">links
| 
| !! 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

[Aryeh Gregor]

On Wed, Aug 12, 2009 at 8:31 AM, Andrew Garrett 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 Vasiliev 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

[Victor Vasiliev]

Aryeh Gregor wrote:
> 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.

What comment would you except to "public function getArticleText()"?

By the way, is there a policy which describes where should we use
get*/set* methods and where should we use public fields?

--vvv

___
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]

On Wed, Aug 12, 2009 at 8:31 AM, Andrew Garrett 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

[Andrew Garrett]

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

[Bryan Tong Minh]

On Wed, Aug 12, 2009 at 2:38 AM, Chad 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 (private>protected>public), but not
necessarily go down without possibly breaking things you are unaware
of.


Bryan

___
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]

On Tue, Aug 11, 2009 at 8:29 PM, dan nessett wrote:
> --- On Tue, 8/11/09, Chad  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 al

Re: [Wikitech-l] MW test infrastructure architecture

[dan nessett]

--- On Tue, 8/11/09, Chad  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

[Chad]

On Tue, Aug 11, 2009 at 8:10 PM, Aryeh
Gregor wrote:
> On Tue, Aug 11, 2009 at 6:39 PM, dan nessett 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

[Aryeh Gregor]

On Tue, Aug 11, 2009 at 6:39 PM, dan nessett 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

[dan nessett]

--- On Tue, 8/11/09, Alexandre Emsenhuber  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

[Alexandre Emsenhuber]

Le 11 août 09 à 22:51, dan nessett a écrit :

> --- On Tue, 8/11/09, Alexandre Emsenhuber  
>  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

[dan nessett]

--- On Tue, 8/11/09, Robert Leverington  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

[dan nessett]

Brion Vibber  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/path&path=%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

[David Gerard]

2009/8/11 Brian :
> On Tue, Aug 11, 2009 at 2:55 PM, Robert Leverington 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.

> 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

[Brian]

On Tue, Aug 11, 2009 at 2:55 PM, Robert Leverington 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.
>

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

[Robert Leverington]

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

[dan nessett]

--- On Tue, 8/11/09, Alexandre Emsenhuber  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

[Brion Vibber]

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/path&path=%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

[Brion Vibber]

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

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

[dan nessett]

--- On Tue, 8/11/09, 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.

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

[Brion Vibber]

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

[Alexandre Emsenhuber]

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

[Chad]

On Tue, Aug 11, 2009 at 3:35 PM, dan nessett wrote:
> --- On Tue, 8/11/09, Brion Vibber  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