Fix issue #120 by delaying unexpected method call evaluation

elvetemedve commented 5 years ago

This PR fixes the bug #120.

The problem

Unexpected method call check is happening immediately after the method called on the test subject. This check is not aware of the method prophecies (expected method calls) defined by spies (shouldHaveBeenCalled). Therefor it throws an exception, but it shouldn't.

Given the following specification and implementation, PHPSpec reports an error, but it shouldn't.

Specification

class SpiesExampleSpec extends ObjectBehavior
{
    function it_does_not_expect_spied_on_methods(\DateTime $time)
    {
        $time->getOffset()->willReturn(-1800);

        $this->modifyTimestamp($time);

        $time->setTimestamp(1234)->shouldHaveBeenCalled();
    }
}

Implementation

class SpiesExample
{
    public function modifyTimestamp(\DateTime $time)
    {
        $time->getOffset();
        $time->setTimestamp(1234);
    }
}

Output

  10  ! it does not expect spies
      method call:
        Double\DateTime\P1->setTimestamp(1234)
      was not expected.
      Expected calls are:
        - getOffset()

The solution

Instead of throwing the exception, the unexpected method calls are recorded, and they are checked again after an example is executed. At this point all spies have registered their method prophecies, so we can identify the correct list of unexpected method calls.

Consequences

The order of reported error types has changed.

Before

Unexpected method calls (any method call without return promise)
Spies (e.g. shouldHaveBeenCalled)
Other predictions (e.g. shouldBeCalled)

After

Spies (e.g. shouldHaveBeenCalled)
Unexpected method calls (any method call without return promise)
Other predictions (e.g. shouldBeCalled)

To make the order unchanged, it would be possible to delay the evaluation of spy predictions in a similar way how it's done with unexpected method calls.

Also currently the "other predictions" check raises an AggregateException with all the failed predictions. By delaying spy prediction checks, we could aggregate all failed predictions into one AggregateException.

tkotosz commented 5 years ago

FYI @ciaranmcnulty We paired with Géza today to attempt to fix this really old issue. Please let us know what you think about it. :)

ciaranmcnulty commented 5 years ago

Looks good - is it a BC break though?

tkotosz commented 5 years ago

I am not sure... The test result for any existing test should be the same as before, the only difference is the evaluation order of the predictions, which allows to support mocking some methods while only spying others on the same collaborator. As mentioned in the PR we could even make the order of errors to be the same as before with a bit more change while still fixing the problem of https://github.com/phpspec/prophecy/issues/120

elvetemedve commented 5 years ago

Test evaluation is not equivalent (see consequences section), but does not break BC.

ciaranmcnulty commented 4 years ago

Some previously-failing tests will now pass, right? I don't think that's a BC break and I'm pleased to see this contribution :-)

stof commented 4 years ago

There is also another drawback: the stack trace of the UnexpectedCallException will not longer tell you where the unexpected call was done.

phpspec / prophecy