22

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?

5
  • 1
    are these assertions on or off in release?
    – jk.
    Jun 19, 2012 at 8:32
  • I work primarily with Java where assertions have to be enabled manually.
    – Flo
    Jun 19, 2012 at 15:32
  • Just found this "Asserts boost productivity by tracking down bugs" programmers.stackexchange.com/a/16317/32342
    – Flo
    Jun 19, 2012 at 16:18
  • how do you define "too many" ?
    – haylem
    Jun 25, 2012 at 19:12
  • @haylem There are "too many" assertions if another programmer reads the code and says "I'm tired of these assertions".
    – Flo
    Jun 26, 2012 at 13:54

9 Answers 9

28

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.

3
  • 2
    Assertions are documentation, but that doesn't make duplicating them legitimate. On the contrary, it even raises suspicions about the quality of the tests if it seems like the same assertions are needed more than once. Jun 19, 2012 at 0:26
  • 1
    @YamMarcovic I think this is acceptable because the two "duplicated" pieces of code serve two different, and distinct purposes. I think it is useful to have them in both locations, despite the duplication. Having the assertions in the method is invaluable for documentation, and they are obviously required for testing.
    – Oleksi
    Jun 19, 2012 at 0:41
  • 2
    Assertions in code for me are complementary to the unit testing assertions. You won't have 100% test coverage and the assertions in code will help you to narrow down failing unit tests faster. Jun 19, 2012 at 5:46
16

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.

6

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.

0
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).

2
  • 4
    If an error condition or invalid argument condition can be caused by the user (caller) of the public interface, this should be given the full proper error-handling treatment. This means formatting an error message that is meaningful for the end-user, accompanied with error code, logging, and attempts to recover data or restore to a sane state. Substituting proper error handling with assertions will make the code less robust and harder to troubleshoot.
    – rwong
    Jun 19, 2012 at 4:53
  • The assertions could (and in many cases should) include logging and throwing exceptions (or error codes in C--). Jun 19, 2012 at 6:58
4

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).

2

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.

1
  • I have asserts like "If a matcher as a parent, that parent has child". It sounds stupid, but it will indeed sounds much less stupid when it will pop in the console due to a programmer error. It's indeed very useful. On top of that, it documents invariant and assumptions, which can be broken when refactoring. So, it makes refactoring much easier too. It's also zero cost, since it's removed for production builds. It's so sad that it's under used! Aug 4, 2022 at 7:43
0

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.

0

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.

-2

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.

3
  • -1 Unit testing a class as a whole can be dangerous since you end up testing more than one thing per test. This makes really brittle tests. You should be testing one piece of behaviour at once, which often corresponds to testing just one method per test.
    – Oleksi
    Jun 19, 2012 at 0:47
  • Also in regards to "it suggests that the writer of the code didn't really know what he was doing [...] you do.". Future developers might not be as clear on what a particular method does. In this case, having pre and post conditions in the form of asserts can be invaluable in understanding a piece of code.
    – Oleksi
    Jun 19, 2012 at 0:48
  • 2
    @Oleksi Good testing is about valid usage scenarios, not about asserting every smallest irrelevant detail. Redundant asserts are indeed a code smell because the obscure intent, and are just another bad example of defensive programming. Jun 19, 2012 at 8:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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