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

Fix return type error for unexpected method calls #518

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tkotosz
Copy link
Contributor

@tkotosz tkotosz commented Jan 4, 2021

Problem

This PR attempts to fix the issue of triggering a type error when an unexpected method call happens. Now it will return the correct type and let the code execution finish, so the predictions can be evaluated at the end of the test run.
See related comment here: #441 (comment)
PR which introduced the issue: #441
Relevant GitHub issues:

Example - Classes to test

<?php

namespace Tkotosz\TestProject;

class Foo
{
    public function doStuff(Bar $bar)
    {
        $x = $bar->aaa();
        $bar->bbb($x+4);
    }
}
<?php

namespace Tkotosz\TestProject;

class Bar
{
    public function aaa(): int
    {
        return 42;
    }

    public function bbb(int $x): int
    {
        return $x+1;
    }
}

Example - spec 1

<?php

namespace spec\Tkotosz\TestProject;

use PhpSpec\ObjectBehavior;
use Tkotosz\TestProject\Foo;
use Tkotosz\TestProject\Bar;

class FooSpec extends ObjectBehavior
{
    function it_test_1(Bar $bar)
    {
        $bar->aaa()->willReturn(22);

        $this->doStuff($time);

        $bar->bbb(22+4)->shouldHaveBeenCalled();
    }
}

The test passes: The only expectation we have is that method bbb should have been called with 26 and that happened so everything is fine.
Note: This works with using shouldBeCalled before doStuff as well.

Example - spec 2

<?php

namespace spec\Tkotosz\TestProject;

use PhpSpec\ObjectBehavior;
use Tkotosz\TestProject\Foo;
use Tkotosz\TestProject\Bar;

class FooSpec extends ObjectBehavior
{
    function it_test_2(Bar $bar)
    {
        $bar->aaa()->willReturn(22);

        $this->doStuff($time);

        $bar->bbb(22+4)->shouldNotHaveBeenCalled();
    }
}

The test fails: The only expectation we have is that method bbb should NOT have been called with 26 and that happened so the test fails. (This also works using Argument::any() in the prediction)
Note: This works with using shouldNotBeCalled before doStuff as well.

Example - spec 3

<?php

namespace spec\Tkotosz\TestProject;

use PhpSpec\ObjectBehavior;
use Tkotosz\TestProject\Foo;
use Tkotosz\TestProject\Bar;

class FooSpec extends ObjectBehavior
{
    function it_test_3(Bar $bar)
    {
        $time->aaa()->willReturn(22);

        $this->doStuff($time);
    }
}

The test passes: There are no expectations so there is nothing that could fail here. So now the call to the method bbb is completely ignored, still not reported as an unexpected call. I am not sure if this is the desired behavior 🤔 since this basically eliminates the unexpected method call errors entirely. You will only see errors if you have a failed expectation.

@ciaranmcnulty
Copy link
Member

Sorry for the late review; the code change seems fine

The change of behaviour is what worries me, not sure how to handle it! Possibly we should have a shouldNotBeCalled-type expectation for all methods that aren't otherwise stubbed?

In either case we should add some tests that capture the new behaviour explicitly

@tkotosz
Copy link
Contributor Author

tkotosz commented Mar 17, 2021

no prob @ciaranmcnulty

The change of behaviour is what worries me, not sure how to handle it! Possibly we should have a shouldNotBeCalled-type expectation for all methods that aren't otherwise stubbed?

Hmm interesting idea... I will probably have some time to play with this over the weekend, then I will get back to you. :)

@Jean85
Copy link
Contributor

Jean85 commented Mar 17, 2021

This seems to me a broken solution: in the third example, there is no behaviour defined for bbb(), which still needs to return an integer. Prophecy will instead try to return null, and the code will break with a fatal nonetheless.

I think that we're battling the same issue from different angles (I was getting into it with #508 + #509): stubs and spies can have undefined behavior, but that was defined in Prophecy long before PHP got return types. Now that we have them, returning null is no longer a valid fallback, so we have to do something else.

@tkotosz
Copy link
Contributor Author

tkotosz commented Mar 17, 2021

Prophecy will instead try to return null, and the code will break with a fatal nonetheless.

yep currently that is case, but it is changed in this PR, so prophecy returns an int instead of null to avoid the type error

@stof
Copy link
Member

stof commented Mar 17, 2021

but it returns a generated value that might still break the following code if it does not match other invariants of the code.

@Jean85
Copy link
Contributor

Jean85 commented Mar 17, 2021

FTR, we're discussing reverting the whole #441 behaviour in #472.

@ciaranmcnulty
Copy link
Member

I'd appreciate any comments on #528 as a way forward

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

Successfully merging this pull request may close these issues.

4 participants