Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.10.0 regression #457

Closed
yannickl88 opened this issue Dec 18, 2019 · 17 comments
Closed

1.10.0 regression #457

yannickl88 opened this issue Dec 18, 2019 · 17 comments

Comments

@yannickl88
Copy link

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
Copy link
Author

related to changes made in #441

@stof
Copy link
Member

stof commented Dec 18, 2019

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

@ciaranmcnulty
Copy link
Member

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

Are you using Prophecy outside of a testing framework?

@yannickl88
Copy link
Author

@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
Copy link
Member

stof commented Dec 18, 2019

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

@ciaranmcnulty
Copy link
Member

@yannickl88 Hm, phpunit should be calling checkPredictions for you

@yannickl88
Copy link
Author

yannickl88 commented Dec 18, 2019

@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
Copy link
Member

stof commented Dec 18, 2019

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
Copy link
Author

@stof but would you consider this a BC break?

@stof
Copy link
Member

stof commented Dec 18, 2019

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
Copy link

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
Copy link
Member

@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
Copy link
Author

yannickl88 commented Dec 19, 2019

@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
Copy link
Author

yannickl88 commented Dec 19, 2019

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
Copy link
Author

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

@ciaranmcnulty
Copy link
Member

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

@yannickl88
Copy link
Author

alright, fair enough. Thanks for clearing that up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants