Refactoring the tests again

Before more functionality is added there is some refactoring to do. We are going to do some timing tests and so the TimeTestCase class definitely needs its own file. Let's say tests/time_test_case.php...

<?php
if (! defined('SIMPLE_TEST')) {
    define('SIMPLE_TEST', 'simpletest/');
}
require_once(SIMPLE_TEST . 'unit_tester.php');

class TimeTestCase extends UnitTestCase {
    function TimeTestCase($test_name = '') {
        $this->UnitTestCase($test_name);
    }
    function assertSameTime($time1, $time2, $message = '') {
        if (! $message) {
            $message = "Time [$time1] should match time [$time2]";
        }
        $this->assertTrue(
                ($time1 == $time2) || ($time1 + 1 == $time2),
                $message);
    }
}
?>

Adding a timestamp to the Log

I don't know quite what the format of the log message should be for the test, so to check for a timestamp we could do the simplest possible thing, which is to look for a sequence of digits.

<?php
require_once('../classes/log.php');
require_once('../classes/clock.php');

class TestOfLogging extends TimeTestCase {
    function TestOfLogging() {
        $this->TimeTestCase('Log class test');
    }
    function setUp() {
        @unlink('../temp/test.log');
    }
    function tearDown() {
        @unlink('../temp/test.log');
    }
    function getFileLine($filename, $index) {
        $messages = file($filename);
        return $messages[$index];
    }
    function testCreatingNewFile() {
        ...
    }
    function testAppendingToFile() {
        ...
    }
    function testTimestamps() {
        $log = new Log('../temp/test.log');
        $log->message('Test line');
        $this->assertTrue(
                preg_match('/(\d+)/', $this->getFileLine('../temp/test.log', 0), $matches),
                'Found timestamp');
        $clock = new clock();
        $this->assertSameTime((integer)$matches[1], $clock->now(), 'Correct time');
    }
}
?>

We can get the tests to pass simply by adding a timestamp when writing out to the file. Yes, of course all of this is trivial and I would not normally test this fanatically, but it is going to illustrate a more general problem. The log.php file becomes...

<?php
require_once('../classes/clock.php');
    
class Log {
    var $_file_path;
        
    function Log($file_path) {
        $this->_file_path = $file_path;
    }
        
    function message($message) {
        $clock = new Clock();
        $file = fopen($this->_file_path, 'a');
        fwrite($file, "[" . $clock->now() . "] $message\n");
        fclose($file);
    }
}
?>

Our new test is full of problems, though. What if our time format changes to something else? Things are going to be a lot more complicated to test if this happens. It also means that any changes to the clock class time format will cause our logging tests to fail also. This means that our log tests are tangled up with the clock tests and extremely fragile. It lacks cohesion, which is the same as saying it is not tightly focused, testing facets of the clock as well as the log. Our problems are caused in part because the clock output is unpredictable when all we really want to test is that the logging message contains the output of Clock::now(). We don't really care about the contents of that method call.

Can we make that call predictable? We could if we could get the log to use a dummy version of the clock for the duration of the test. The dummy clock class would have to behave the same way as the Clock class except for the fixed output from the now() method. Hey, that would even free us from using the TimeTestCase class!

We could write such a class pretty easily although it is rather tedious work. We just create another clock class with same interface except that the now() method returns a value that we can change with some other setter method. That is quite a lot of work for a pretty minor test.

Except that it is really no work at all.

A mock clock

To reach instant testing clock nirvana we need only three extra lines of code...

require_once('simpletest/mock_objects.php');

Mock::generate('Clock');

$clock = &new MockClock($this);

Our test case is on the first steps of a radical clean up...

<?php
require_once('../classes/log.php');
require_once('../classes/clock.php');
Mock::generate('Clock');

class TestOfLogging extends UnitTestCase {
    function TestOfLogging() {
        $this->UnitTestCase('Log class test');
    }
    function setUp() {
        @unlink('../temp/test.log');
    }
    function tearDown() {
        @unlink('../temp/test.log');
    }
    function getFileLine($filename, $index) {
        $messages = file($filename);
        return $messages[$index];
    }
    function testCreatingNewFile() {
        ...
    }
    function testAppendingToFile() {
        ...
    }
    function testTimestamps() {
        $clock = &new MockClock($this);
        $clock->setReturnValue('now', 'Timestamp');
        $log = new Log('../temp/test.log');
        $log->message('Test line', &$clock);
        $this->assertWantedPattern(
                '/Timestamp/',
                $this->getFileLine('../temp/test.log', 0),
                'Found timestamp');
    }
}
?>

Next we create our log and send a message. We pass into the message() call the clock we would like to use. This means that we will have to add an optional parameter to the logging class to make testing possible...

class Log {
    var $_file_path;
    
    function Log($file_path) {
        $this->_file_path = $file_path;
    }
    
    function message($message, $clock = false) {
        if (!is_object($clock)) {
            $clock = new Clock();
        }
        $file = fopen($this->_file_path, 'a');
        fwrite($file, "[" . $clock->now() . "] $message\n");
        fclose($file);
    }
}

Does that extra parameter in the Log class bother you? We have changed the interface just to facilitate testing after all. Are not interfaces the most important thing? Have we sullied our class with test code?

Possibly, but consider this. Next chance you get, look at a circuit board, perhaps the motherboard of the computer you are looking at right now. On most boards you will find the odd empty hole, or solder joint with nothing attached or perhaps a pin or socket that has no obvious function. Chances are that some of these are for expansion and variations, but most of the remainder will be for testing.

Think about that. The factories making the boards many times over wasting material on parts that do not add to the final function. If hardware engineers can make this sacrifice of elegance I am sure we can too. Our sacrifice wastes no materials after all.

Still bother you? Actually it bothers me too, but not so much here. The number one priority is code that works, not prizes for minimalism. If it really bothers you, then move the creation of the clock into another protected factory method. Then subclass the clock for testing and override the factory method with one that returns the mock. Your tests are clumsier, but your interface is intact.

Again I leave the decision to you.