How extensively should one test?

[ReeD]

I am trying to learn using Simpletest, so I wrote tests for the DB connection class and wrote the class itself. Here's how it looks like (test names should be self-explanatory):

<?php



define('SIMPLE_TEST', '../simpletest/');



require_once(SIMPLE_TEST . 'unit_tester.php');

require_once(SIMPLE_TEST . 'reporter.php');



require_once('../classes/class.MySQLDatabase.php');



define('db_host', 'localhost');

define('db_name', 'test');

define('db_user', 'root');

define('db_pass', '');



class TestOfMySQLDatabase extends UnitTestCase

{

  var $connection;



  function TestOfMySQLDatabase()

  {

    $this->UnitTestCase('Test of MySQL Database');

  }



  function setUp()

  {

    $this->connection = mysql_connect(db_host, db_user, db_pass, true);

    $this->query("CREATE DATABASE " . db_name);

    mysql_select_db(db_name, $this->connection);

    $this->query("CREATE TABLE test_table (test_field TEXT)");

    $this->query("INSERT INTO test_table VALUES ('test_value')");

    $this->db = &new MySQLDatabase(db_host, db_name, db_user, db_pass);

  }



  function tearDown()

  {

    mysql_query("DROP DATABASE " . db_name, $this->connection);

    mysql_close($this->connection);

  }



  function testSuccessfullConnection()

  {

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

  }



  function testGoodQuery()

  {

    $result = $this->db->query("SELECT test_field FROM test_table");

    $this->assertTrue(is_object($result));

    $this->assertIdentical($result->fetch(), array('test_field' => 'test_value'));

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

  }



  function testBadQuery()

  {

    $result = $this->db->query("foo");

    $this->assertIdentical($this->db->error(), true);

    $this->assertIdentical($this->db->getError(), 'SQLError');

  }



  function testGoodManipulate()

  {

    $this->db->manipulate("INSERT INTO test_table VALUES ('another_value')");

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

    $result = $this->db->query("SELECT test_field FROM test_table WHERE test_field='another_value'");

    $this->assertTrue(is_object($result));

    $this->assertIdentical($result->fetch(), array('test_field' => 'another_value'));

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

  }



  function testBadManipulate()

  {

    $this->db->manipulate("foo");

    $this->assertIdentical($this->db->error(), true);

    $this->assertIdentical($this->db->getError(), 'SQLError');

  }



  function testErrorsAreClearedAfterEachQuery()

  {

    $this->db->manipulate("foo");

    $this->assertIdentical($this->db->error(), true);

    $this->assertIdentical($this->db->getError(), 'SQLError');

    $result = $this->db->query("SELECT test_field FROM test_table");

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

    $result = $this->db->query("foo");

    $this->assertIdentical($this->db->error(), true);

    $this->assertIdentical($this->db->getError(), 'SQLError');

    $this->db->manipulate("INSERT INTO test_table VALUES ('booh')");

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

  }



  function testSuccessfullConnectionDisconnect()

  {

    $this->db->disconnect();

    $this->assertIdentical($this->db->error(), false);

    $this->assertIdentical($this->db->getError(), '');

  }



  //Connection problem tests (separate db object is created)

  function testSuccessfullConnectionWrongDB()

  {

    $db = &new MySQLDatabase(db_host, 'booh', db_user, db_pass);

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'NoDB');

    $result = $db->query("SELECT test_field FROM test_table");

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'NoDB');

    $db->manipulate("INSERT INTO test_table VALUES ('booh')");

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'NoDB');

  }



  function testUnsuccessfullConnection()

  {

    $db = &new MySQLDatabase('booh', db_name, db_user, db_pass);

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'UnableToConnect');

    $result = $db->query("SELECT test_field FROM test_table");

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'UnableToConnect');

    $db->manipulate("INSERT INTO test_table VALUES ('booh')");

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'UnableToConnect');

  }



  function testSuccessfullConnectionWrongDBDisconnect()

  {

    $db = &new MySQLDatabase(db_host, 'booh', db_user, db_pass);

    $db->disconnect();

    $this->assertIdentical($db->error(), false);

    $this->assertIdentical($db->getError(), '');

  }



  function testUnsuccessfullConnectionDisconnect()

  {

    $db = &new MySQLDatabase('booh', db_name, db_user, db_pass);

    $db->disconnect();

    $this->assertIdentical($db->error(), true);

    $this->assertIdentical($db->getError(), 'UnableToDisconnect');

  }



  function query($sql)

  {

    mysql_query($sql, $this->connection);

  }

}

    

