Recently, when I attended Adopt OpenJDK test fest,
I found good examples of test writing cases how NOT to do it. After that I decided that I would like to write a
simple guide about how to write maintainable tests.
If you haven’t been writing tests already… well let’s just say that you should do so.
I am pretty sure that there are lotsofliteratureaboutthat topic. One thing that beginner test writers ignore is that they should pay the same
attention and importance to tests as the actual code that tests are supposed
to check. By not following this simple advice, you will get into cases when
understanding test is almost impossible and changes required to be made in
code are very hard to reflect in test. As an example, I am going to show
this simple test from OpenJDK which is
supposed to check thread pools’ functionality:
We can clearly see that this is an old school test. I think it was actually written before any of JUnit or TestNG frameworks were present. Lack of testing framework has greatly shaped this test being unreadable. Lets focus on the test itself. It’s “test(String)” method. There are several problems with that code but main one is that we don’t know what the code is supposed to test. From the test class name we could guess that it is somehow related to core thread time out functionality. Also, this test has a bug in it and this is the reason why I initially picked it up.
In test there are main three stages: prepare thing you are going to test, execute code, make assertions on the ending state. There is one optional stage that depends on the thing you are testing - it’s “cleanup”. There are situations where you may want to release resources or restore environment state (though this would be more likely an integration test). In the example above there is no clear separation between the three so we should introduce that. Now code could look something like this:
Notice that I also moved constants out of test body to reduce “noise”. Furthermore, in ThreadPool preparation code I removed irrelevant checks – those checks tested whether we actually were setting pool in correct state which we did so explicitly. If our preparator won’t work in correct way the test itself will fail (of course you have to be aware of false positives). After separating state preparation it would be sensible to remove legacy testing code and transition to TestNG. The reason I am using TestNG is that the OpenJDK project is using TestNG as the preferred framework, but in reality it doesn’t matter which one you choose. Change itself is quite trivial:
It makes sense to extract preparation and cleanup stages into @BeforeMethod and @AfterMethod handlers to ease code reuse and clarify structure:
Utilizing framework functionality lets us focus on the test itself instead of how to track failures and display them – now that is being handled by TestNG. Also, after these changes we can see what does the test actually test – thus we rename our test method to expose that knowledge explicitly to “shouldRemoveThreadsAfterCoreThreadTimeoutPeriod”. This step is very important because having readable, easily understandable test intention makes our test act as a documentation. Naming style here isn’t really important while it is readable. I am quite used to camelCasing but_you_can_use_other styles, too.
Paying closer attention to the test we can see where the problem resides – after executing threads we are checking for the amount of threads active that we have started but that isn’t necessarily the case because some threads could have finished before we make that assertion. Also, that check isn’t really relevant to our test – it checks data set up and not state after execution. Furthermore, we can see that this „timeOut“ waiting isn’t effective either – on some machines, configurations or loads it could timeout before threads complete. In this case I would introduce CyclicBarrier and wait until all threads end. And we should do this in readable way so maintainer would understand what is going on. In the end this test looks like this:
We have almost fixed the test. The remaining problem is that we only check thread timeout case. What about the case if we aren’t expiring threads? That should be checked also if we would like to avoid false positives. Adding this case would do that:
But we still have uncovered test case – what if we would like them to expire but only after our specified timeout? This code should do that:
It’s worth noting that I have hidden all „noise“ in private methods at the bottom of the test file. This approach hides helper code from the eyes but it is still easily accessible. I also like private method approach because then if I see duplication I can move that method to separate class that is responsible for that concept, make that method as static and use static imports to easily fix compilation problems. Of course this would only work if you were using Java. In other languages you may have to take another approach but the main concept holds – extract, refactor, reuse.
We covered test transformation from being unreadable and thus unmaintainable to test which describes our functionality. Before that we needed to actually check JavaDoc to understand how that feature behaves but now test itself describes intended functionality so in case something breaks you have documentation straight before your eyes.
In conclusion I would like to emphasize a few points which make tests maintainable:
Test are of same importance as the code it tests. Tests are valuable asset that enable change and confidence in your system. Thus all clean code concepts such as DRY should still be followed here. Extract common mocks or stubs. Extract preparation code as builders where appropriate.
Separate tests in three stages: prepare objects for testing, execute logic and check state.
Make variables and methods self describable – it should be clear from first sight why they are there. Use your test as documentation.
All “noise” (code that is irrelevant to test itself) should be hidden. Hide that in private methods or another test objects.
Don’t make tests complicated – no “for” or “while” loops, no nested checks – all execution and checks should happen in the same method and in plain, linear fashion. This helps to keep flow of the test under control and makes it a lot more understandable
Test Driven Development – classic book. Most of the concepts are now accepted as the de facto standard, but at the time when it was released it was a groundbreaker. Since this book is really short, it is probably still worth reading even if you are familiar with testing already.