13

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!

  • @DocBrown it is being used inside the $this->context->addViolation call, inside the if. – iLikeBreakfast May 20 '16 at 7:34
  • 1
    Must have been blind, sorry. – Doc Brown May 20 '16 at 7:37
  • So all the :: are statics? – Ewan May 20 '16 at 7:50
  • Yes, in PHP the :: is for static methods. – Andy May 20 '16 at 8:02
  • @Ewan yes, :: calls a static method on the class. – iLikeBreakfast May 20 '16 at 8:02
11

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
            );
        }
    }
}
  • This is perfect! Didn't even think of proxies. Thanks! – iLikeBreakfast May 20 '16 at 10:12
  • 2
    I believe Michael Feather's referred to this as the "Wrap Static" technique in his book "Working Effectively with Legacy Code". – RubberDuck May 20 '16 at 11:41
  • 1
    @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. – Andy May 20 '16 at 11:44
  • 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 May 20 '16 at 11:48
  • 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 May 20 '16 at 12:18
4

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);
}
  • I like the idea, but I want to keep the code more object-orientated instead of passing around methods like this. – iLikeBreakfast May 20 '16 at 10:11
  • @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. – user22815 May 20 '16 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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