[wp-hackers] Process for submitting new unit tests

[Ian Dunn]

Thanks Bryan, that was really helpup. I'll update the Codex with some of 
that.


On 01/29/2013 12:10 AM, Bryan Petty wrote:
> On Mon, Jan 28, 2013 at 10:22 PM, Ian Dunn <ian at iandunn.name> wrote:
>> The intro section of the Automated Testing page on the Codex says,
>>
>> "The process to add your test suite and test cases to existing suites is to
>> add them to the WordPress Core Trac ticket that has the bug that the test
>> case or suite tests. After you've done this, add "unit-tested" to the
>> keywords of the ticket to inform the lead developers that it has been
>> added." -- http://codex.wordpress.org/Automated_Testing#Automated_testing
>>
>> ...but the Submitting the Test Cases section at the end says,
>>
>> " New test cases should be submitted to the main WordPress trac, setting the
>> component to 'Unit Tests.' These will then get reviewed and added to the
>> Subversion repository." --
>> http://codex.wordpress.org/Automated_Testing#Submitting_the_Test_Cases
>>
>> So, the first paragraph says to attach new tests to the ticket the tests
>> were written for, while the other paragraph says to open a second ticket
>> whose only purpose would be submitting the new tests for inclusion in the
>> suite.
>>
>> Can someone tell me which one is correct? I can fix the article once I find
>> out.
> Actually, they are both correct. New unit tests that have nothing to
> do with any existing tickets are more than welcome if the component in
> question is lacking decent tests, and those should be on their own new
> ticket. At the same time, it is still appropriate to attach unit tests
> for a specific bug to it's existing ticket.
>
> Although, I think the appropriate keyword is actually
> "has-unit-tests", and not "unit-tested". There's already at least one
> ticket using that, it follows convention with "has-patch", and I've
> never actually seen "unit-tested" used before.
>
> It might be helpful to actually figure out if the Core Contributor
> Handbook is adequate enough to remove (and redirect) that Codex page,
> preferring the CCH page here:
> http://make.wordpress.org/core/handbook/automated-testing/
>
>> Also, the article doesn't offer much in the way of guidance on what kinds of
>> new tests are desired, or how extensive they should be. I'm working on some
>> small modifications to wp_mail() in #23291, and they're relatively straight
>> forward, but I still want to be cautious about any unintended consequences.
>> The current suite only has 6 tests for wp_mail(), so there's a lot that
>> could go wrong without being detected.
>>
>> I'm probably going to write at least half a dozen tests just for my own
>> sanity, but I can submit them to the tests repo if that's desired. I know
>> some people consider excessive unit testing a waste of time, or might not
>> want to see the suite grow to the tens of thousands, or prefer system
>> testing and don't want to bother updating unit tests when internals change.
>> Is there a consensus on what should be in the suite, or any official
>> guidelines?
> This can certainly be very subjective, and there really isn't any
> official guidelines on this. The unit tests are still not anywhere
> close to extensive or have good code coverage. They are still very
> young and sparse. Really any help is appreciated with getting the
> tests filled out with at least the most common usage and
> configurations on most components (and mail is certainly a very
> heavily used one) as long as the tests do what they claim. Use your
> best judgement, but do keep a few of the following tips in mind.
>
> Right now the unit tests take over 2 minutes to run on decent
> hardware. Not as long as they used to take at one point, but still
> pretty long by most developers' standards. Testing the most important
> aspects of every component comes first in priority of course, but try
> not to push the tests on time. The tests are configured in a way that
> is very wasteful on resources (spawning new processes with every test
> suite for example - reloading all of WordPress), and hopefully that
> can be changed in the future so current tests drop under a minute, but
> many of the existing tests won't work that way for now.
>
> Given the time constraints, you might be tempted to stuff more
> assertions into single test methods (it will run faster). Many of the
> unit tests do this, some even including assertions inside of frequent
> loops. However, this is generally considered a bad practice as it
> makes it hard to tell why the test failed when it does. So try not to
> do that unless you know it will be easy to tell what's wrong assuming
> you are only given the assertion value passed in, the expected value,
> and the name of the test method.
>
> Specifically since you're talking about the mail component, I just
> want to point out that PHPMailer has it's own set of unit tests that
> are supposed to cover all aspects of PHPMailer itself. Don't add unit
> tests that mostly only test it's functionality. For example, if a WP
> mail method accepts a parameter that really is just passed directly
> into PHPMailer without being touched, you probably don't need any test
> variations based on that parameter.
>
> I'm sure some others here have some good advice to add to this too.
>
> --
> Regards,
> Bryan Petty
> _______________________________________________
> wp-hackers mailing list
> wp-hackers at lists.automattic.com
> http://lists.automattic.com/mailman/listinfo/wp-hackers