$test = &new TestOfMySQLDatabase();

$test->run(new HtmlReporter());



?>

And this is the class:

<?php





require_once('class.MySQLResultIterator.php');



class MySQLDatabase

{

  var $connection;

  var $select;

  var $error;



  function MySQLDatabase($host, $name, $username, $password)

  {

    $this->connection = @mysql_connect($host, $username, $password, true);

    if ($this->connection)

    {

      $this->select = mysql_select_db($name, $this->connection);

      if ($this->select)

      {

        $this->error = '';

        return;      

      }

      $this->error = 'NoDB';

      return;

    }

    $this->error = 'UnableToConnect';

  }



  function query($sql)

  {

    if ($this->select)

    {

      $result = mysql_query($sql, $this->connection);

      if (!$result)

      {

        $this->error = 'SQLError';

        return;

      }

      $this->error = '';

      return $iterator = &new MySQLResultIterator($result);

    }

  }



  function manipulate($sql)

  {

    if ($this->select)

    {

      $result = mysql_query($sql, $this->connection);

      if (!$result)

      {

        $this->error = 'SQLError';

        return;

      }

      $this->error = '';

    }

  }



  function disconnect()

  {

    if ($this->connection)

    {

      $this->error = '';

      mysql_close($this->connection);

      return;

    }

    $this->error = 'UnableToDisconnect';

  }



  function getError()

  {

    return $this->error;

  }



  function error()

  {

    return (bool)$this->error;

  }

}



?>

When I did all this, now I wonder: aren't I doing too much testing? How extensively should one test classes? You can easily notice, that tests pretty much cover all possible variations of method usage. Is that how it should be tested?

In my example there's quite a lot of code in the tests... Do you think all the tests are proper or are some of them unneccessary at all? Now since I pretty much finished it, any comments on this would be greatly appreciated.

[lastcraft]

Hi...

Test until you are confident.

I would say that you haven't over tested, but you have over asserted . When a test fails, you want to quickly go to the point of failure to see what went wrong. The ...assert(is_object()) call just add clutter I think, also the repeated checking for no error. You can always debug this stuff once you get the failure.

One way to think about this is to just say in words what you want done. That becomes the test title of course, and the expected result is yourassertion. That way you usually only write one or two assertions each time.

Minor quibble. I would test for pattern sin the error messages, rather than exact lettering. The tests should be an aid to refactoring, but if you have to edit them every time you change the wording of a message they will really start to irritate.

I would expect fairly excessive testing of a connection object. Lot's of things can go wrong. On average I get about as much test code as real code.

yours, Marcus

[ReeD]

Originally Posted by lastcraft

The ...assert(is_object()) call just add clutter I think

You're absolutely right here, it's not needed at all. It was originally just a temporary assertion until I did not have the MySQLResultIterator fully coded, and I forgot to remove it later.

Originally Posted by lastcraft

also the repeated checking for no error. You can always debug this stuff once you get the failure.

Now i'm not quite sure about this... Say, when I define a test 'I should run a valid query successfully, and I should get no errors after that', I literally mean that when using the finished object, after running a valid query it should result in no error (thus I use assertions). Maybe you could explain this a bit more or just point exactly what could be easily removed in the actual tests without missing anything to test.

[lastcraft]

Hi...

Originally Posted by ReeD

'I should run a valid query successfully, and I should get no errors after that'

You can express this more directly...

class TestOfDatabseConnection extends UnitTestCase {

    ...

    function testCanReadBackSingleRow() {

        $this->db->manipulate("INSERT INTO test_table VALUES ('another_value')");

        $result = $this->db->query(

                "SELECT test_field FROM test_table " .

                "WHERE test_field='another_value'");

        $this->assertIdentical(

                $result->fetch(),

                array('test_field' => 'another_value'));

    }



