Make a test fail if a mocked method is not expected to be invoked (Reopen) #65

Closed
ghost opened this Issue Sep 22, 2011 · 10 comments

Comments

Projects
None yet
7 participants
@ghost

ghost commented Sep 22, 2011

Hi, i found this issue in the closed section : #9

I reopened it because i consider it pretty major.

Here is an example, i want to test this :

class UniverseController
{
   public function doMaintenance(Universe $universe)
   {
      if (!$universe->peopleIsHappy())
      {
         $universe->giveJoy();
      }
   }
}
//Universe object
class Universe
{
   public function peopleIsHappy()
   {
      return true;
   }

   public function giveJoy()
   {
      //Give Joy
   }

   public function killEveryone()
   {
      die ("Everyone's dead now.");
   }
}

Now here's the test for the doMaintenance Method.

class UniverseControllerTest extends PHPUnit_Framework_TestCase
{
   public function testDoMaintenance()
   {
      $universeMock = $this->getMock("Universe");

      $universeMock->expects($this->once())->method("peopleIsHappy")->will($this->returnValue(false));
      $universeMock->expects($this->once())->method("giveJoy");

      $subject = new UniverseController();

      $subject->doMaintenance($universeMock);
   }
}

So if you run it as is, now, everything is fine.

Now imagine there's a troll that edits the UniverseController code and accidentally calls the killEveryone method as in this :

class UniverseController
{
   public function doMaintenance(Universe $universe)
   {
      if (!$universe->peopleIsHappy())
      {
         $universe->giveJoy();
      }
      $universe->killEveryone();
   }
}

You can confirm that it's a terrible error. Now rerun the test, still passes. Even though the $universe is a mock, it's really dangerous to ignore "unexpected" invocations.

The same thing happen if you take the other way around, if initially the doMaintenance() method behavior WAS to killEveryone() and you forget to expect it, the test will run, and someone come and see this line makes no sense, remove it and tests still good. So basically you can change the "contract" of a method without breaking tests.

So to me it is clear the test SHOULD fail, and i don't really understand the argument emitted in the issue i linked above. If it breaks a lot of test to fail unexpected mocked methods calls, then i'd say the test aren't really viable, they're assuming a mocked method call to return null when not expected at all.

If it would be too big to change this, maybe there could be a sort of "strict" mock that would NOT allow unexpected methods to be called.

I checked the code of the PHPUnit_Framework_MockObject_InvocationMocker::invoke method and it clearly isn't trivial to make sure the $invocation->methodName has at least once matcher attached to it, but my example above shows a clear flaw in the mocking system.

What do you think?

@vestild

This comment has been minimized.

Show comment
Hide comment
@vestild

vestild Jan 11, 2013

I solved this problem for me:
I add class

class FinalConstraint extends PHPUnit_Framework_MockObject_Matcher_InvokedRecorder{

    /**
     * @var integer
     */
    protected $expectedCount;

    /**
     * @param interger $expectedCount
     */
    public function __construct($expectedCount)
    {
        $this->expectedCount = $expectedCount;
    }

    /**
     * @param  PHPUnit_Framework_MockObject_Invocation $invocation
     * @throws PHPUnit_Framework_ExpectationFailedException
     */
    public function invoked(PHPUnit_Framework_MockObject_Invocation $invocation)
    {
        parent::invoked($invocation);

        $count = $this->getInvocationCount();

        if ($count > $this->expectedCount) {
            $message = $invocation->toString() . ' unexpected';

            throw new PHPUnit_Framework_ExpectationFailedException($message);
        }
    }

    /**
     * Returns a string representation of the object.
     *
     * @return string
     */
    public function toString() {
        return 'FinalConstraint';
    }

    /**
     * Verifies that the current expectation is valid. If everything is OK the
     * code should just return, if not it must throw an exception.
     *
     * @throws PHPUnit_Framework_ExpectationFailedException
     */
    public function verify() {
        $count = $this->getInvocationCount();
        if($count != $this->expectedCount){
            throw new PHPUnit_Framework_ExpectationFailedException(
                sprintf(
                    'Methods of class was expected to be called %d times, ' .
                        'actually called %d times.',

                    $this->expectedCount,
                    $count
                )
            );
        }
    }
}

And use:


$universeMock->expects($this->at(0))->method("peopleIsHappy")->will($this->returnValue(false));
$universeMock->expects($this->at(1))->method("giveJoy");

$universeMock->expects(new FinalConstraint(2))->method(new PHPUnit_Framework_Constraint_IsAnything());

vestild commented Jan 11, 2013

I solved this problem for me:
I add class

class FinalConstraint extends PHPUnit_Framework_MockObject_Matcher_InvokedRecorder{

    /**
     * @var integer
     */
    protected $expectedCount;

    /**
     * @param interger $expectedCount
     */
    public function __construct($expectedCount)
    {
        $this->expectedCount = $expectedCount;
    }

    /**
     * @param  PHPUnit_Framework_MockObject_Invocation $invocation
     * @throws PHPUnit_Framework_ExpectationFailedException
     */
    public function invoked(PHPUnit_Framework_MockObject_Invocation $invocation)
    {
        parent::invoked($invocation);

        $count = $this->getInvocationCount();

        if ($count > $this->expectedCount) {
            $message = $invocation->toString() . ' unexpected';

            throw new PHPUnit_Framework_ExpectationFailedException($message);
        }
    }

