Proposal for "Unit tests, new packages, good practice"

» Metadata » Status
  • Status: Proposed
» Description
The following dot points have been pretty much shamelessly taken from http://misko.hevery.com/code-reviewers-guide/ and other related talks, writings, etc.


I would like to propose for all new packages / PEPr proposals:

1) All new packages have unit tests (PHPUnit or phpt)
If you don't have some kind of tests in your PEPr proposal, it shouldn't go to call for votes / votes should be conditional.

They don't have to be complete, but at least a start.




For packages before being considered a stable release:


2) The unit tests should completely control the test environment

Mock out HTTP requests, database results, etc. This means really fast tests, and stops false positives.


3) Tests should cover both expected and common error cases

For example, if you are doing something over HTTP; inject a mock 404, 403 or 505 response; and expect certain exceptions to be raised.


4) Code should avoid the use of static methods which can't be mocked out.


// Bad:
function foo() {
$result = Bar::someComplexSlowOperation(); // I can't make a MockBar with canned results
return $result + 1;
}


5) Use of Agile Documentation for PHPUnit tests should be encouraged - FooTest::testShouldPerformSomeAction() instead of FooTest::testMethodName()

Hint: try phpunit --testdox YourTest some time :)


6) An AllTests.php should be provided, so that it can integrate into pear-wide test coverage.

See http://cvs.php.net/viewvc.cgi/pear/AllTests.php?view=markup and http://lauken.com/doconnor/ for a little bit more info.


7) Aim for near 100% coverage (as much coverage as is reasonably possible, at least coverage of every method)


8) All tests should run just fine from CVS/SVN.

A directory structure of:

Services_FooBar_BarFoo/Services/FooBar/BarFoo.php

instead of:

Services_FooBar_BarFoo/FooBar/BarFoo.php

Basically, when you

cd Services_FooBar_BarFoo
phpunit tests/AllTests.php

or

pear run-tests -r tests/


All of the require_once's should pick up CVS's first, not the installed version


9) If there is something which can't be mocked out - ie, a DB driver - and it needs configuration; the unit tests should skip, not fail.

phpt:

--SKIPIF--
<?php if (!file_exists('config-local.php')) { die("skip Can't run test without setting up a DB server! See config-dist.php"); } ?>


phpunit

FooTest::setup() {
if (!file_exists('config-local.php')) {
$this->markTestSkipped("Can't run test without setting up a DB server! See config-dist.php");
}
}



10) Unless there's a very compelling reason, your code should work just fine with PHPUnit's backupGlobals off.

IE: Avoid using $_POST, $_GET, $_REQUEST or any other superglobals unless you are specifically designed to mess with them - HTTP_Session for instance.


11) In general, make sure you can pass dependencies in through the constructor or setters.


// Bad
function __construct() {
$this->request = new HTTP_Request2();
}



// Good.
function __construct(HTTP_Request2 $request = null) {
if ($request === null) {
$request = new HTTP_Request2();
}

$this->setRequest($request);
}



12) In general, your constructor should avoid doing any Real Work - it takes values and puts them into correct places.


// Bad
function __construct(HTTP_Request2 $request) {
$this->result = $request->get('http://example.com/');
}




// Good
class Foo {
function __construct(HTTP_Request2 $request) {
$this->request = $request;
}

function fetch() {
$this->result = $this->request->get('http://example.com/');
}
}

$foo = new Foo(new HTTP_Request2());
$foo->fetch();

or

function factory() {
$foo = new Foo(new HTTP_Request2());
$foo->fetch();

return $foo;
}

$foo = factory();


13) In general, the new operator should not appear outside of the constructor or factory methods - otherwise this cannot be mocked out.


//Bad
function bar() {
$bar = new Bar();
return $bar->doSomething();
}



//Good
function bar(Bar $bar) {
return $bar->doSomething();
}

function bar() {
return $this->bar->doSomething();
}
» Dependencies » Links
» Timeline » Changelog
  • First Draft: 2008-12-29
  • Proposal: 2008-12-29