    function testValidQueryLeavesNoError() {

        $result = $this->db->query(

                "SELECT test_field FROM test_table " .

                "WHERE test_field='another_value'");

        $this->assertFalse($this->db->isError());

    }

    ...

} 

By placing the error test once in it's own test, the tests get a little more focused and I become confident enough that I don't bother repeating it. As I say, this is a minor quibble. Maybe it's just my way of doing things.

Anyway, you'll find on the first refactor those tests that help and those that hinder.

BTW one thing I have found useful about the connection type objects is to add transaction support too and fold the close method into the disconnection. It becomes a MysqlTransaction object then, which I personally find easier to work with. You just go...

$transaction = &new MysqlTransaction(); // begin

$transaction->execute(...);

$transaction->commit();   // commit 

I find I get less state to keep track of that way.

yours, Marcus

[McGruff]

I think I might disagree with Marcus about too many error assertions. You've got query methods which can be called repeatedly during the lifetime of the object. Each call must reset the error state depending on whether the query failed or not. If you were using mysql_error() rather than setting your own error strings (I'd probably prefer the former since they're more informative) you'd automatically get the error relating to the last query. Thus you could test once to make sure that mysql_error is in fact wired up to the getError method and then forget all about it in the other tests.

However, since you're setting your own error values, you can't rely on mysql_error automatically resetting itself with each new query. It would be possible to write an implementation which persisted error states between queries and therefore that should be one of the constraints. Checking the getError value in each test might not be such a bad idea but if you can think of a single test to deal with remembered errors that would be better. Better still if you can avoid setting error properties in the first place - there's no state to be remembered.

There's a cheat I use a lot for errors:

 class Foo
 {
     var $bar_error = 'aargh!';
     // etc
 }
                          
 class TestOfFoo
 {
     function testFooError()   {
         // create the error state and then...
         $this->assertError($foo->bar_error);
     }
 } 

Although $bar_error should be a private property, in php4 that's only a gentleman's agreement thus allowing you to assert the error string without creating an accessor which you aren't going to need outside the test. If you edit the error property the test stays the same. You can do something similar with genuinely private properties in php5: see Derick Rethans blog (scroll down for Private Properties Exposed).

Do you really need the error() method? In the calling script, you could just check if the getError return value is an empty string.

I'd be tempted to take the database selection out of there in order to simplify things.

Finallty, I think you're definitely on the right track overall. Unit tests should be fine-grained. If something fails, you want the tests to tell you what and why. Test every last detail which is of interest. Ignore everything which isn't so you don't over-constrain possible solutions.

In saying that, I wrote a 130-line class tonight with a 400-line unit test so I seem to be well off the 1:1 test/code ratio.

[ReeD]

Thanks, comments of both of you are really helpfull.

Marcus:

class TestOfDatabseConnection extends UnitTestCase {

    function testCanReadBackSingleRow() {

        $this->db->manipulate("INSERT INTO test_table VALUES ('another_value')");

        $result = $this->db->query(

                "SELECT test_field FROM test_table " .

                "WHERE test_field='another_value'");

        $this->assertIdentical(

                $result->fetch(),

                array('test_field' => 'another_value'));

    }

} 

This test covers valid row manipulation. But when you look at it, how do you know if you are going to get no error after running $db->manipulate()? You would still need to cover this somehow.

I do see a lot of error assertions in my tests, but I can't see a way to reduce the count, however, I'd really like to have it reduced.

McGruff:

Do you really need the error() method? In the calling script, you could just check if the getError return value is an empty string.

I just like it this way

if ($db->error())

{

  $_SESSION['msg'] = $lng[$db->getError()];

  //Terminate

} 

rather than

$error = $db->getError();

if (!empty($error))

{

  $_SESSION['msg'] = $lng[$error];

  //Terminate

} 

I'd be tempted to take the database selection out of there in order to simplify things.

That would make me write 'SELECT * FROM db_name.db_table ...' which I would rather avoid. It's just simplier.

[lastcraft]

Hi...

Originally Posted by ReeD

