Hi...
Ambush Commander wrote:Hi Marcus, delighted to see you drop by!
No problem!
Except that you have a load of comments within the test case. If the comments became the test method names, then it would remain about the same size wouldn't it? I like the refactoring of assertDef(), but what does "Def" mean? Otherwise it all looks cool to me.
This is almost stylistic preference, but I'd write it like this...
Code: Select all
require_once 'HTMLPurifier/DefinitionCache.php';
require_once 'HTMLPurifier/ShortNames.php'; //
class DefinitionCacheTest extends UnitTestCase {
private $old_schema;
function setUp() {
$this->old_schema = ConfigSchema::instance();
ConfigSchema::instance(new ConfigSchema());
ConfigSchema::defineNamespace('Test', 'Test namespace');
ConfigSchema::define('Test', 'DefinitionRev', 1, 'int', 'Definition revision.');
}
function tearDown() {
ConfigSchema::instance($this->old_schema);
}
function config() {
generate_mock_once('Config');
$config = new ConfigMock();
$config->version = '1.0.0'; // hopefully no conflicts
$config->setReturnValue('get', 10, array('Test', 'DefinitionRev'));
$config->setReturnValue('getBatchSerial', 'hash', array('Test'));
return $config
}
function cache() {
return new HTMLPurifier_DefinitionCache('Test');
}
function test???() {
$this->assertFalse($this->cache()->isOld('1.0.0-hash-10', $this->config()));
$this->assertTrue($this->cache()->isOld('1.5.0-hash-1', $this->config()));
}
function test???() {
$this->assertTrue($this->cache()->isOld('0.9.0-hash-1', $this->config()));
}
function test???() {
$this->assertTrue($this->cache()->isOld('1.0.0-hash-1', $this->config()));
}
function test???() {
$this->assertTrue($this->cache()->isOld('1.0.0beta-hash-11', $this->config()));
}
function test???() {
$this->assertTrue($this->cache()->isOld('0.9.0-hash2-1', $this->config()));
$this->assertFalse($this->cache()->isOld('1.0.0-hash2-1', $this->config()));
$this->assertTrue($this->cache()->isOld('1.0.0beta-hash2-11', $this->config()));
}
}
I would have pulled the assertTrue() and assertFalse() lines into a more meaningful assertion name, but I don't know what they are doing. How would you describe them? What would you call the "???" here? I feel I am missing something vital. That said, i don't know a lot about HTML purification

.
Perhaps pull out chunks of set up into methods. Also there are often two or three tests rolled into one.
It's only fair that I put one of mine in the firing line...
Code: Select all
require_once(dirname(__FILE__) . '/accounts_test_case.php');
class TestManualAccountSetUp extends AccountsTestCase {
...
function testCanListPaymentsAgainstAnAccount() {
$this->nowJuly(1, 2007);
$account = $this->createFred();
$this->nowJuly(2, 2007);
$purchase1 = $this->accounts()->purchaseManually(
$account, 'One year subscription', 200, 'GBP');
$this->accounts()->payByCheque($purchase1, 200, 'GBP');
$this->nowJuly(3, 2007);
$purchase2 = $this->accounts()->purchaseManually(
$account, 'Top 10,000 keywords report', 50, 'USD');
$this->accounts()->payByCheque($purchase2, 50, 'USD');
$payments = $this->accounts()->findPaymentsAgainstAccount($account);
$this->assertEqual(
$this->justFields($payments, array('method', 'amount', 'currency')),
array(array('method' => 'Cheque', 'amount' => 50, 'currency' => 'USD'),
array('method' => 'Cheque', 'amount' => 200, 'currency' => 'GBP')));
}
...
}
...
There are about five test cases in the file, each with about a half dozen to a dozen integration tests like the above. The accounts model has about a dozen such test scripts.
The lines starting with...
...create the AccountsModel class with the correct DB driver. This and the justFields() method exist in the AccountsTestCase.
This is mostly a DB driven app. (the AccountsModel methods map to stored procs), and so testing parts of result sets is common. the justFields() methods extracts a slice for testing to avoid making the test dependent on the rest of the data. This is what I mean by avoiding over testing.
The entire DB schema is replaced on every test. This action lives in the AccountsTestCase too. Setting the dat is necessary because the product line can change in the future and so invalidate current tests.
This was the longest test method I could find in the entire Accounts module.
Well, I guess I can't complain then. I like immediate feedback, so I'm literally running test-cases after a one or two line change.
Me too. I'll run just the current test file for each edit (10 secs), the suite for the module often (1-3 mins) and all_tests.php (20 mins+) before a check in.
How does one know when a test is overspecified? On one hand, you want to make sure all the functionality is tested, on the other hand, you don't want the test to be fragile.
Right. You have to only test for the feature you are checking and nothing else.
For example, if you want to assert that the persons name appears in the title bar of a web page, then this is sufficient...
Code: Select all
$this->assertTitle(new PatternExpectation('/fred/i'));
...rather than...
Code: Select all
$this->assertTitle('My application - welcome Fred!');
In effect, stop worrying about the cascading error rushes!
Oh no, definitely worry about that

. Just don't make it the fault of the test suite. I feel that testing each feature in isolation trumps testing each class in isolation for this reason.
Say you are testing a HTML tag stripper...
Code: Select all
function assertStrippedAs($html, $text) {
$stripper = new HtmlSaxParser(new TextOnlyBuilder());
$this->assertEqual($parser->parse($html), $text);
}
function asserPreserved($text) {
$this->assertStrippedAs($text, $text);
}
function testEmptyDocumentPreserved() {
$this->assertPreserved('');
}
function testWhenNoTagsThenDocumentPreserved() {
$this->assertPreserved('Just text');
}
function testSingleLineCommentStripped() {
$this->assertStrippedAs('<!-- junk -->', '');
$this->assertStrippedAs('Hi<!-- junk -->', 'Hi');
$this->assertStrippedAs('<!-- junk -->There', 'There');
}
function testSingleLineCommentStrippedAndSpaceAddedIfNeeded() {
$this->assertStrippedAs('Hi<!-- junk -->There', 'Hi There');
$this->assertStrippedAs('Hi <!-- junk -->There', 'Hi There');
$this->assertStrippedAs('Hi<!-- junk --> There', 'Hi There');
$this->assertStrippedAs('Hi<!-- junk --> There', 'Hi There');
}
...
If something fundamental fails, then everything will fail. That's not something you would check in though, and it's obvious you just broke it. Subtler errors will be caused by people trying to change your code. They may still get quite a few failures, but fewer if they were just modifying an existing feature.
Whilst changing the mechanics, it's probably best to have a small unit test that tests just one tag and run only that. That small test can have the ithe internal mocking and the like. That way the mechanics test are factored out from the semantic ones.
yours, Marcus