Wednesday, August 18, 2010

Refactoring PHPUnit's getMock()

Not an actual refactoring, but at least the introduction of a layer of indirection, a Parameter object, called PHPUnit_Framework_MockSpecification. I have already written the patch in a branch of my github repository. They are actually two independent patches, since PHPUnit core and the mocking component are in two separate repositories:
All functionalities were Test-Driven Developed.

Use cases
The current API of getMock(), the Facade for the mocking library, actually prescribes 7 parameters. Most of them are optional, like in use case (a):
But if you want to specify an uncommon parameter, you have to include the previous ones, and hunt around for their default values, praying that you will get them right and insert the boolean or empty values in the correct order, like in (b):
return $this->getMock('MyClass', array(), array(), '', false);
In some cases (c):
$this->getMock('MyClass', array(), array(), '', true, true, false);
A Specification objectBuilder pattern, which I intend to propose as a feature request after getting some feedback from the community, will aid some of these use cases. For example a) remains the same: there is no need to complicate the API here.
For case b):
For case c):

State of development
I have currently implemented support for 6/7 of the getMock parameters in the MockSpecification object (only the autoload-related parameter is missing). This solution is an instance of the Builder pattern (I need a new name for MockSpecification, which started out as a parameter object but then acquired a getMock() method for a faster access to the created object).
Once the mock is created, it behaves exactly like an ordinary mock: MockSpecification calls getMock() internally.
This would be a totally backward compatible change, since it only adds a new way to creating a mock.

What I want from you
Any feedback, from glitches in the code to better names for the API methods and the class itself. I guess PHPUnit_Framework_Mock_MockBuilder can be the right name. Once tidied up the code, I'll open a ticket for a feature request on PHPUnit's trac asking to assess it and merge in the master repository.


fqqdk said...

I would invert certain defaults: for example I would consider good practice to not enable parent constructor by default, because I consider constructor injection better, and when mocking a class I don't want to deal with the mocked class' dependencies (coz that's why i'm mocking the class).
So I'd change the api to
...->callOriginalConstructor(), and if this isn't called, the API should assume that I don't want to call the original constructor.

I also have a tendency to use certain setups for certain roles in a test, so I tend to write wrappers for getMock() myself.

One kind of helper method I find myself writing for almost every project is the dummy($className) method, which creates a mock object of a certain type, and presets it so that it won't accept any calls (...->expect($this->never())->method($this->any()) or something like that), to make it clear that said object is just passing by (through layers mostly).

Or there's the argumentSpy(...) method which gives me an object for a certain scenario: an object of certain type whose certain method will be invoked once, and the parameters of the calls are matched only.

Another one is the factoryFor(...) method which I use in a scenario where I have to test some object who has a factory as a dependency to create other objects which then will be used by said objects. This method just creates a mock factory whose methods yield the other mock objects I create in the test without any hassle.

I find using these methods help clarify intents and make test code look nicer. Do you plan to include stuff like these?

jakyra said...

I love this. I could really use something like this.

Giorgio said...

@fqqdk: for the defaults, I used the settings from getMock() default arguments, so to change them can be confusing.
About the other methods instead, they are part of a larger scope of functionalities, so I would consider them for a second iteration. This is not in PHPUnit yet. :)

Unknown said...

@Giorgio this is brilliant. I do like the idea of not having to specify all the getMock() parameters before the one you actually want to set, and at the same time making it easier to read and understand. This can only be a bonus!
I will be trying your patches out over the weekend and I will give you my opinions over the implementation sometime after.

@fqqdk With your callOriginalConstructor() idea I both agree and disagree with you, let me explain.
I believe you are right when you said, "I consider constructor injection better", I do too; but I don't think this patch should reverse the default behaviour of PHPUnit. Both methods of creating a mock object should exactly match otherwise it could get very confusing, especially for noobs.
On another note, I like your suggested getMock() wrappers for common tasks/roles. I think I'll look into implementing something similar.

Giorgio said...

@Dan: the patches have been accepted and are in phpunit's master branch. The Builder will be available from release 3.5

Unknown said...

Pretty good efforts, using getMock is currently a major headache. Thanks for you work there.

Regarding default behavior, I would argue that this is the best time to change it. Halfway common sense is, the current behavior could be optimized, so introducing a new API is the perfect time to make things better. So my suggested default would be: no parent constructor, no parent clone, all found methods are mocked per default (maybe a good set of mutators to change this like mockMethod(string methodName) disableMocking(string methodName)), no constructor arguments.

fqqdk said...

We have been talking about two layers of abstraction. One is a quite thin builder wrapper over a factory method, the other is a more sophisticated way to create specific types of mocks for certain situations. As long as you use the second, you won't have any problems with the first. By that I mean the actual defaults of parameters won't matter in the majority of the situations. :)