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

getMockForAbstractClass sucks for testing concrete methods #1245

Closed
JeroenDeDauw opened this issue May 2, 2014 · 5 comments
Closed

getMockForAbstractClass sucks for testing concrete methods #1245

JeroenDeDauw opened this issue May 2, 2014 · 5 comments

Comments

@JeroenDeDauw
Copy link
Sponsor Contributor

While creating a test for a class that had some collaborator, I wanted to verify that a particular method of this collaborator got called, with some specific values. So I wrote this:

$platform = $this->getMock('Doctrine\DBAL\Platforms\AbstractPlatform');

$collector = new DropSchemaSqlCollector($platform);

$platform->expects($this->exactly(2))
    ->method('getDropForeignKeySQL');

This fails because it's an abstract class, so getMockForAbstractClass needs to be used.

$platform = $this->getMockForAbstractClass('Doctrine\DBAL\Platforms\AbstractPlatform');

Which results in the test failing:

Expectation failed for method name is equal to string:getDropForeignKeySQL when invoked 2 time(s).
Method was expected to be called 2 times, actually called 0 times.

I've been using PHPUnit for quite some time, and it was not obvious to me what was wrong. It took @Ocramius on IRC telling me I had to specify the concrete methods to mock. To me it's totally not intuitive that only a subset of methods (the abstract ones) get mocked by default in getMockForAbstractClass. That is issue one with this method.

The second issue is that passing in the methods is done with the 7th argument. Having 7 arguments (or 8 like this method) is rather insane if you ask me. However for this use case the real issue is that one needs to specify 5 parameter for no reason whatsoever just so you can pass in the method name(s). That is not something you want all over your tests, so you need to construct a method as follows:

private function getMockedAbstractClass($className, array $methods)
{
    return $this->getMockForAbstractClass(
        $className,
        array(),
        '',
        true,
        true,
        true,
        $methods
    );
}

I hope it's not controversial that this use case is not well served at present. (Unless I'm unaware of some other way to do this, which would be ideal.) Is adding a method with the interface of the one posted here, or something along the same lines to PHPUnit, a viable approach?

(You can find the whole test in which I encountered this here)

@whatthejeff
Copy link
Contributor

You can always use the builder interface.

$this->getMockBuilder('Doctrine\DBAL\Platforms\AbstractPlatform')
     ->disableOriginalConstructor()
     ->setMethods(array('getDropForeignKeySQL'))
     ->getMockForAbstractClass();

@JeroenDeDauw
Copy link
Sponsor Contributor Author

Yeah, that counts as a good way of doing it which I did not think about :)

@whatthejeff
Copy link
Contributor

Yeah, it's unfortunate that the mocking interface is still haunted by the mistakes of the past :/

@JeroenDeDauw
Copy link
Sponsor Contributor Author

Actually, even if there is a new API, it still sucks that one does not get an error when using the old one incorrectly? How about throwing an exception when someone added an "->expects()->method() thing" for a method that is not mocked?

@JeroenDeDauw
Copy link
Sponsor Contributor Author

Now added that here #1249

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

2 participants