SitePoint Sponsor

User Tag List

Results 1 to 9 of 9
  1. #1
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Unit Test Correctness

    Hi there,
    I'm currently adding unit tests (simpletest 1.0) to an existing application. The domain model uses active records and I have a class called DbAccess that is passed around and provides data access methods along with a db handler. But when I mock the DBAccess class the testmethods tend to get are rather complicated (at least in my opinion) so I'm not sure I can trust them. E.g. when I do an expectOnce() and forget the tally() I don't get an error when the method is called twice. I will give you a small sample of the classes and the test. I'll keep it as short as possible to illustrate my questions.

    DbAccess, this one will be mocked later, but only partially as I use type hinting. (I'm not ready to abandon type hinting right now.)
    PHP Code:
    <?php
    class DbAccess {
        protected 
    $dbHandler;
        public function 
    __construct() { 
            
    $this->dbHandler mysql_connect('localhost''user''');
        }
        public function 
    query($sql) {
            return 
    mysql_query($sql$this->dbHandler);
        }
        public function 
    getRow($sql) {
            
    $result $this->query($sql);
            return 
    mysql_fetch_assoc($result$this->dbHandler);
        }
        
    //[...]
    }
    ?>
    A part of the simplified domain class Server.
    PHP Code:
    <?php
    class Server {
        public function 
    __construct($idDbAccess $dbAccess) {
            if (!
    Server::isValid($id$dbAccess)) {
                throw new 
    Exception('Server with ID '$id .' is not valid.');
            }
            
    $this->dbAccess $dbAccess;
            
            
    $sql 'SELECT * FROM common.server WHERE serverId = '$this->id .' LIMIT 1';
            
    $this->raw $this->dbAccess->getRow($sql);
            
    // setting some more instance properties ...
        
    }
        public static function 
    isValid($idDbAccess $dbAccess) {
            
    $sql 'SELECT COUNT(serverId) AS c FROM common.server WHERE serverId = '$id;
            
    $row $dbAccess->getRow($sql);
            return (
    $row['c'] == '1') ? true false;
        }
        
    // some more methods ...
    }
    ?>
    Now my testing code. Not nice.
    PHP Code:
    <?php
    Mock
    ::generatePartial('DbAccess''DbAccessMock', array('getRow''query'));

    class 
    ServerTest extends UnitTestCase {
        protected 
    $serverId 1;
        protected 
    $dbAccessMock;

        public function 
    setUp() {
            
    $this->dbAccessMock = new DbAccessMock($this);
        }
        public function 
    testCreation() {
            
    $this->dbAccessMock->expectArgumentsAt(0'getRow', array('SELECT COUNT(serverId) AS c FROM common.server WHERE serverId = 1'));
            
    $this->dbAccessMock->expectArgumentsAt(1'getRow', array('SELECT * FROM common.server WHERE serverId = '$this->serverId .' LIMIT 1'));

            
    $this->dbAccessMock->setReturnValueAt(0'getRow', array('c'=>'1'));
            
    $this->dbAccessMock->setReturnValueAt(1'getRow', array('serverId'=>$this->serverId'serverHostname'=>$this->serverHostname));
            
            
    $server = new Server($this->serverId$this->dbAccessMock);
        }
        public function 
    testFailedCreation() {
            
    $this->dbAccessMock->setReturnValueAt(0'getRow', array('c'=>'0'));
            try { 
                  
    $server = new Server(1$this->dbAccessMock); 
                  
    $this->fail(); } 
               catch (
    Exception $e) { $this->pass(); }
        }
    }
    ?>
    Is this the correct way to test? I'm a little shocked that the testcode will often exceed the method in size and complexity. I think it's very likely my testcode contains errors. Is there a better way to support type hinting? Is there a better way to test exceptions?

    How do you ensure your tests are correct? I learned that refactoring can only be done safely when backed up by reliable tests, but how can I ever be sure or at least confident? (Sometimes I think unit tests only shift the problem of correctness away from the application code to the test code. Though writing correct test code may be easier.)

    How do I chose correct test data? I want to test with "correct" and "false" arguments as well as with boundary values. If I don't get that right, my test is worthless. So another question for honest test infected people: How often do you find your tests are not sufficient or even incorrect?

    Thanks for any input,
    Christoph

  2. #2
    eschew sesquipedalians silver trophy sweatje's Avatar
    Join Date
    Jun 2003
    Location
    Iowa, USA
    Posts
    3,749
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Marcus is prepairing to release another tarball soon. The current cvs version no longer requires either the ($this) parameter in the Mock constructor, or the $mock->tally() call. Simplifies code quite a bit.

    Since you are using PHP5, you might want to do something like:

    PHP Code:
    interface iDb {
    function 
    query($sql);
    function 
    getRow($dql);
    }

    class 
    DbAccess implements iDb {
      
    //...
    }

    class 
    Server {
        public function 
    __construct($idiDb $dbAccess) { 
            
    // ...
        
    }

    Then just:
    PHP Code:
    Mock::generate('iDb');
    class 
    ServerTest extends UnitTestCase {

        public function 
    testCreation() {
            
    $db = new MockiDb;
            
    $db->expectArgumentsAt(0'getRow', array('SELECT COUNT(serverId) AS c FROM common.server WHERE serverId = 1'));
            
    $db->expectArgumentsAt(1'getRow', array('SELECT * FROM common.server WHERE serverId = '$this->serverId .' LIMIT 1'));

            
    $db->setReturnValueAt(0'getRow', array('c'=>'1'));
            
    $db->setReturnValueAt(1'getRow', array('serverId'=>$this->serverId'serverHostname'=>$this->serverHostname));
            
            
    $server = new Server($this->serverId$db);
        } 
    //...

    Jason Sweat ZCE - jsweat_php@yahoo.com
    Book: PHP Patterns
    Good Stuff: SimpleTest PHPUnit FireFox ADOdb YUI
    Detestable (adjective): software that isn't testable.

  3. #3
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Hmm, but then I would have application code just for the test. Doesn't that smell? But somewhere I read Marcus is aware of this type hinting issue, so I'm willing to live with that until, say 2.0. Or do partial mocks have any disadvantages apart from a little more code at generation?

    Does the rest of my testcode make sense? Especially all this expectAt() and setReturn()--I find it so very inflexible ... but I guess that's mostly the fault of my clumsy db accessor class.

  4. #4
    SitePoint Wizard silver trophy kyberfabrikken's Avatar
    Join Date
    Jun 2004
    Location
    Copenhagen, Denmark
    Posts
    6,157
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by FrlB
    Hmm, but then I would have application code just for the test. Doesn't that smell?
    I'd say the contrary. Typehinting enforces a type. In PHP5 you can denote type in two ways: Either through a class or through an interface. An interface is a pure type, in that it only specifies a type - not an implementation. A class on the other hand, is type + implementation. If you typehint to a class, you form what is known as a concrete class dependency. The tests are teaching you to avoid this and separate type from implementation.

  5. #5
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by kyberfabrikken
    If you typehint to a class, you form what is known as a concrete class dependency.
    Point taken. Thanks for the clarification.
    Quote Originally Posted by kyberfabrikken
    The tests are teaching you to avoid this and separate type from implementation.
    Oah, these unit tests are teaching me a lot. Don't get me started ... :)

  6. #6
    simple tester McGruff's Avatar
    Join Date
    Sep 2003
    Location
    Glasgow
    Posts
    1,690
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I'd suggest a simple rule about hints: if the test passes with hints removed, leave them out. With TDD the aim is to do the simplest thing.

    If I'm testing data access classes I use a real database and sample data set up and torn down for the tests. This should pass but will fail and I bet you can't see why at first:
    PHP Code:
          $this->dbAccessMock->expectArgumentsAt(0
              
    'getRow'
              array(
    'SELECT COUNT(serverId) AS c FROM  common.server WHERE serverId = 1')); 
    That's the kind of thing which can leave me stumped, banging my head on the desk for half an hour if it's late and I'm tired. Sql is too "meaning-rich" simply to assert as a string. There are too many variations which are semantically identical: spaces, case, predicate order etc etc. There's also the question if it makes sense in the context of the target database (eg correct table names) or if the string is even valid sql. Easiest way to check is to use a real database.

  7. #7
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by McGruff
    Sql is too "meaning-rich" simply to assert as a string. There are too many variations which are semantically identical: spaces, case, predicate order etc etc. There's also the question if it makes sense in the context of the target database (eg correct table names) or if the string is even valid sql.
    Yeah, I thought about parametrizing (?) the SQL so I could do something like select($table, $columns, $joins, $where, $order, $limit) so it would be easier to check, but that seemed far too much effort for my modest needs. Also checking with regular expressions and allowing order changes and messed up whitespace. But that makes it even more complicated--and I'm not exactly what one would call regex wizard.

    Quote Originally Posted by McGruff
    Easiest way to check is to use a real database.
    Hmm, after setting it up testing should be very straight forward and I would have to bother a little less about errors within my test code. I will definitely think about it. Any serious disadvantages to take into account?

  8. #8
    ********* Victim lastcraft's Avatar
    Join Date
    Apr 2003
    Location
    London
    Posts
    2,423
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    Hi...

    Quote Originally Posted by FrlB
    Any serious disadvantages to take into account?
    Yes, but they are well worth the cost. The extra work is having to switch the database to a localhost/test version during testing. You should do this anyway. Immunises yourself against changing server architectures. The other hassle will be setting up and tearing down the schema for each test. This will slow the tests down a little.

    On the other hand asserting SQL strings is a mugs game. Who cares if your SQL contains an equals sign or a like clause? Everytime you refactor you will break the tests, yet the tests are supposed to be an aid to refactoring. The tests should be kept green while you refactor to check that what you are doing is still correct. I would say that failing this requirement is a test smell.

    Not only that, but tests are difficult to name when you do string assertions. Do you want testSearchQueryForBeerDrinkersContainsLikeClause() or testCanFindSetOfAllPeopleWhoDrinkBeer()? the second will make a lot more sense down the line. Tests should read like a specification where possible.

    Like McGruff I would make the round trip to the DB when testing the ActiveRecords themselves. You can then mock these classes in the higher level tests.

    Your tests will get shorter too .

    yours, Marcus
    Marcus Baker
    Testing: SimpleTest, Cgreen, Fakemail
    Other: Phemto dependency injector
    Books: PHP in Action, 97 things

  9. #9
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by kyberfabrikken
    If you typehint to a class, you form what is known as a concrete class dependency. The tests are teaching you to avoid this and separate type from implementation.
    Ok, I've added the iDb interface and implement it in DbAccess. So do I get this right: If I choose type hinting I need interfaces to implement from to avoid binding my methods to an implementation (which is less stable than an interface). If I don't use type hinting at all there is no need to use interfaces other that being a "contract".
    Quote Originally Posted by lastcraft
    The extra work is having to switch the database to a localhost/test version during testing. You should do this anyway. Immunises yourself against changing server architectures. The other hassle will be setting up and tearing down the schema for each test. This will slow the tests down a little.
    I'm thinking about MySQL's temporary tables, any votes against this? Speed should not be an issue with less than 50 classes to test, slowest will always be the webtests anyway ...

    As always: Your suggestions have been most valuable. Thanks.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •