12
\$\begingroup\$

This shows a test case for an old caching library that I use for a project. It features simple save/load/delete functions (sadly static calls) but what I want to focus on is the test code written for this class.

In my opinion the unit tests for a class should show how all functions in the class work and what to expect from that class.

Recently we got this great change to show off code, so I'd like to ask you if you can read that test code, understand what the class might do and how you would improve upon it.

<?php

class DatenCacheTest extends PHPUnit_Framework_TestCase {

    function testNothingFound() {
        $this->assertSame(false, DatenCache::load("testNotHereCache"));
    }

    function testSaveLoadSimple() {
        $sDatenSimple = "testStringSaveLoadSimple";
        $sDatenSimpleParam = "testStringParam";
        $sDatenSimpleParam2 = "testStringParam2";
        $sDatenSimpleParam3 = "";
        $sDatenSimpleParam4 = 0;

        $this->assertSame(true, DatenCache::save("testCacheSimple", false, $sDatenSimple));
        $this->assertSame($sDatenSimple, DatenCache::load("testCacheSimple", false));

        $this->assertSame(true, DatenCache::save("testCacheLinearParam", "string", $sDatenSimpleParam));
        $this->assertSame($sDatenSimpleParam, DatenCache::load("testCacheLinearParam", "string"));

        $this->assertSame(true, DatenCache::save("testCacheLinearParam", 5, $sDatenSimpleParam2));
        $this->assertSame($sDatenSimpleParam2, DatenCache::load("testCacheLinearParam", 5));

        $this->assertSame(true, DatenCache::save("testCacheBoundStringParam", false, $sDatenSimpleParam3)); 
        $this->assertSame($sDatenSimpleParam3, DatenCache::load("testCacheBoundStringParam", false));

        $this->assertSame(true, DatenCache::save("testCacheBoundIntParam", false, $sDatenSimpleParam4));
        $this->assertSame($sDatenSimpleParam4, DatenCache::load("testCacheBoundIntParam", false));

        $oObj = new stdClass();
        $oObj->bob = 2;
        $this->assertSame(true,   DatenCache::save("testCacheBoundObjParam", $oObj, "zwei"));
        $this->assertSame("zwei", DatenCache::load("testCacheBoundObjParam", $oObj));
   }

    function testSaveLoadArray() {
        $aDaten = array("da" => "ten", array("ten" => "da"), "Striche" => "' \";s:5:", "h''uh" => "mep", "\\mep\\" => "^^", "Zeilenumbruch" => "\n", "Zeug" => "::}}{:", "'" => '"', '"' => "'");

        $this->assertSame(true, DatenCache::save("testCacheArray", false, $aDaten));
        $this->assertSame($aDaten, DatenCache::load("testCacheArray", false));
    }

    function testSaveLoadExpired() {
        $this->assertSame(true,  DatenCache::save("testCacheExpired", false, "testStringLoadExpired", "-1 seconds"));
        $this->assertSame(false, DatenCache::load("testCacheExpired", false));
    }

    function testSaveLoadObject() {
        $oObj = new stdClass();
        $oObj->bob = 2;
        $this->assertSame(true, DatenCache::save("testCacheBoundIntParam", false, $oObj));

        $oNewObj = DatenCache::load("testCacheBoundIntParam", false);
        $this->assertEquals($oObj, $oNewObj);
        $this->assertSame($oObj->bob, $oNewObj->bob);

    }

