current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 11 down vote favorite

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!

share|improve this question
    
@DocBrown it is being used inside the $this->context->addViolation call, inside the if. – iLikeBreakfast yesterday
1  
Must have been blind, sorry. – Doc Brown yesterday
    
So all the :: are statics? – Ewan yesterday
    
Yes, in PHP the :: is for static methods. – David Packer yesterday
    
@Ewan yes, :: calls a static method on the class. – iLikeBreakfast yesterday
up vote 9 down vote accepted

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
            );
        }
    }
}
share|improve this answer
    
This is perfect! Didn't even think of proxies. Thanks! – iLikeBreakfast yesterday
1  
I believe Michael Feather's referred to this as the "Wrap Static" technique in his book "Working Effectively with Legacy Code". – RubberDuck yesterday
    
@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. – David Packer yesterday
1  
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. – RubberDuck yesterday
5  
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. – Eterm yesterday
up vote 4 down vote

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);
}
share|improve this answer
    
I like the idea, but I want to keep the code more object-orientated instead of passing around methods like this. – iLikeBreakfast yesterday
    
@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. – Snowman yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.