Closed webignition closed 6 years ago
I created PR #226 to address this.
@webignition While I don't have strong argument against the addition of a ManagerInterface
when creating the Manager
class I was under the impression that no interface was needed as the manager is a simple "builder" and many strategies can be used to build a Rules
object (that's why the Rules
object contains named constructor). Couldn't you just used the Rules
object in your test directly ?
@nyamsprod That's a good point. Yes, I can mock a Rules
instance in cases that need such an object.
In relation to the problem I'm facing for which I created this issue, I'm creating a service that creates a Rules
object and which makes use of a Manager
object to do so. In this case, I would like to be able to mock the Manager
that is used by the service such that calls to Manager::getRules()
are deterministic and which don't rely on the making of any filesystem calls or HTTP requests.
For clarification, here is the (very simplistic) service in question:
class RulesFactory
{
/**
* @var Manager
*/
private $manager;
public function __construct(Manager $manager)
{
$this->manager = $manager;
}
/**
* @return Rules
*/
public function create()
{
return $this->manager->getRules();
}
}
It's a very basic factory used within a Symfony project that creates a Pdp\Rules
object, then available a Symfony service via some additional service config not included here, to be injected into other services.
The other services into which a Rules
object is injected can certainly be passed a mocked Rules
instance within unit tests. Unit tests for the above service itself need to be passed a mocked Manager
object.
With the changes suggested in this issue, I could then modify the constructor of the above service to read:
public function __construct(ManagerInterface $manager)
{
$this->manager = $manager;
}
This then allows a mock built around the ManagerInterface
definition to be used as needed.
Issue summary
I'm developing a service that uses a
Pdp\Manager
so as to get aPdp\Rules
instance from which I can then get the registrable domain for a given URL.I'd like to mock a
Pdp\Manager
instance in unit tests. Unit tests are happy to assume thatPdp\Manager
works correctly. The tests just care about the return values ofPdp\Manager::getRules()
andPdp\Manager::refreshRules()
.I should be able to create a mock
Pdp\Manager
instance with Mockery so as to mock the return values of this relevant public methods.Mockery cannot create full mocks of classes declared as
final
, as is the case withPdp\Manager
. That the class is declared as final is fine in this case.This would be a non-issue if
Pdp\Manager
implemented an interface against which mocks could be created.Note: I have for now resorted to creating functional tests which mock the cache and http client passed to the
Pdp\Manager
constructor. This allows me to test my code without the tests having to make like HTTP requests when refreshing the PSL data. This works but is non-ideal as functional tests are more costly (take longer) than what the equivalent unit tests could be.System informations
Not a bug ... but here's the info anyway.
Standalone code, or other way to reproduce the problem
Expected result
$rulesFactory
should be aFoo\Services\Pdp\RulesFactory
instance. Methods of this instance can be called in tests. Such methods make use of the mockedPdp\Manager
instance.Actual result
"Mockery\Exception: The class \Pdp\Manager is marked final and its methods cannot be replaced. Classes marked final can be passed in to \Mockery::mock() as instantiated objects to create a partial mock, but only if the mock is not subject to type hinting checks."
The latter part of the exception message ("only if the mock is not subject to type hinting checks") also prevents me from creating a partial mock from an instantiated
Pdp\Manager
instance as the exampleRulesFactory
class is type-hinted to expect aPdp\Manager
argument.Interface solution
Create
Pdp\ManagerInterface
which defines the signatures forgetRules()
andrefreshRules()
and havePdp\Manager
implement this new interface.I'm happy to pop together a PR to deal with this.