This test covers valid row manipulation. But when you look at it, how do you know if you are going to get no error after running $db->manipulate()? You would still need to cover this somehow.

There is something called "the Samurai principle". It goes "Return victorious or don't return at all". Basically it worked, or it through an exception/disconnect. You should never be in a state that both has an error, and is sending out results. How can you trust the results?

If you get any kind of error, don't return the result. Then test for the result only when looking at a happy path.

yours, Marcus

[ReeD]

Originally Posted by lastcraft

Basically it worked, or it through an exception/disconnect.

I assume you meant "Basically it worked or it didn't".

Originally Posted by lastcraft

You should never be in a state that both has an error, and is sending out results.

In the case of testing manipulate() with valid query, there are no results sent.

Here's how I look at it. When writing tests, you define certain behaviour you want the class to have. Let's take the same valid manipulate() call. Before writing the test, I formulate the behaviour as this: when running a valid query with manipulate(), it should do what's in the SQL sentence AND, since it has done it correctly, it should return no error ($db->error() === false). The first part of the requirement (before 'AND') your test fullfills - you run valid query and check (via query()) if it actually performed the SQL instruction. But the test simply doesn't cover an important thing - there should be no error after running the query. When writing the test, I imagine how I will be using the class: I would run manipulate() and immediately after that I would check if any error occured. If the query was valid, no error should be there. But the test simply does not cover the error part. Thus 'I do not know' if after running valid manipulate() query, I would not get any error, hence I would write the no-error assertion.

This example you gave me below was in response to my comment 'I should run a valid query successfully, and I should get no errors after that', but I think it's a bit incorrect since although it shows two tests for query() and manipulate(), error check is present in one only and not another.

class TestOfDatabseConnection extends UnitTestCase {

    ...

    function testCanReadBackSingleRow() {

        $this->db->manipulate("INSERT INTO test_table VALUES ('another_value')");

        $result = $this->db->query(

                "SELECT test_field FROM test_table " .

                "WHERE test_field='another_value'");

        $this->assertIdentical(

                $result->fetch(),

                array('test_field' => 'another_value'));

    }



    function testValidQueryLeavesNoError() {

        $result = $this->db->query(

                "SELECT test_field FROM test_table " .

                "WHERE test_field='another_value'");

        $this->assertFalse($this->db->isError());

    }

    ...

} 

I hope you understand what I mean, it just seems to me that no-error assertions are required in your example 'testCanReadBackSingleRow()'.

[lastcraft]

Hi...

Firstly, let me say that this whole niche is pretty new. I could be way off and you may be utterly correct. However...

Originally Posted by ReeD

In the case of testing manipulate() with valid query, there are no results sent.

No, but there is an effect on the system. Basically, once inserted, you should be able to read it back with a select. That's the core test of any storage system. Checking that errors are reported correctly can be in a separate bunch of tests. Checking the absence of an error, at least more than once, is duplication to me.

Also, do you really trust that write just because no error was reported? That is placing a lot of trust in the code. As you have to make the stronger (read back) test anyway, why not do just that one.

This is at teh test level of course. You may, at the code level, want to return false if the row was not written. For example, a duplicate key. Again, I would have a separate test for this for this case. I suspect though, that you will never check the return value in the code either. You might be designing ahead with such a return value.

Tests are also documentation for the system. Having clear tests is nice.

yours, Marcus

[ReeD]

All right, so if I understand it correctly, you suggest separating main functionality tests from error reporting tests, right?

Checking the absence of an error, at least more than once, is duplication to me.

I assume you mean this for the same method. You do agree that it would be best to test error reporting extensively for all methods (combinations of them)?

[lastcraft]

Hi.

Test until you are confident. Remember, unit tests are white box tests. This means you can use the code to guide you as to what will break.

yours, Marcus

[ReeD]

Yeah, I do want to test untill I am confident, just want to not have any redundant tests and avoid some possible mistakes... Was expecting a bit more 'yes/no' answer to my last post.

[OfficeOfTheLaw]

How extensively should one test?

Every possible thing. Every conceivable thing. Remember, for every section of untested code, you will likely have as many bugs as you do users.