Fix issue #120 by delaying unexpected method call evaluation

[elvetemedve]

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]

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]

Looks good - is it a BC break though?

[tkotosz]

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 #120

[elvetemedve]

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

[ciaranmcnulty]

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]

This PR caused another BC break (which breaks running the phpspec suite of Propĥecy itself): it now adds method prophecies for shouldNotBeCalled calls, which can report other calls as unexpected by turning a stub into a mock.

[stof]

Returning null all the time here means that an unexpected call has a very good chance of triggering a TypeError due to not respecting the return type, instead of being reported as an unexpected call.

[ScreamingDev]

Dunno if already aware of, but wouldn't describing a fallback via Argument::cetera() do the trick?
See #472 (comment)

[stof]

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