0

I'm working on a unit test for a small class. I'm using this class to get down and dirty with PHPUnit so I can start properly test bigger pieces of code that I write in the future.

Consider the following code I am trying to test:

/**
 * Registers a starting benchmark tick.
 *
 * Registers a tick with the ticks registry representing the start of a benchmark timeframe.
 *
 * @param string $id The identifier to assign to the starting tick.  Ending tick must be the same.
 * @return bool Returns TRUE if a tick was registered successfully or FALSE if it was not.
 * @since 0.1
 */
public function start($id)
{
    $this->tick($id . "_start");
    if($this->getStart($id) != false) {
        return true;
    }
    return false;
}

/**
 * Retrieves a registered start tick.
 *
 * Checks to see if a start tick is registered.  If found the microtime value (as a float) is
 * returned, otherwise FALSE is returned.
 *
 * @param string $id The identifier to lookup the tick under.
 * @return mixed The microtime (as a float) assigned to the specified tick or FALSE if the tick
 * start hasn't been registered.
 * @since 0.1
 */
public function getStart($id)
{
    if(isset($this->ticks[$id . "_start"])) {
        return $this->ticks[$id . "_start"];
    }
    return false;
}

And the following is the actual test code:

public function testBadStartTick()
{
    $this->assertFalse($this->bm->start("What_Invalid_Key_Fits_Here?"))
}

The problem is that this test function always returns true no matter how many times I try to make it return false. I have tried giving null values, keys of 300+ characters, empty arrays, even the instance of a new object. In all cases PHP either breaks or throws some kind of warning. When PHP doesn't break, my value gets converted to something that PHP will accept in the array key and then my test doesn't pass when attempting to do a $this->assertFalse().

I would like to achieve the highest amount of code coverage possible.

So my question is if these methods, given their current code, will ever return false under normal operation?

I'm thinking that because I am appending text (which is for management purposes) I am always supplying some kind of key that PHP will accept regardless of what I give for $id.

Any thoughts?

Thanks in advance!

Crackertastic
  • 4,958
  • 2
  • 30
  • 37
  • What does that `tick()` method actually do? That's the missing puzzle piece in your question. We cannot see the whole code path if you call `start()` in your test because anything might happen in this method. Including code that makes it impossible for your test case to run successfully. For example, if `tick()` registers that key in the `ticks[]` array, the call to `start()` will never return false, and mocking that will simulate a reality that cannot exist. In that case, the entire `if` is obsolete and you can always return true. – Sven May 07 '14 at 22:57
  • @Sven That is actually all that `$this->tick()` does, and was part of the reason for my question. I was curious if it was possible to provide PHP with a value that cannot be used as an array key, but won't bring PHP to its knees at the same time. If it wasn't possible, then what I was already thinking (that you have already touched on), was true that I can't ever get `false` out of `$this->start()`. So the ultimate fix is to ditch mocking the object in my test and remove the `if`. – Crackertastic May 08 '14 at 02:00
  • PHP will always either make the best it can to accept a variable as an array key, or fail fatally. Note that strings containing only digits will always be converted to integers. If you want to validate that you get passed a valid value as key, you have to do it yourself in your code to avoid fatal errors, like `if (is_string($id)){ /* add to array */}` - that will make a difference. – Sven May 08 '14 at 16:03

2 Answers2

0

How about editing your code yo verify the data start receives before using it?

public function start($id)
{
    if ($id !== null) { // perhaps more validation is needed?
        $this->tick($id . "_start");
        if($this->getStart($id) != false) {
            return true;
        }
    }

    return false;
}

public function getStart($id)
{
    if(isset($this->ticks[$id . "_start"])) {
        return $this->ticks[$id . "_start"];
    }
    return false
}

Then you can test like this:

public function testBadStartTick()
{
    $this->assertFalse($this->bm->start(null))
}
Cristian Quiroz
  • 385
  • 1
  • 8
  • Thanks for the response. I'm looking for a solution that doesn't involve editing the code. If what I am after is impossible then I can simply the return values since it isn't really needed. – Crackertastic May 07 '14 at 19:29
  • I believe that what you are after is then not possible, since the concatenation will cause `$id` to be casted as a string. – Cristian Quiroz May 07 '14 at 19:38
0

You will need to use mocks to do this.

It will require you to do this 2 parts to get the full coverage.

But you will have to do this

public function testBadStartTick()
{
    $bm = $this->getMock('bm', array('getStart'));

    $bm->expects($this->any())
         ->method('getStart')
         ->will($this->returnValue(false));

    $this->assertFalse($bm->start("What_Invalid_Key_Fits_Here"))
}

Of course replace bm for your actual classname (if needed with namespace). Like this you will mock the functionality of getStart for that test only and you can test what your result would be.

Of course if you want full coverage and test everything you will need as well

public function testGoodStartTick()
{
    $this->assertTrue($this->bm->start("What_Invalid_Key_Fits_Here"))
}

This will test the path if everything goes good. Because you don't mock there anymore it will use the actual getStart function.

public function testMissingStartTick()
{
    $this->assertFalse($this->bm->getStart("What_Invalid_Key_Fits_Here"));
}

This will test the getStart() independent of the other function and because of that the key wont exist and will return false.

The true path you should not have to test separate because you will test that with testGoodStartTick()

Edit: As people have said in the comments and i should have put that here as a warning it is never wise to have code that will never be executed. As you had a check there i thought tick() might do some magic that sometimes will not add the key to the array but i understand from your comment that that is not the case. So indeed in your case my answer is not the correct one and is best to follow the suggestion of removing the code that you know will always be true.

melvin
  • 1,145
  • 8
  • 12
  • Thanks. Looks like that got me a lot farther. They only issue I am having now is that I get a return value of `null` instead of `false`. Any thoughts on that? – Crackertastic May 07 '14 at 20:05
  • @Crackertastic i updated the answer for you. Problem was that you need a partial mock. When you add an array with function names as second parameter to the getMock it will mock just those functions otherwise it will mock everyhing – melvin May 07 '14 at 20:24
  • That worked wonderfully, thanks a lot! I was looking on the PHPUnit website for an API reference, but at first glance I only see a manual with various use case scenarios. – Crackertastic May 07 '14 at 20:32
  • If you want to read up on mocks (you can do so much more) http://phpunit.de/manual/3.7/en/test-doubles.html – melvin May 07 '14 at 20:34
  • Maybe I'm missing something but the code can never return false as the key is always set before calling getStart(), so for me it makes no sense mocking the object to make getStart() return false. You're not testing the object, just changing the way it works. – gontrollez May 07 '14 at 21:02
  • @gontrollez well yes and no in this case I did not see the tick function who knows what it does or will do. But if there is only one path that can be taken then you might opt for changing the function. But he was looking for an option without code changes and if you want full coverage then the mock is necessary – melvin May 07 '14 at 21:09
  • @melvin I disagree. If some parts of the code can never be reached you can't get full coverage. Instead you remove them, because they will never be executed. Other option is that the code is wrong, and I think this is the case. The OP states that he was trying to obtain falses but the tests didn't pass. Then he should change the code, obviously. Or do you change the tests when they fail? – gontrollez May 07 '14 at 21:27
  • @melvin Probably suggesting to mock the class under test isn't the best idea. If you cannot reach code in the real class in the test, you cannot reach that code in real use. So why is that code there? Or why do you think you can reach it? – Sven May 07 '14 at 22:54
  • gontrollez and sven after seeing Crackertastics comment i have to agree with you that he is better of removing the if statement. Crackertastic i update my answer but since it is not valid anymore you might want to unaccept it so i can delete it. – melvin May 08 '14 at 06:18