    /**
     * Returns a string representation of the object.
     *
     * @return string
     */
    public function toString() {
        return 'FinalConstraint';
    }

    /**
     * Verifies that the current expectation is valid. If everything is OK the
     * code should just return, if not it must throw an exception.
     *
     * @throws PHPUnit_Framework_ExpectationFailedException
     */
    public function verify() {
        $count = $this->getInvocationCount();
        if($count != $this->expectedCount){
            throw new PHPUnit_Framework_ExpectationFailedException(
                sprintf(
                    'Methods of class was expected to be called %d times, ' .
                        'actually called %d times.',

                    $this->expectedCount,
                    $count
                )
            );
        }
    }
}

And use:


$universeMock->expects($this->at(0))->method("peopleIsHappy")->will($this->returnValue(false));
$universeMock->expects($this->at(1))->method("giveJoy");

$universeMock->expects(new FinalConstraint(2))->method(new PHPUnit_Framework_Constraint_IsAnything());
@edorian

This comment has been minimized.

Show comment
Hide comment
@edorian

edorian Jan 14, 2013

Collaborator

We can't change the way the getMock() function works because thats the behavior that is expected.

If we make a new mockig API or a new entry point to the mocking API it's something I'd strongly consider but for now workarounds like the one showed here are the only option to get that behavior.

I'd also consider a PR that adds something like $mock->treatAllUndefinedMethodCallsAsFailures(); (with a saner name) as a workaround for people/projects that want that behavior.

But the main point is that, for BC reasons, we can't change the current behavior. So closing this. Sorry

Collaborator

edorian commented Jan 14, 2013

We can't change the way the getMock() function works because thats the behavior that is expected.

If we make a new mockig API or a new entry point to the mocking API it's something I'd strongly consider but for now workarounds like the one showed here are the only option to get that behavior.

I'd also consider a PR that adds something like $mock->treatAllUndefinedMethodCallsAsFailures(); (with a saner name) as a workaround for people/projects that want that behavior.

But the main point is that, for BC reasons, we can't change the current behavior. So closing this. Sorry

@edorian edorian closed this Jan 14, 2013

@lackovic10

This comment has been minimized.

Show comment
Hide comment
@lackovic10

lackovic10 Oct 5, 2015

This is an old issue, but also 👍 from me for @ghost. I think it would be great to be able to create a mock object with a flag $strict, which would make a test fail if a method call is not expected, or even throw an exception. Has anything similar been done recently ??

This is an old issue, but also 👍 from me for @ghost. I think it would be great to be able to create a mock object with a flag $strict, which would make a test fail if a method call is not expected, or even throw an exception. Has anything similar been done recently ??

@sylfabre

This comment has been minimized.

Show comment
Hide comment
@sylfabre

sylfabre Sep 1, 2016

@edorian Would you still consider a PR with a method $mock->allowUnexpectedInvocation(true / false); which would set a $allowUnexpectedInvocation property (true as default value) used near line 145 of class PHPUnit_Framework_MockObject_InvocationMocker

image
to
image

@ghost solution is nice but you still need to count how many times you've used ->expects()

sylfabre commented Sep 1, 2016

@edorian Would you still consider a PR with a method $mock->allowUnexpectedInvocation(true / false); which would set a $allowUnexpectedInvocation property (true as default value) used near line 145 of class PHPUnit_Framework_MockObject_InvocationMocker

image
to
image

@ghost solution is nice but you still need to count how many times you've used ->expects()

@lennerd

This comment has been minimized.

Show comment
Hide comment
@lennerd

lennerd Sep 20, 2016

Sad to see this closed as this could be a part of the MockBuilder. 👍

lennerd commented Sep 20, 2016

Sad to see this closed as this could be a part of the MockBuilder. 👍

@emil-nasso

This comment has been minimized.

Show comment
Hide comment
@emil-nasso

emil-nasso Nov 2, 2017

Has anything changed in recent phpunit versions that makes something in the line of what @sylfabre showed possible?

Has anything changed in recent phpunit versions that makes something in the line of what @sylfabre showed possible?

@sylfabre

This comment has been minimized.

Show comment
Hide comment
@sylfabre

sylfabre Nov 2, 2017

@emil-nasso I've added this to our backlog, I'll try to make a PR once we're done with our upgrade to PHP 7

sylfabre commented Nov 2, 2017

@emil-nasso I've added this to our backlog, I'll try to make a PR once we're done with our upgrade to PHP 7

@emil-nasso

This comment has been minimized.

Show comment
Hide comment
@emil-nasso

emil-nasso Nov 2, 2017

@sylfabre That is great news, i wish i could buy you a beer or even give you a kiss. :)

@sylfabre That is great news, i wish i could buy you a beer or even give you a kiss. :)

@emil-nasso

This comment has been minimized.

Show comment
Hide comment
@emil-nasso

emil-nasso Jan 30, 2018

Can i see the backlog anywhere or the status of this issue?

Can i see the backlog anywhere or the status of this issue?

@amorimjuliana

This comment has been minimized.

Show comment
Hide comment
@amorimjuliana

amorimjuliana Jan 31, 2018

Any news about this?

Any news about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment