How to go about testing un-injectable code?

Question

So I have the following piece of code in use all over my system. We're currently writing unit tests retrospectively (better late than never was my argument), but I don't see how this would be testable?

public function validate($value, Constraint $constraint)
{
 $searchEntity = EmailAlertToSearchAdapter::adapt($value);
 $queryBuilder = SearcherFactory::getSearchDirector($searchEntity->getKeywords());
 $adapter = new SearchEntityToQueryAdapter($queryBuilder, $searchEntity);
 $query = $adapter->setupBuilder()->build();
 $totalCount = $this->advertType->count($query);
 if ($totalCount >= self::MAXIMUM_MATCHING_ADS) {
 $this->context->addViolation(
 $constraint->message
 );
 }
}

Conceptually this should be applicable to any language, but I'm using PHP. The code simply builds up an ElasticSearch query object, based on a Search object, which in turn is built off an EmailAlert object. These Search and EmailAlert's are just POPO's.

My problem is that I don't see how I can mock out the SearcherFactory (which uses the static method), nor the SearchEntityToQueryAdapter, which needs the results from SearcherFactory::getSearchDirector and the Search instance. How do I inject something that gets built from results within a method? Maybe there's some design pattern I'm not aware of?

Thanks for any help!

• iLikeBreakfast: @DocBrown it is being used inside the $this->context->addViolation call, inside the if.
• Doc Brown: Must have been blind, sorry.
• Ewan: So all the :: are statics?
• Andy: Yes, in PHP the :: is for static methods.
• iLikeBreakfast: @Ewan yes, :: calls a static method on the class.

Answer 1

There are some posibilites, how to mock static methods in PHP, the best solution I have used is the AspectMock library, which can be pulled through composer (how to mock static methods is quite understandable from the documentation).

However, it's a last-minute fix for a problem which should be fixed in a different way.

If you still want to unit test the layer responsible for transforming queries, there's a pretty quick way how to do it.

I am assuming right now the validate method is part of some class, the very quick fix, which does not require you to transform all your static calls to instance call, is to build classes acting as proxies for your static methods and inject these proxies into classes which previously used the static methods.

class EmailAlertToSearchAdapterProxy
{
 public function adapt($value)
 {
 return EmailAlertToSearchAdapter::adapt($value);
 }
}
class SearcherFactoryProxy
{
 public function getSearchDirector(array $keywords)
 {
 return SearcherFactory::getSearchDirector($keywords);
 }
}
class ClassWithValidateMethod
{
 private $emailProxy;
 private $searcherProxy;
 public function __construct(
 EmailAlertToSearchAdapterProxy $emailProxy,
 SearcherFactoryProxy $searcherProxy
 )
 {
 $this->emailProxy = $emailProxy;
 $this->searcherProxy = $searcherProxy;
 }
 public function validate($value, Constraint $constraint)
 {
 $searchEntity = $this->emailProxy->adapt($value);
 $queryBuilder = $this->searcherProxy->getSearchDirector($searchEntity->getKeywords());
 $adapter = new SearchEntityToQueryAdapter($queryBuilder, $searchEntity);
 $query = $adapter->setupBuilder()->build();
 $totalCount = $this->advertType->count($query);
 if ($totalCount >= self::MAXIMUM_MATCHING_ADS) {
 $this->context->addViolation(
 $constraint->message
 );
 }
 }
}

• iLikeBreakfast: This is perfect! Didn't even think of proxies. Thanks!
• RubberDuck: I believe Michael Feather's referred to this as the "Wrap Static" technique in his book "Working Effectively with Legacy Code".
• Andy: @RubberDuck I am not entirely sure it is called proxy, to be honest. That's what I have been called it for as long as I can remember using it, Mr. Feather's name is probably better suited, I haven't read the book, though.
• RubberDuck: The class itself is certainly a "proxy". The dependency breaking technique is called "wrap static" IIRC. I highly recommend the book. It's full of gems like you've provided here.
• Eterm: If your job involves adding unit tests to code, then "working with legacy code" is a strongly recommended book. His definition of "legacy code" is "code without unit tests", the whole book is in fact strategies for adding unit tests to existing untested code.

Answer 2

First, I would suggest to split this up into separate methods:

public function validate($value, Constraint $constraint)
{
 $totalCount = QueryTotal($value);
 ShowMessageWhenTotalExceedsMaximum($totalCount,$constraint);
}
private function QueryTotal($value)
{
 $searchEntity = EmailAlertToSearchAdapter::adapt($value);
 $queryBuilder = SearcherFactory::getSearchDirector($searchEntity->getKeywords());
 $adapter = new SearchEntityToQueryAdapter($queryBuilder, $searchEntity);
 $query = $adapter->setupBuilder()->build();
 return $this->advertType->count($query);
}
private function ShowMessageWhenTotalExceedsMaximum($totalCount,$constraint)
{
 if ($totalCount >= self::MAXIMUM_MATCHING_ADS) {
 $this->context->addViolation(
 $constraint->message
 );
 }
}

This leaves you in a situation where you can consider to make those two new methods public and unit test QueryTotal and ShowMessageWhenTotalExceedsMaximum individually. A viable option here is actually not to unit test QueryTotal at all, since you would essentially test only ElasticSearch. Writing a unit test for ShowMessageWhenTotalExceedsMaximum should be easy and makes much more sense, since it would actually test your business logic.

If, however, you prefer to test "validate" directly, consider to pass the query function itself as a parameter into "validate" (with a default value of $this->QueryTotal), this will allow you to mock out the query function. I am not sure if I got the PHP syntax right, so in case I did not, please read this as "Pseudo code":

public function validate($value, Constraint $constraint, $queryFunc=$this->QueryTotal)
{
 $totalCount = $queryFunc($value);
 ShowMessageWhenTotalExceedsMaximum($totalCount,$constraint);
}

• iLikeBreakfast: I like the idea, but I want to keep the code more object-orientated instead of passing around methods like this.
• user22815: @iLikeBreakfast actually this approach is good regardless of anything else. A method should be as short as possible and do one thing and one thing well (Uncle Bob, Clean Code). This makes it easier to read, easier to understand, and easier to test.