Test All The Things!

Over the past few months, my spare time at work has been spent combing through the unit tests for an Angular application we have written, shipped, and iterated over for the past 18 months or so.

Our coverage was lower than I’d like, but more importantly, the tests that were in place weren’t able to be trusted, and often very fragile.

I needed to hand over this project to a team new to JavaScript, so I felt it was a worthwhile endeavour to change our test suite to be more internally consistent. I also wanted to make it easier to understand why a test failed rather than relying on assumed knowledge of JavaScript equality, Angular quirks and so on.

Here I will go through things that I refactored in our test suite, and explain my rationale for doing them.

Note: I’m using Jasmine 1.3 syntax for these demos, hopefully it’s still easy enough to follow, and I think that the guidance should transfer over to other assertion frameworks quite well.

One expectation per ‘it’ statement.

Before my big refactoring job, there would be single ‘it’ statements like this at the start of a controller’s test suite:

This is not great because if any of these scope values are not equal to what we expect then we won’t necessarily be able to figure out exactly what has broken, because our output would only tell us:

checking scope values FAILED Expected undefined to be false. Expected undefined to be true. TypeError: ‘undefined’ is not a function (evaluating ‘_scope.qux()’)

This isn’t tremendously helpful, and when that “checking scope values” test starts growing and growing, a single failing expectation starts feeling like trying to find a needle in a haystack.

My first proposal for testing your web application is rigidly, strictly, and with all the passion you can muster, enforce one expectation per test.

The previous example would look something more like this, once the single responsibility principle had been applied:

I know what you’re thinking…

“Why on earth would you change 6 lines into 55?”

I think that it’s important to set up explicit tests for anything you rely on in your test suite. I also think that anything that could potentially be undefined should be tested for definition as a matter of completeness, but that’s definitely an arguable case.

We could test that _scope.baz.fred was equal to 3 without any of the set up, but when we’re testing an object 7 levels deep and a property doesn’t exist after 3, I want a test to fail before it gets to that point.

‘Describe’ is for state or parent objects

I found that in order to keep to a standard that I could always apply, I had to make the rules that I followed simple. The next one I want to discuss is my use of ‘describe’ blocks.

This does mean that your test suite will wind up looking much more nested than otherwise, but, with code folding, and carefully scoped tests (which I’ll come onto later), I feel like it makes the tests purpose and scope a lot easier.

I found that stopping whenever I wrote “when” into the description of an ‘it’ statement and moving it into a ‘describe’ block made the expectations I wrote a lot easier to reason about. I think the ‘it’ is basically your opportunity to pretty print the intent of your expectation, and everything else should be covered by ‘describe’ statements and closures.

When combined with one expectation per ‘it’ statement, narrowing your ‘describe’ statements to either a single state change, or storing the parent object’s tests, means that your tests start to follow a much more logical, repeatable pattern.

This was really important to me because I really wanted the tests to be easy to debug if they failed, and wanted good practices to keep being followed after I’d handed the project over.

A ‘describe’ statement is the right choice if:

• You have a new state to test, (the obvious first one being ‘on load’, for things that change when the controller is executed for the first time).
• You have a new object or function to test.

An ‘it’ statement will be okay if:

• Your previous expectation requires the same assumptions be made, and you are testing the same object or function as in that expectation.

Bad example

Better example

I think that it’s important to handle each state change that we could go through within our unit testing. Ensuring here that _scope.qux is undefined when the promise rejects is potentially “over-testing” but I’d prefer to cover the eventuality with a test just to confirm that my thoughts about the code are correct.

Finally, a describe statement should only describe one state change. Each time you need to perform an action, whether that’s resolving a promise, or calling a service etc, you should use a ‘beforeEach’. It’s important to ensure that work being done in a ‘beforeEach’ statement should be explicitly described, as you can see in the above example, so that any future new functionality can be tested. If _scope.bar() needs to do more later, we have points to test it before or after the promise resolution from the service call.

Don’t repeat scopes

As I went through our unit tests, I found that, in a similar way to our controllers, copy and paste coding meant that finding one mention of a variable or function wasn’t necessarily a sure-fire way of being near all the times that the variable or function was set or changed. This is ostensibly a sign of a large code base where people aren’t entirely sure what effects their changes would have if, instead of just adding new functions, they used the functions already there but changed them slightly to fit.

This meant that sometimes we’d wind up coming back to the same object or function, and we’d check different properties or outcomes in different places.

With the previous ideas already in place, refactoring them was a bit easier, and the rules for when to use a ‘describe’ statement also go a long way to ensuring this rule isn’t broken.

By repeating scope you increase the fragility of your tests, and people may hit the first mention of the object you’ve changed, modify those tests, and not modify other tests to keep them up to date. This can lead to people who don’t trust their tests, and their failure becomes just part and parcel of their existence, because no-one’s taken an interest in diagnosing the problems, and fixing them.

At the earliest point in your tests that you can test every line of code in your controller, you should do it, ideally in the same order that the controller does it. You’ll need to write some non-test related code in order to set up your environment, mock your services, inject your controller etc, but once you have the minimum state required to explore your functions in depth, test all of their functionality.

There are some cases where that does not work, for example when functions rely on state being passed into the controller, so reloading the controller with a different state is required. Those should be kept to a minimum however, as they couple controllers tightly together, and it can cause issues.

Leave expected values out of ‘it’ statements

One thing that you may notice when you look at my tests is that I only really use the ‘describe’ statements to give context to my ‘it’ statements. My ‘it’ statements are, as I said before, just a nice reminder of the expectation, and the context should be given by the describe statements that they are part of.

Some people are tempted to write tests that look like this:

This one (should) be less contentious than my other thoughts on testing, but I basically think that we should be explicit in what we’re testing, but the console output on failure is there to help us.

With the following test setup, we get help from the test output when the expectation fails, and only ever need to change the value we have in our tests in one place.

Conclusions

I’m sure that there are more optimisations that could be made to a large test suite. I opted for these because I had some time constraints, and I felt that I could accomplish all of these changes within that time-frame.

I think that anyone new to Jasmine could benefit from following these instructions, and it’s helped me a lot when debugging, and going through with such granularity has helped me discover bugs that we’d never hit before, and simplified a lot of the logic in our controllers.

In closing, please let me know if you agree or disagree with anything in here, I’m still new to the whole dev scene, and I always accept feedback.