    function testSaveLoadComplexParam() {
        $aKonfigEins  = array(DatenCache::ANLAGEN_ID() => 1, DatenCache::SESSION_ID() => "id1");
        $aKonfigZwei  = array(DatenCache::ANLAGEN_ID() => 1, DatenCache::SESSION_ID() => "id2");
        $aKonfigDrei  = array(DatenCache::ANLAGEN_ID() => 1, DatenCache::SESSION_ID() => "id3");
        $aKonfigVier  = array(DatenCache::ANLAGEN_ID() => 1);
        $aKonfigFuenf = array(DatenCache::SESSION_ID() => "id3");
        $aKonfigSechs = array(DatenCache::ANLAGEN_ID() => 1, DatenCache::SESSION_ID() => "id3", DatenCache::PERSON_ID() => 1);

        $this->assertSame(true, DatenCache::save("testCacheComplexParam", $aKonfigEins, "v1"));
        $this->assertSame("v1", DatenCache::load("testCacheComplexParam", $aKonfigEins));
        $this->assertSame(true, DatenCache::save("testCacheComplexParam", $aKonfigZwei, "v2"));
        $this->assertSame("v2", DatenCache::load("testCacheComplexParam", $aKonfigZwei));

        $this->assertSame(false, DatenCache::load("testCacheComplexParam", $aKonfigDrei));
        $this->assertSame(false, DatenCache::load("testCacheComplexParam", $aKonfigVier));
        $this->assertSame(false, DatenCache::load("testCacheComplexParam", $aKonfigFuenf));
        $this->assertSame(false, DatenCache::load("testCacheComplexParam", $aKonfigSechs));

    }

    function testInvalidate() {
        $this->assertSame(true, DatenCache::save("testCacheSimple", false, "testStringInvalidate"));
        DatenCache::invalidate("testCacheSimple");
        $this->assertSame(false, DatenCache::load("testCacheSimple", false));
    }

    function testInvalidateWithParams() {
        $sTestStringEins = "testStringInvalidateWithParams";
        $sTestStringZwei = "testStringInvalidateWithParamsZwei";
        $aKonfigEins = array(DatenCache::ANLAGEN_ID() => 1, "Pony" => false);
        $aKonfigZwei = array(DatenCache::ANLAGEN_ID() => 1, "Pony" => true);

        $this->assertSame(true, DatenCache::save("testCacheInvalidateWithParams", $aKonfigEins, $sTestStringEins));
        $this->assertSame(true, DatenCache::save("testCacheInvalidateWithParams", $aKonfigZwei, $sTestStringEins));
        DatenCache::invalidate("testCacheInvalidateWithParams", $aKonfigEins);
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigEins));
        $this->assertSame($sTestStringEins, DatenCache::load("testCacheInvalidateWithParams", $aKonfigZwei));


        $aKonfigDrei = array(DatenCache::ANLAGEN_ID() => 1, DatenCache::BENUTZER_ID() => 123, "bob" => "please");
        $aKonfigVier = array(DatenCache::ANLAGEN_ID() => 2, "Pony" => true, "bob" => "please");
        $aPartEins   = array(DatenCache::ANLAGEN_ID() => 1);
        $aPartZwei   = array(DatenCache::BENUTZER_ID() => 123);
        $aPartDrei   = array("bob"  => "please");

        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigDrei, $sTestStringZwei."1"));
        DatenCache::invalidate("testCacheInvalidateWithParams", $aPartEins);
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigDrei));

        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigDrei, $sTestStringZwei."2"));      
        DatenCache::invalidate("testCacheInvalidateWithParams", $aPartZwei);
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigDrei));

        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigDrei, $sTestStringZwei."3"));
        DatenCache::invalidate("testCacheInvalidateWithParams", $aPartDrei);
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigDrei));



        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigEins, $sTestStringEins));
        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigZwei, $sTestStringEins));
        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigDrei, $sTestStringEins));
        DatenCache::invalidate("testCacheInvalidateWithParams", array(DatenCache::ANLAGEN_ID() => 1));
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigEins));
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigZwei));
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigDrei));

        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigEins, $sTestStringEins));
        $this->assertSame(true,  DatenCache::save("testCacheInvalidateWithParams", $aKonfigVier, $sTestStringEins));
        DatenCache::invalidate("testCacheInvalidateWithParams", array(DatenCache::ANLAGEN_ID() => 1));
        $this->assertSame(false, DatenCache::load("testCacheInvalidateWithParams", $aKonfigEins));
        $this->assertSame($sTestStringEins, DatenCache::load("testCacheInvalidateWithParams", $aKonfigVier));

    }

    function testCleanup() {
        $this->assertSame(true, DatenCache::save("testCacheSimple",       false, "testString", "-5 seconds"));
        $this->assertSame(true, DatenCache::save("testCacheSimpleBleibt", false, "testString", "+30 seconds"));
        DatenCache::cleanup();
        $this->assertSame("testString", DatenCache::load("testCacheSimpleBleibt", false));
        $this->assertSame(false, DatenCache::load("testCacheSimple", false));
        // Direkt auf der DB Prüfen ob der Datensatz wiklich gelöscht wurde und nicht nur ausgelaufen ist
        $this->assertSame(
            array(),
            McDb::getConnection(DB_MC_CACHES)->getRow(
                "SELECT * FROM mc_caches.T_DATEN_CACHE WHERE PARAMETER = ? AND VALUE = ?", "NAME", "testCacheSimple"
            )
        );

    }

    function testCleanupWithParam() {
        $aKonfig = array(DatenCache::ANLAGEN_ID() => 1);
        $this->assertSame(true, DatenCache::save("testCacheSimple", $aKonfig, "testString", "+120 seconds"));
        DatenCache::cleanup();
        $this->assertSame("testString", DatenCache::load("testCacheSimple", $aKonfig));
    }

    function testInvalidateByParam() {
        $mValue1 = 1;
        $mValue2 = 2;
        $aKonfigEins = array(DatenCache::ANLAGEN_ID() => $mValue1);
        $aKonfigZwei = array(DatenCache::ANLAGEN_ID() => $mValue2);

        $this->assertSame(true, DatenCache::save("testCacheInvalidateByParam",  $aKonfigEins, "testString"));
        $this->assertSame(true, DatenCache::save("testCacheInvalidateByParam2", $aKonfigZwei, "testString2"));

        DatenCache::invalidateByParam(DatenCache::ANLAGEN_ID());

        $this->assertSame(false, DatenCache::load("testCacheInvalidateByParam",  $aKonfigEins));
        $this->assertSame(false, DatenCache::load("testCacheInvalidateByParam2", $aKonfigZwei));


        $this->assertSame(true, DatenCache::save("testCacheSimple",  $aKonfigEins, "testString"));
        $this->assertSame(true, DatenCache::save("testCacheSimple2", $aKonfigZwei, "testString2"));

        DatenCache::invalidateByParam(DatenCache::ANLAGEN_ID(), $mValue1);

        $this->assertSame(false, DatenCache::load("testCacheSimple",  $aKonfigEins));
        $this->assertSame("testString2", DatenCache::load("testCacheSimple2", $aKonfigZwei));
    }

    function testInvalidateByParamLike() {
        $mValue1 = "I wan't a Pony";
        $mValue2 = "No Pony for you !";
        $mValue3 = "xxx";

        $this->assertSame(true, DatenCache::save("PonyCache", array("PonyParam" => $mValue1), "test1"));
        $this->assertSame(true, DatenCache::save("PonyCache", array("PonyParam" => $mValue2), "test2"));
        $this->assertSame(true, DatenCache::save("PonyCache", array("PonyParam" => $mValue3), "test3"));

        DatenCache::invalidateByParam("PonyParam", "%Pony%", true);

        $this->assertSame(false,   DatenCache::load("PonyCache", array("PonyParam" => $mValue1)));
        $this->assertSame(false,   DatenCache::load("PonyCache", array("PonyParam" => $mValue2)));
        $this->assertSame("test3", DatenCache::load("PonyCache", array("PonyParam" => $mValue3)));
    }

    /**
     * @expectedException PHPUnit_Framework_Error
     */
    function testInvalidSaveTime() {
        $this->assertSame(false, DatenCache::save("testFail", null, "test1", "invalidStrtotime"));
    }

    function testInvalidSaveTimeReturn() {
        $this->assertSame(false, @DatenCache::save("testFail", null, "test1", "invalidStrtotime"));
    }
}
\$\endgroup\$
  • \$\begingroup\$ This looks like the long-code-questions version of Stack Overflow \$\endgroup\$ – Carlos Muñoz Jan 20 '11 at 19:19
7
\$\begingroup\$

Other than long lines, I'm not seeing something too unreadable. I don't advise shortening function names as doing so makes it less clear what the function is doing. Instead, try splitting the function calls being passed as parameters onto different lines by either splitting the line across multiple lines or storing the returned value as a variable and passing that variable in.

| improve this answer | |
\$\endgroup\$
7
\$\begingroup\$

You should provide the optional string message as last argument for assertion calls; turn this:

$this->assertSame(true, DatenCache::save("testCacheSimple", false, $sDatenSimple));

Into this:

$this->assertSame(
  true,
  DatenCache::save("testCacheSimple", false, $sDatenSimple),
                                      "a successful [describe operation] ".
                                   "is expected for [describe arguments]");

This form reminds me of Perl instructions do something or die "message", and I use the indentation, keeping the message alone, right-aligned, on a separate line when needed, to put it in perspective:

assert( something,                                   "something expected");
assert( something else,                         "something else expected");
assert( something long
        that spans multiple lines,
                                                         "something long ".
                                     "that spans multiple lines expected");

In this way, all the messages get aligned on the right, and can be read separately.

Also, you should use the most specific assertion available instead of using assertSame() all the time. When comparing with true, using assertTrue() instead would clarify the intent.

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

IMHO, much like when writing a class a single method should do one thing and do that one thing well.

You should be doing less assertions per function in your test class, preferably 1 assertion per function.

As a general rule the least lines of code in a test function the better the test.

This way you can document a test of a usage example for each function and assertion pair and in effect you will be creating usage documentation for the class under test.

Edit: I just noticed after another look at your test that you are testing two functions in alot of the tests (load and save). These should be tested individually as one may affect the other and may produce unpredictable results.

Simple = Robust

I obviously don't know the class under test but i suspect you should be writing tests similar to below. Isolating the methods being tested and breaking the tests into smaller units. Hopefully this will help you.

<?php
// Instead of $this->assertSame() use $this->assertTrue() when testing for true.
// much easier to see what outcome is needed from test.

class DatenCacheTest extends PHPUnit_Framework_TestCase {

function setup(){
    // runs before every test like __construct() in a class.
    $this->str = 'testString';
}

function tearDown(){
    // runs after every test like __destruct() in a class.
}

function testSaveSimple(){
    $this->assertTrue( DatenCache::save("testCacheSimple", false, $this->str) );
}

function testSaveLinearParam(){
    $this->assertTrue( DatenCache::save("testCacheLinearParam", "string", $this->str) );
}

function testSaveBoundObjParam(){
    $oObj = new stdClass();
    $oObj->bob = 2;
    $this->assertTrue( DatenCache::save("testCacheBoundObjParam", $oObj, "zwei") );
}
// complete tests for all aspects of DatenCache::save()

// Test DatenCache::load() because DatenCache::save() has been tested.
function testLoadSimple(){
    DatenCache::save("testCacheSimple", false, $this->str);
    $this->assertSame( $this->str, DatenCache::load($this->str, false) );
}

function testLoadNoCache(){
    $this->assertFalse( DatenCache::load("testNoCache") );
}
\$\endgroup\$
  • \$\begingroup\$ I agree with this. Also you can use @covers in the docblocks to ensure that your test coverage is not over-estimated and that each test covers only the method it is trying to test. \$\endgroup\$ – Paul Feb 4 '12 at 3:26
3
\$\begingroup\$

I lack the syntax knowledge to say I understand the class. I can say reading it is a bit hard due to long lines and the fact that the dominant patterns are repetitive method/variable names. Test names are descriptive enough

Maybe if you could extract methods like DatenCache::load to Dload and $this->assertSame to aSame it would get less busy. Or make an utility function, since you're asserting equality on booleans (so assertTrue and assertFalse could make it clearer). Where possible, constructing more arrays outside the calls could also help.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.