How to write testable code

This is something I put together for the engineering team at Howsy. Hopefully it will be of some assistance for others. The examples are in PHP but can be abstracted to any OOP language

What is Unit Testing?

It’s simply a test that runs against an individual ‘unit’ or component of software. We are testing the smallest possible implementation of a class/method. A unit test does not rely on any dependencies (other classes/libraries or even the database)

Read more about unit testing here.

Why is it important?

I mean we could just stick with integration tests right? They are easy to write, however ...

The goal of any Software Project is to produce a maintainable, extensible codebase which meets business requirements. Writing effective unit tests help us to ensure we are writing SOLID code.

Read more about solid here.

How can we write more testable code?

I have taken a very simple class and it is typical of some of the code I have seen over the years.

class ToActive
{
    public function transition(int $accountId): void
    {
        $account = Account::find($accountId);
        if ($this->canTransition($accountId)) {
            $account->setState('active')->save();
        }
    }

    private function canTransition(Account $account): bool
    {
        $validator = new ToActiveValidator();

        if ($validator->validate($account)) {
            return true;
        }
        return false;
    }
}

On the surface the class appears to work. It finds an account from the DB, calls a validation method in another class and returns true/false. Based on the result the Account may or may not be updated.

Although this class works it is impossible to unit test due to two different dependencies being instantiated rather than injected. The two offending pieces of code are

$account = Account::find($accountId);

$validator = new ToActiveValidator();

As they are in the body of our methods they will be called during unit testing. This will cause us to rely on the responses from other classes in order to run a test. This makes any resulting tests we write for this class not true unit tests.

We can refactor this class very easily to make it more easily testable and therefore more extensible and loosely coupled.

Refactoring

class ToActive
{
   private $account;
   private $validator;

   public function __construct(AccountInterface $account, StateTransitionValidatorInterface $validator)
   {
       $this->account   = $account;
       $this->validator = $validator;
   }

   public function transition(): void
   {
       if ($this->validator->validate($this->account)) {
           $this->account->setState('active')->save();
       }
   }
}

As you can see from the above we are now injecting dependencies.

Our class does not care about the concrete implementation of the Account or the Validator - only that they adopt the required interfaces. This again makes our code more loosely coupled. In the future maybe we may completely change the StateTransitionValidator implementation or even add additional validators. Our code is now less tightly coupled and therefore more testable.

Now let’s get to testing

Looking at the class we have one public method. We should not be trying to test private methods as they are implementation details. If you find your class has too many private methods or feel that they need testing it is a sure fire sign that your class could do with splitting out into smaller components.

There are two paths within the public method - the validation passes and we save the entity or the validation does not pass and we don't save it.

I will write the test assuming the validation passes first. We are going to be mocking our dependencies (something not possible with the original class architecture).

public function testToActiveWithPassingValidationShouldTransition()
{
   /** @var Mockery\MockInterface|Account $account */
   $account = Mockery::Mock(Account::class);
   $account->shouldReceive('setState')
       ->with('active')
       ->andReturn($account)
       ->once();
   $account->shouldReceive('save')
       ->andReturn($account)
       ->once();

   /** @var Mockery\MockInterface|ToNotActiveValidator $validator */
   $validator = Mockery::mock(ToActiveValidator::class);
   $validator->shouldReceive('validate')
       ->with($account)
       ->andReturn(true)
       ->once();

   $service = new ToActive($account, $validator);
   $service->transition();
}


public function testToActiveWithFailingValidationShouldNotTransition()
{
   /** @var Mockery\MockInterface|Account $account */
   $account = Mockery::Mock(Account::class);
   $account->shouldReceive('setState')
       ->with('active')
       ->andReturn($account)
       ->never();
   $account->shouldReceive('save')
       ->andReturn($account)
       ->never();

   /** @var Mockery\MockInterface|ToNotActiveValidator $validator */
   $validator = Mockery::mock(ToActiveValidator::class);
   $validator->shouldReceive('validate')
       ->with($account)
       ->andReturn(false)
       ->once();

   $service = new ToActive($account, $validator);
   $service->transition();
}

