[wp-hackers] Process for submitting new unit tests

[Bryan Petty]

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