Are too many assertions code smell?

Question

I've really fallen in love with unit testing and TDD - I am test infected.

However, unit testing is normally used for public methods. Sometimes though I do have to test some assumptions-assertions in private methods too, because some of them are "dangerous" and refactoring can't help further. (I know, testing frameworks allow testing private methods).

So it became a habit of mine that the first and the last line of a private method are both assertions.

However, I've noticed that I tend to use assertions in public methods (as well as the private) just "to be sure". Could this be "testing duplication" since the public method assumptions are tested from the outside by the unit testing framework?

Could someone think of too many assertions as a code smell?

Answer 1

Those assertions are really useful for testing your assumptions, but they also serve another really important purpose: documentation. Any reader of a public method can read the asserts to quickly determine the pre and post conditions, without having to look at the test suite. For this reason, I recommend you keep those asserts for documentation reasons, rather than testing reasons. Technically you are duplicating the assertions, but they serve two different purposes and are very useful in both.

Keeping them as asserts is better than simply using comments, because they actively check assumptions whenever they are run.

Answer 2

It sounds like you are trying to do Design-by-Contract by hand.

Doing DbC is a good idea, but you should at least consider switching to a language which does support it natively (such as Eiffel) or at least use a contract framework for your platform (e.g. Microsoft Code Contracts for .NET is pretty nice, arguably the most sophisticated contract framework out there, even more powerful than Eiffel). That way, you can better leverage the power of contracts. E.g. using an existing framework, you can surface the contracts in your documentation or an IDE (e.g. Code Contracts for .NET are shown in IntelliSense and, if you have VS Ultimate, can even be checked statically by an automatic theorem prover at compile time while you type; likewise many Java contract frameworks have JavaDoc doclets that will automatically extract the contracts into your JavaDoc API documentation).

And even if it turns out that in your situation there is no alternative to doing it manually, you now at least know what it is called, and can read up about it.

So, in short: if you are indeed doing DbC, even if you don't know it, then those assertions are perfectly fine.

Answer 3

Whenever you do not have full control over your input parameters, it is a very good idea to test up front for simple errors. Fail on null for instance.

This is not duplication of your tests, as they should test that the code fails appropriately given bad input parameters, and then document that.

I do not think you should assert on the return parameters (unless you explicitly have an invariant you want the reader to understand). This is also the job of the unittests.

Personally I do not like the assertstatement in Java, since they can be turned off and then it is a false security.

Answer 4

I think that using assertions in public methods is even more important, since there you do not control the inputs and may be more likely to have an assumption broken.

Testing input conditions should be done in all public and protected methods. If the inputs are passed directly to a private method, then testing its inputs may be redundant.

Testing output conditions (e.g. object state or that return value != null) should be done in the inner methods (e.g. private methods). If the outputs are passed directly from a private method to the output of a public method without additional calculations or inner state changes, then testing the output conditions of the public method may be redundant.

I do agree with Oleksi, however, that the redundancy may increase readability and it also may increase maintainability (if direct assignment or returning is no longer the case in the future).

Answer 5

It's hard to be language-agnostic about this issue, as the details of how assertions and 'proper' error/exception handling are implemented have a bearing on the answer. Here's my $0.02, based on my knowledge of Java & C++.

Assertions in private methods are a good thing, assuming you don't go overboard and put them everywhere. If you're putting them on really simple methods or repeatedly checking things like immutable fields, then you're cluttering up the code needlessly.

Assertions in public methods are generally best avoided. You should still be doing things like checking that the method contract isn't violated, but if it is then you ought to be throwing suitably-typed exceptions with meaningful messages, reverting state where possible, etc. (what @rwong calls "the full proper error handling treatment").

Generally speaking, you should only be using assertions to help your development/debugging. You can't assume that whoever uses your API will even have assertions enabled when they use your code. Though they do have some use in helping document the code, there are often better ways to document the same things (i.e. method documentation, exceptions).

Answer 6

Adding to the list of (mostly) exceptional answers, another reason for lots of asserts, and duplication, is that you don't know how, when or by whom the class will be modified over it's life time. If you are writing throw away code that will be dead in a year, not a concern. If you are writing code that will be in use in 20+ years, it will look quite different - and a assumption you have made may no longer be valid. The guy who makes that change will thank you for the asserts.

Also not all programmers are perfect - again, the asserts mean that one of those "slip ups" won't propagate too far.

Do not underestimate the effect these asserts (and other checks about assumptions) will have in reducing the cost of maintenance.

Answer 7

Having duplicate asserts is not wrong per se, but having the asserts "just to make sure" isn't the best. It really boils down to what exactly each test is trying to accomplish. Only assert what is absolutely needed. If a test uses Moq to verify a method is invoked, it doesn't really matter what happens after that method is invoked, the test is only concerned with making sure the method is invoked.

If every single unit test has the same set of asserts, except for one or two, then when you refactor a bit all of the tests could fail for the exact same reason, instead of showing you the true impact of the refactor. You could end up in a situation where you run all the tests, they all fail because every test has the same asserts, you fix that failure, run the tests again, they fail for another reason, you fix that failure. Repeat until done.

Also consider the maintenance of your test project. What happens when you add a new parameter, or the output is tweaked every so slightly, would you have to go back through a bunch of tests and change an assert or add a assert? And, depending on your code, you could end up having to setup a lot more just to make sure all the asserts pass.

I can understand the allure of wanting to include duplicate asserts in the unit test, but it really is overkill. I went down the same path with my first test project. I went away from it because the maintenance alone was driving me nuts.

Answer 8

However, unit testing is used for public methods.

If your testing framework allows for accessors, then unit testing private methods is also a good idea (IMO).

Could someone think of too many assertions as a code smell?

I do. Assertions are okay, but your first goal should be making it so that the scenario isn't even possible; not just that it doesn't happen.

Answer 9

It's bad practice.

You shouldn't test public/private methods. You should test the class as a whole. Just make sure you have good coverage.

If you go the (primitive) way of testing each method separately as a rule of thumb, you'll find it extremely hard to refactor your class in the future. And that's one of the best things TDD enables you to do.

Besides, it is duplication. Furthermore, it suggests that the writer of the code didn't really know what he was doing. And the funny thing is, you do.

Some people treat their tests with less care than their production code. They shouldn't. If anything, my experience suggests to even treat the tests with more care. They're worth it.