Breaking it down

First of all we use Mockery to mock our account class.

We tell our mock that we will call the method setState with an argument of active. We also assert that this function will be called exactly once (this is our actual test assertion)
We also assert that the save() method will be called exactly one time.

We also mock our concrete validator class. We tell our mock that the validate method shall be called once and that it will always return true.

The tests will pass and you can see we have done so without actually relying on our dependencies. These were mocked and injected into our class with all of their methods returning exactly what we defined in our test.

Testing the reverse

We also want to test the second path (what if the validation returns false, business rules dictate we should therefor not transition the object)

Take a look at the second test and you can see our test is pretty similar only now we are forcing the validator to return false. As such our assertions have changed and we now expect setState and Save to never be called on our account object.

What we achieved

The original code was hard to test and tightly coupled. The refactor has allowed us to make the code more testable but a great side affect of this is that the code is also now more flexible.

Let me know if you have any questions in the comments!

Comments

[Jesse Phillips]

See when I look at that function I'm less concerned with whether the function calls into the validator or that it tries to set a specific state.

I want to know if the correct validations occur when activation is done, or that the same validation occurs in related system components, or that to inactive has an inverse validation.

I can read what that code does, the test just created a new language for me to learn. If the code changes and breaks the test, why are you changing code you did not intend to change?

Understandably this is an example, but I'm still more interested in why the validator didn't get the tests.

[Daniel]

Hi Jesse

The validation class would of course have a completely separate set of unit tests.

This was to demonstrate how to unit test. I don’t even show the code for the validation class as it’s implementation does not matter for this example.

I appreciate the validation may be interesting to test but we still need to unit test this class and method in order to achieve good coverage. What if at some point the code was refactored and the state update was not actually called - okay great we validate the state can be updated but we never actually call to do so. Both elements need testing. I just chose this one for the purpose of the example.

I’m not sure what you mean by “why are you changing code you did not intend to change” - I refactored the code because it was necessary in order to make it SOLID and testable.

[Jesse Phillips]

I'm challenging the notion that this was nessessary. Your introducing a change into the codebase that needs additional review and testing.

Yes you added tests which were not there before, but that is the point. You've refactored code that did not have its behavior covered by tests.

Consider this, you've chosen to show specific code which demonstrates mocking and yet there is not enough information or tests to verify that the system behaves the same. Sure we can see that an account has had a validation done along with being requested to activate. But I can't tell you if it is the same account as before the refactoring or if the same validation is being run against it.

This makes code more complex and increases the risk behavior changed.

What if at some point the code was refactored and the state update was not actually called

That is a risk I'm willing to take because like this change if the only reason for change is meeting principles and code coverage metrics I'd reject the change and request that the refactoring take place when a behavior change is needed.

I can appreciate that this is a tutorial on writing unittest and testable code and thus the specific code shown is only what is needed to present the tooling to do so. I'm wanting to surface further thoughts on the implications of the changes and how the examples do not show the completed work.

What does your Test Cover?

Jesse Phillips ・ Jan 27 ・ 1 min read

#discuss

Had you just written a unit test against the original private method I would not have had concerns with changes to functionality. Yes it would have broken principles and we still wouldn't be testing if the account got its activation called. But we could have covered the account validation edge cases and failures, an area I think is at greater risk of being wrong or breaking from changes to code elsewhere.

[Daniel]

'Consider this, you've chosen to show specific code which demonstrates mocking and yet there is not enough information or tests to verify that the system behaves the same. Sure we can see that an account has had a validation done along with being requested to activate. But I can't tell you if it is the same account as before the refactoring or if the same validation is being run against it.'

That is not true. The code was not covered by tests and now it is. The test for a state transition adequately covers the following business requirements:

