1.10.0 regression

[yannickl88]

With the release of 1.10.0 we noticed some of our tests breaking. After some digging this is because the behavior has slightly changed. Maybe it's best explained with an example:

require __DIR__ . '/vendor/autoload.php';

class A {
    public function someMethod() {
        return 42;
    }
    public function otherMethod() {
    }
}

class Regression {
    public function test(A $a) {
        try {
            $a->someMethod();
        } catch (\Throwable $e) {
            return false;
        }
        return true;
    }
}

$prophet = new Prophecy\Prophet();
$prophecy = $prophet->prophesize(A::class);
$prophecy->otherMethod()->shouldNotBeCalled();

var_dump((new Regression())->test($prophecy->reveal()));

With 1.9.0 this outputs: bool(false) With 1.10.0 this outputs: bool(true)

This is because the prophecy no longer throws an error when dealing with methods which have not been prophesied. And while I admit this is a poor test, it means that some of our tests are breaking and need fixing with a feature release.

Is this an intended side effect?

[yannickl88]

related to changes made in https://github.com/phpspec/prophecy/pull/441

[stof]

your code snippet is never calling $prophet->checkPredictions(). so this is an invalid usage of Prophecy.

[ciaranmcnulty]

This evaluation has moved later, during checkPredictions as @stof says

Are you using Prophecy outside of a testing framework?

[yannickl88]

@stof not sure how that is related... I'm just demonstrating changed behavior.

@ciaranmcnulty No, we are using it inside phpunit. The example is just to show the changed behavior.

The core of the issue is that some code contains things like catch (\Throwable $e) and previously the test might have thrown an exception due to a missing prophecy call instead of a proper stub. So now no longer throwing an exception during execution changes this behavior and makes some tests fail.

[stof]

well, it will still throw this exception later and so make the test fail as well.

[ciaranmcnulty]

@yannickl88 Hm, phpunit should be calling checkPredictions for you

[yannickl88]

@stof @ciaranmcnulty Maybe this explains the issue better. https://travis-ci.org/yannickl88/prophecy-regression/builds/626601090

PhpUnit with the above test case. Problem is that is now fails where it previously didn't.

[stof]

I don't think Prophecy ever documented that the "unexpected method call" exception would be thrown at the time the method is called rather than when checking predictions (and btw, depending on how you define the prediction, it was already throw when checking predictions).

We did not envision the case where your own code is catching the Prophecy exceptions to alter its behavior.

[yannickl88]

@stof but would you consider this a BC break?

[stof]

the issue is, if we consider that case a BC break, we can basically never change anything in Prophecy (and never fix any bug due to that), as that might trigger the same kind of changes.

[pjordaan]

I had also an issue that my unit tests fail since the release of 1.10. I did the quick way by adding conflict on 1.10 for now and see what needs to be fixed.

[ciaranmcnulty]

@yannickl88 in the first example your test should fail right? You're saying the method shouldn't be called, but it is. The problem was that the over-eager catch was making the test pass

Are your phpunit tests similar cases?

[yannickl88]

@ciaranmcnulty No, I'm saying that otherMethod should not be called, which it isn't. This is only to have at least one method prophecy to force an error on unexpected someMethod.

But yeah, we had this in our code. All tests were faulty, I admit, but we are still fixing stuff because of this. Real world example:

class WaitForDeletionDate
{
    public function perform(Task $task)
    {
        if (! $contract = $task->getContract()) {
            throw new \RuntimeException(sprintf(
                'Task #%d does not have a contract associated with it.',
                $task->getId()
            ));
        }
        // ...
    }
}

class WaitForDeletionDateTest extends TestCase
{
    public function testPerformNoContract(): void
    {
        $this->task->getContract()->willReturn(null);

        $this->expectException(\RuntimeException::class);

        $this->action->perform($this->task->reveal());
    }
}

This used to work perfectly fine and also looks fine. But with the new changes it fails because $task->getId() should return an int. Previously this worked because $task->getId() threw a Prophecy exception that is hadn't been prophesied. The main issue was that the author of the tests didn't check the exception message, else he could have spotted this. But these mistakes are easily made.

[yannickl88]

As a side note and with the ability of hindsight, I would have preferred that the exception would still be thrown as it was in 1.9.0 but also a deprecation notice that there was still an Unexpected method call.

This way you can show the list of missed Unexpected method call's at the end as deprecations and have a plan to fix them. With the new major release you can remove the exception at call-time.

[yannickl88]

why was this closed? Or is the official stance that this was an intended side-effect?

[ciaranmcnulty]

Yes, I think if faulty tests were wrongly passing before we don't need to consider it a break.

[yannickl88]

alright, fair enough. Thanks for clearing that up!