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

add existing/non-existent method helpers #1139

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

unstoppablecarl
Copy link
Contributor

@unstoppablecarl unstoppablecarl commented Aug 18, 2021

This PR adds some helper methods I proposed in this issue:
#1129

Context

Currently you can set methods to be strictly checked to exist and match the method signature and return type via

Mockery::getConfiguration()->allowMockingNonExistentMethods(true/false);

This is set to false by default. There is no clean API to be able to specify the desired behavior for the method you are mocking.

Changes

2 methods are added to the mock object. They wrap the $mock->shouldReceive() method.

$mock->shouldReceiveExisting()

sets Mockery::getConfiguration()->allowMockingNonExistentMethods(true);
then stores the mocked method
then reverts Mockery::getConfiguration()->allowMockingNonExistentMethods($prev); to whatever it was set to previously

$mock->shouldReceiveNonExistent()

Does the same as above but the reverse. Sets to allow non-existent methods then sets it back to what it was previously.

Testing

I have not been able to get the tests to run locally. I get a ton of

The use statement with non-compound name 'Closure' has no effect

errors. I have not been able to find any reference to this error message anywhere. There are no docs on running the tests locally either. Any help would be appreciated.

@davedevelopment
Copy link
Collaborator

Thank you for the PR. I'll try and look in to the test runs.

@@ -20,6 +20,8 @@

namespace Mockery;

use Closure;
use Mockery;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two lines need removing to get the tests running.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davedevelopment Thanks that got the tests working! So if I add some tests will this be accepted?

@davedevelopment
Copy link
Collaborator

The more I think about this, the less and less enamoured I am by it. I've been using Mockery extensively for almost 10 years and never have I really thought this was something I needed.

I appreciate the effort and all, but it's still a 👎 from me.

@unstoppablecarl
Copy link
Contributor Author

Without checking that a method signature is valid, changes to a mocked method signature will not cause a test to fail. This means you cannot rely on mocked class/interface testing to detect incorrect or changed signatures.

Example

You have this starting code:

class Foo
{
    public function doSomething(string $value): string
    {
        return $value . ' with a suffix';
    }
}

class Bar
{
    public function useFoo(Foo $foo): void
    {
        $foo->doSomething('bar');
    }
}

class FooTest extends TestCase
{
    public function test_doSomething()
    {
        $expected = 'whatever with a suffix';

        $actual = (new Foo)->doSomething('whatever');

        $this->assertEquals($expected, $actual);
    }
}

class BarTest extends TestCase
{
    public function test_useFoo()
    {
        $foo = Mockery::mock(Foo::class);

        $foo->shouldReceive('doSomething')
            ->once()
            ->with('bar')
            ->andReturn('bar with a suffix');

        (new bar)->useFoo($foo);
    }
}

The Foo class is updated to the following code. You then see the FooTest fails and update it to use the new signature.

class Foo
{
    public function doSomethingAndSomethingElse(string $value, int $count): string
    {
        return $value . ' with a suffix ' . $count;
    }
}

class FooTest extends TestCase
{
    public function test_doSomethingAndSomethingElse()
    {
        $expected = 'whatever with a suffix 99';

        $actual = (new Foo)->doSomethingAndSomethingElse('whatever', 99);

        $this->assertEquals($expected, $actual);
    }
}

The BarTest will continue to pass as it is not checking that the method signature is valid. In any medium or large sized project, this is important as you cannot hold the entire codebase in your head and remember all the things that need to be updated.

There are cases like magic methods where this has to be disabled as there is no method signature to check.

Currently, it is common for devs to use some form of the code below in cases with magic methods.

        $thing->shouldReceive('methodThatExists')
            ->once()
            ->with('foo')
            ->andReturn('bar');

        $this->allowingNonExistingMockMethods(function () use ($thing) {
            $thing->shouldReceive('methodThatIsMagic')
                ->once()
                ->with('foo')
                ->andReturn('bar');
        });

        // with this PR the following could be used instead
        $thing->shouldReceiveNonExistent('methodThatIsMagic')
            ->once()
            ->with('foo')
            ->andReturn('bar');
    }

    protected function allowingNonExistingMockMethods(callable $callback)
    {
        $config = Mockery::getConfiguration();
        $allowed = $config->mockingNonExistentMethodsAllowed();
        $config->allowMockingNonExistentMethods(true);

        try {
            $result = $callback();
        } finally {
            $config->allowMockingNonExistentMethods($allowed);
        }

        return $result;
    }
}

I thought it would be beneficial for convenience and consistency across different codebases that have their own version of this. So this PR is just giving a definition to a common pattern that every codebase I've worked with in the last 6-ish years has had.

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.

None yet

2 participants