• Given a transition validator passes we expect the set state method to be called with the relevant state. We also expect the model to be saved.

• Given a transition validator fails we do not expect to update the state or update the model.

'That is a risk I'm willing to take because like this change if the only reason for change is meeting principles and code coverage metrics I'd reject the change and request that the refactoring take place when a behavior change is needed.'

Given you have no domain knowledge it is not a risk for you to take. I also find it surprising that considering the unit tests cover the relevant paths and as mentioned cover business requirements that you think they serve only for coverage.

'This makes code more complex and increases the risk behavior changed.'

The code is not more complex. It serves the exact same function as before in a similar number of lines and using no more abstraction.

I think you have some fundamental misunderstandings about the code/testing.

'Had you just written a unit test against the original private method I would not have had concerns with changes to functionality. Yes it would have broken principles and we still wouldn't be testing if the account got its activation called. But we could have covered the account validation edge cases and failures, an area I think is at greater risk of being wrong or breaking from changes to code elsewhere.'

This is where you demonstrate the lack of knowledge mentioned above. As I already stated the account validation edge cases and failures would be handled by the unit tests for the actual class that handles the validation. This class only handles the transitioning. Writing tests for the validation would be done elsewhere. More troubling is your wish to test a private method.

[Jesse Phillips]

I appreciate your concerns for my lack of domain knowledge and disregard for principals you hold. But I have my concerns that you aren't adequately testing the original behavior, and this response only reaffirms you don't understand my concerns because you're only focusing on one part of the original code.

As for the domain knowledge, you mentioned a refactoring gone awry. I'm claiming the refactor should not happen, why is domain knowledge relevant when I still don't let the bad code through?

[Daniel]

The refactoring was necessary in order to make the code less tightly coupled and in order to allow it to be unit tested. Unit tests are impossible when I have concrete dependencies instantiated in my methods.

The unit tests for cover the business requirements and paths for this class. There would be a myriad of other tests covering the validation class but they are not relevant here.

[Jesse Phillips]

I was not referring to the validation class. There is not enough code present to verify the correct validation is happening, sure you have tests in the validation class, but is that the class used?

[Theodore R. Smith]

I hope you have learned a lot in the last three years. You totally lack understanding of LSP, SRP and testing in general, yet have a pompous attitude to boot.

[Daniel]

@Jessie - There is plenty of code present to verify the transition takes place and this is what we are testing.

I already provided the business requirements for the class. Which of these requirements does the code not test ?

There are two paths - both of these are adequately covered.

'sure you have tests in the validation class, but is that the class used?'

What do you mean by this - this demonstrates to me you do not understand how to write software. You can see from the code that the class is being used. It is injected and its responses determine whether the transition runs or not.

The actual validation in this case is being mocked because it is a dependency of this class.

[Jesse Phillips]

Yes the validator is injected, but that is only the test code. Did the original calling code get updated to inject the correct class? I don't know because it isn't tested.

This is something a merge request for this refactor would answer (because the article is a tutorial) thus my comments that their is more happening then just tests being added.

[Daniel]

Of course the calling code would have been updated to inject the correct class. This has its own tests as well.

I didn't add it because at some point the tutorial has a boundary and short of showing every call from input to output (including my DI container and framework code) I'm not going to cover every eventuality in a short post.

The tutorial was simply taking a single class and making it more testable.

[Jesse Phillips]

I do understand the challenge with writing a consice tutorial, I'm trying to point out there is still risk introduced by this refactor.

I would actually love to see how you test that the correct verification object is injected in a unittest, I personally don't know how I'd do it.

[Daniel]

I think you may still have a slight misunderstanding of the SOLID principles.

The validation object is type hinted on the constructor. If I attempted to pass any other object (one that does not implement the interface) I would throw an exception. The test in this case is to test the actual State Transition class. I have other tests at the level these classes are called. These are the tests that would pick up if I was passing an incorrect interface.

