TDD extremists

The other day I came across a particularly abusive post about TDD. Here’s a quote:

If you don’t use TDD in your project you are either lazy or you simply don’t know how TDD works. Excuses about lack of time don’t apply here.

I’ve been seeing this type of attitude for a while now but this one had some code attached so I decided to rant about it. First of all tests have costs, maybe the trade-off is worth it but it’s important to actually acknowledge that it’s happening in the first place.

1) Spending your limited time structuring your workflow around tests could be better spent around architecture and design of the overall code base.
2) In a poorly architected code base test give a false sense of security.
3) Most interesting use cases that you actually want to test are not amenable to testing.
4) Tests mean you have extra code that you need to maintain.
5) Writing tests manually is not particularly efficient.

The purpose of good design is to decrease the surface area of possible mistakes by construction of the architecture. What does this mean in practice?

Here’s the original class from the post:

1 & 2 & 3) Architecture/Design

There’s quite a few things here right away that I think could be improved. First this class is mutable. Someone coming from a functional language or having read “Effective Java” or “Java Concurrency in Practice” will pick up on this right away. Immutability is often a design choice. Do you want to write tests that try to verify the behavior of this class in a concurrent environment? Do you really want to accept a double precision float for money? An arguably better approach is to structure this class so that these problems don’t occur in the first place. Here’s a rewritten version:

Is immutability harder in a language like Java than Clojure or Scala or Kotlin? Yes, but it sure saves a lot of work in the amount tests you have to write. I don’t have to worry about this class not doing the right thing with money because of weird rounding issues/floats. If you want to learn about floating point arithmetic go for it. I prefer to use Joda Money or in this case for the sake of an example I am using BigRational. You don’t have to worry about someone else extending this class and breaking encapsulation. No need to test if synchronization is done correctly. All of these things are extremely hard to unit test to the point where almost nobody does it. However if you think about the architecture of your code before writing tests perhaps you can avoid writing these tests in the first place. I’d argue the easiest things to test are quite simple and don’t end up saving much time. The real hard bugs are in the later category and it’s best to get rid of them via good architecture that doesn’t allow them to exist vs dozens of unit tests that will try to achieve the same thing.

4) Unit tests are things you have to maintain. I think most people with large code bases have experienced this. I have a 2 line change in the code that causes 20 tests to be changed. It becomes very difficult to change the architecture in a large codebase because of all the tests but the tests don’t necessarily improve or imply good architecture and design. It could be argued that tests failing when code is changed is a good thing but it’s worth acknowledging the costs associated with that kind of a workflow. In the above example I hope it’s clear that even in a simple “POJO” class where there isn’t much going on design is important.

5) It’s difficult to test interactions via unit tests, even in simple cases. Wouldn’t it be better to have your code come up with hundreds or even thousands of tests for you? With the invention of quickcheck and the porting of it to most mainstream languages it’s now possible. This can drastically decrease the amount of unit tests you have to write in the first place and I find it more convenient to write those type of tests after the code is designed and written rather than writing tests first before I have a design.

The worst part for me is that the author is probably aware of all of this since he writes Scala. The above is nothing more than a verbose version of a Scala’s case class or Kotlin’s data class.

Dangers of unit testing undefined behavior

Recently I participated in a intracompany discussion about a concurrency defect. A piece of code looked something like this:

Thread1:

Thread2:

The problem? ‘keepRunning’ a plain boolean without any synchronization. According to the JMM a whole slew of things could go wrong with the above code, one of which is for Thread1 to run forever because it never sees Thread2’s update to ‘keepRunning’.

What’s interesting about this is that there was an integration test that inadvertently tested this scenario and has always passed, so the problem was never caught. Once the code started running a production box which has different hardware characteristics (a lot more cores/memory) this code blew up.

This is one of those examples where unit testing doesn’t produce good results. It’s very dangerous to get an intuition about incorrect concurrent code by running simple unit tests. These one off unit tests run for a short period of time on a box that potentially has few cores, the JIT doesn’t kick in, inlining doesn’t happen the system isn’t under heavy load and the hardware configuration could be favorable to not surfacing the error. As shown in this post (C and Java examples), just because incorrect concurrent code works the way the author expects doesn’t mean it will continue to work. I’m not sure if that was the author’s intention but that’s what I got out of it.

This is why I am pessimistic about types and unit tests when it comes to catching interesting errors found in production. Unit tests/types are good for catching obvious things like “this method doesn’t accept arguments of this category” or “what happens when this method gets passed an empty string instead of a string that I expect?”. I have yet to see a language/test framework that can help with concurrency problems.

I know of are two partial solutions to the concurrency problem.

1) Try to avoid errors by construction, i.e. have good design that makes doing the wrong thing harder. Immutable data structures by default is a big first step in that direction.

2) Feynman method. Think really hard and write code that doesn’t contain concurrency bugs, if that’s not possible try to convince a friend or co-worker to think very hard with you.

The first method is really just a special case of the the Feynman method.

CLJ-DS is worth checking out

I like Clojure. I really like it. I think it’s the best language available on the JVM. Why would anyone want to use Clojure? One word: Concurrency. Clojure is largely designed to address the needs of dealing with concurrency without resorting to primitive constructs such as locks.

Unfortunately using Clojure on projects isn’t always a feasible option because many projects are locked into the existing Java paradigm technically, culturally and economically. I often end up writing concurrent code in Java so having good data structures that minimize locking is a must. I find that most code I inherit that uses a lot of “synchronized” can often be rewritten to drastically cut down on the number of locks thanks to java.util.concurrent and java.util.concurrent.atomic. The part I always missed was an immutable data structure that could be returned to the calling code. This could be achieved by returning defensive copies of every mutable data structure but there is a slicker way.

CLJ-DS is another solution to the problem. It’s a library of Clojure’s persistent data structures ported back to Java with nice Generic type signatures and convenience static methods.

Here’s a typical example of code I often inherit. All the business logic and business variable names have been removed.

Some obvious problems with this code?

  • Since ‘getElements’ returns a mutable set, there is no guarantee that some code outside of ‘FooManager’ won’t ‘.clear()’ or mutate the returned set any further.
  • This code has subtle differences depending on ‘uuid’ existing in idToSet. When ‘results’ is null there might be an expectation of the empty set to be referenced by ‘idToSet’ just as it is in the non-null case.
  • Once the calling code gets a handle on the synchronized ‘results’ from ‘getElements’ it’s not guaranteed that everything is safe since ‘addElement’ uses a different lock to writes to the set in the non-null case.

There’s a better way using CLJ-DS and java’s concurrent package:

No subtle mutations of state and a lot fewer locks and by definition less lock contention. I consider the revised version a lot easier to reason about in no small part because of CLJ-DS library. PersistentMap and PersistentSet implement java.util.Map and java.util.Set respectively.