I could have type hinted against a concrete implementation of a validation class but we should avoid doing that and code to an interface where possible.

[Jesse Phillips]

I understand the purpose of the interface, what I'm seeing is that code is changing and it's behavior is not expected to change. The interface does not define behavior, the validation class tests do, but I need to know the activation behavior has not changed. And this code change, by design, decouples the validation logic from the taking action logic.

[Daniel]

I think you need to go back and revisit your software engineering fundamentals.

[Jesse Phillips]

The purpose of my comment is to get you to think about the implications of following those principles, especially when the modified code had no reason to change other than to meet those principles.

This comment sounds more like you don't want to defend what you did and hope that generic explanations of how to write code will explain why it is OK to modify untested code. I can tell you right now the first advice will be to write a unittest to test the existing code before you refactor.

[Daniel]

As has already been explained clearly in the post - it is impossible to write a unit test for the original code because dependencies were instantiated. How does one write a proper unit test for a piece of code that has hard-coded dependencies.

Your assumption that the code was not covered is interesting though because it actually was covered by feature/integration tests. The feature tests still passed after the refactor by the way. The feature tests took the calling code for the State Transition class - passed with concrete validators and data from the database and then verify that after we call the transition end point the actual record in the DB has transitioned as expected. But all of that is completely out of the scope of the original blog.

Your comments have read like someone who is learning about proper software architecture. The end points you have arrived at differ wildly to your initial (incorrect) assumptions. I will not respond to further comments about this now. It has proven pretty fruitless and I once again suggest you brush up on SOLID principles.

[David Wickes]

Good article - thanks for sharing it!

Although this class works it is impossible to unit test due to two different dependencies being instantiated rather than injected.

This is a great way to spot tightly coupled code that is just aching for a bit of DI. If you can see an object getting newed up inside a method... inject it instead! 💯

First of all we use Mockery to mock our account class.

I tend to eschew the use of mocking libraries and just roll my own test doubles. It stops people arguing with me about which one to use... 🤣

How to write testable code

Of course, the best way to write testable code - to absolutely guarantee that it's testable - is to write the tests first. 😉

[Daniel]

Glad you liked the post !

Yup - if I see ‘new’ anywhere in a PR you can bet I’ll be investigating to see if it should be refactored (it almost always should)

Maybe I’ll do a TDD one in the future :)

[Arnas Kazlauskas]

Why Mockery and not let's say Prophecy?

[Daniel]

The mock library used is not what is important in the article. If I had used Prophecy someone may very well have commented saying 'Why not Mockery' ?

[Theodore R. Smith]

Instead of Mockery/Prophet, why do people not use anonymous classes? Oh, that's right. The vast majority of PHP developers have never bothered to learn them.

Take this:

   $account = Mockery::Mock(Account::class);
   $account->shouldReceive('setState')
       ->with('active')
       ->andReturn($account)
       ->once();
   $account->shouldReceive('save')
       ->andReturn($account)
       ->once();

It can be rewritten into this:

$account = new class extends Account {
    public function setState(string $state): self
    {
        return $this;
    }

    public function save(): self
    {
        return $this;
    }
};

These are largely the same. You can, if you need to, ensure that each of those functions are called only once (use static $runCount), then they would be largely identical.

[Daniel]

I use a mocking framework because it is consistent with what other developers I work alongside will use.

I would have no problems using an anonymous class but I don't see the benefit it offers over a mature, well tested and known library.

[Arnas Kazlauskas]

I get it, no need to be defensive.
Don't get me wrong, I'm not saying that this is a bad choice.
I'm just curious why do you use Mockery as opposed to something that comes out of a box with PHPUnit. I've never tried Mockery so I'm eager to know its advantages.

[Daniel]

My apologies if it came across as defensive. It was not my intention. I have just used Mockery for such a long time I am used to it now.

I have never used Prophecy but taking a look at the differences there appear to be some subtle ones.

I will give it a try on my next project :) Thanks