Here is a typical example of a test method
@Test
public void should_search_by_path() {
Searcher searcher = new Searcher();
Path location = new Path("somewhere");
String data = "data";
searcher.putAt(data, location);
assertThat(searcher.findAt(location), is(data));
}
It seems that many developers consider this good code. I don’t. My main gripe here is that the presence of variables does not add much useful information. Instead, they make the code too verbose.
A first rewrite would produce something like that:
@Test
public void should_search_by_path() {
Searcher searcher = new Searcher();
searcher.putAt("data", new Path("somewhere"));
assertThat(searcher.findAt(new Path("somewhere")), is("data"));
}
Shorter, which is good. I also like the fact it makes clear that it is the value of the parameters that matter, and not their pointers.
We can go further by leveraging local static methods.
@Test
public void should_search_by_path() {
Searcher searcher = new Searcher();
searcher.putAt("data", path("somewhere"));
assertThat(searcher.findAt(path("somewhere")), is("data"));
}
private static Path path(String path) {
return new Path(path);
}
(a static method is not mandatory, but I think it makes its intention of being a utility method clearer)
And, finally, if this test class is dedicated to the Searcher class, I would probably make the searcher variable a field, as it would be used in most test methods.
private Searcher searcher = new Searcher();
@Test
public void should_search_by_path() {
searcher.putAt("data", path("somewhere"));
assertThat(searcher.findAt(path("somewhere")), is("data"));
}
private static Path path(String path) {
return new Path(path);
}
This is more lines than the original example. However, the searcher variable is on top of the class and the path() method is at the bottom, far out of the way of the test methods. Indeed, I would often have several such static methods at the bottom of my classes (and, rarely, one or two more instance variables at the top). I even tend to do so when I have such a single method using them.
Beside the fact that the method has fewer lines, I also like that it makes for more fluent code. It almost reads like a story “when the searcher is being put a String of value ‘data’ at the path of name ‘somewhere’, then it should be able to assert that the String found at the path of name ‘somewhere’ has value ‘data'”.
Another example, so that my position is clear:
@Test
public void should_find_by_address() {
String aValidAddress = "10 Downing Street";
Person aPerson = new Person("David Cameron");
contacts.put(aValidAddress, aPerson);
assertThat(contacts.get(aValidAddress), is(aPerson));
}
I’d very much rewrite this as follows:
@Test
public void should_be_able_to_find__a_person_when_storing_with_a_valid_address() {
contacts.put("10 Downing Street", person("David Cameron"));
assertThat(contacts.get("10 Downing Street"), is(person("David Cameron")));
}
Any thoughts?
Nice, I like how you structure your tests and make them concise and readable.
I see two assumptions and would like to check if I picked them up right. First it seems to me that your approach of exposing the class under test as a fieldsis only valid if its methods under test are functional in nature (have no side effects and the class is stateless). Second that you use types which are strictly value types and have relevant equality operators well defined.
@Marcin Floryan First question: in fact, the methods under test tend to be stateless services. Not strictly functional, but close.
I think I also sometimes do test methods that have side effect. In those cases, it should still work OK, since JUnit would re-instanciate the instance of the class under test for each test.
Second question: yes, the types that are passed tend to be value types. They tend to be immutable and their equals() methods are obvious (and usually implemented with org.apache.commons.lang.builder.EqualsBuilder.reflectionEquals(this, obj)).
Sometimes, there are good reasons for not having a properly implemented equals(). In those cases, I have a separate Matcher that does a deep equal.
Thing I’ll take from it is the idea of making utility methods in tests static. I actually envy that I haven’t thought of it myself. :-) ;-) Nifty indeed.
I respectfully disagree with shying away from variables though.
1) It’s cool if your String is just “data”, or nice well known name like “David Cameron”, but *usually* it can be something like “Nityusauskas Konighiristas” or “Jose Mariachi Alberto sol Vigneri del Parma”. Good luck with writing that twice or thrice in your test and NOT making ANY silly typo or other mistake. Replacing literals with variable is just for that: compiler help and autocompletion, leading to less mistakes and better flow. With my variable, I don’t care if String is short or long, easy or hard to write well. I start with three letters, hit Ctrl+Space and start writing “); And there won’t be any mistake. And I won’t have to be extra cautious (slows one down), and select the name and copy it, then select where you want to paste it and paste it (slows one down, disrupts flow).
You may find yourself surprised how many times I looked at such bug (mistyped String, not necessarily mine) and NOT noticed it till I searched for a bug with debug. People trust what is written. They tend to skim past the String usually assuming it’s exactly the same as all the previous ones. So, if someone made mistake, it often will be missed, even in pair-programnming it may be – I’ve seen it happen.
Oh, and of course that example is for name. What if your String is something else? What if it is a lengthy number (stored as String for some odd but valid reason or stored as Integer, and tested as Integer)? Or a question text from a survey?
2) Variables have names and these names are generic. “Name” means Cecile OR Eric OR Ritishmanu OR Sasza OR Jukka OR … On the other hand, Cecile means just Cecile. Eric means just Eric. Etc. This has two implications.
a) You don’t focus on single, detailed cases, you focus on cases. Your example IMO did not test finding longest user name, but rather whether Eric or Cecile is longer username. Having variables/parameters makes it easier to push different tests data when you wish, by tweaking test setup or changing what utility method returns, or adding a factory method or (with params) simply parametrizing test. On that occasion let me ask you, how will you make a parametrized test with test data in it being literals? Not that you HAVE TO. You don’t. It’s quite fine to have literals, but I feel this technique is limited.
b) If I see variable named creditCardNumber, I know what is being tested (unless someone botched it and named phoneNumber wrong – happens). If I see that what is tested is 12387654534876453 than I know nothing.
This is taken even farther by some folks from TDD, like Corey Haines or Alex Balboa – these guys dislike primitives, saying they don’t know what these are. Seeing “data” is very bad, seeing String data = “data” would be only somewhat better, but still not good. :-) That is because seeing int phoneNr tells you little, when compared to working with objects. Not that I fully agree, but just letting you know.
3) Variables as storage are fantastic for difficult methods output.
It’s cool if your method for path is two or five lines long. But when it becomes more complex, has a for loop, reorders or sorts a collection, gets data from persistence layer (likely in integration tests)? Why would you call it twice? You can store it’s result in a variable and NOT waste time on computations again.
Findbugs considers iterating over HashMap with Keyset only, while modifying values, wasteful. And that is much smaller performance loss when calling some methods twice.
Also, some methods have long names. Say, boolean renamePerformanceProgramWithCascadingNoCache().
Now, with variables I can make boolean wasRenamed and use it in WHEN part of the test and in ASSERT as well. In your case, it would be a rather lengthy line. How that helps code readability, we both know.
Just my two cents about what I found lacking in your post.
Fantastic article. I’m glad I stumbled across your blog.
Just a minor point – whether “searcher” is a field or a local variable (and like you, I prefer it to be a field), there’s no point in having a variable name which is just the same as the name of its type (minus the initial capital) – you may as well just call it x.
In my opinion, a variable name should reflect what that variable is for. This really helps to understand code when you come back to it several months later. This idea applies to all code, of course; but in the case of unit tests, if the variable references the object to test, I prefer to call it toTest. Then, if you need two Searcher objects for any reason (maybe you’re testing equals() or compareTo()), then the second one could be toCompare.
private Searcher toTest = new Searcher( “some value” );
private Searcher toCompare = new Searcher( “some other value” );
@David I agree with you in principle, but in practice I don’t do that in my tests.
In the example you give, besides the point that I’d rather inline the variables and get rid of the variable names altogether, I’d probably name then like this:
private Searcher searcherWithSomeValue = new Searcher( “some value” );
private Searcher searcherWithSomeOtherValue = new Searcher( “some other value” );
I think this is mostly because I want my tests to read like little stories:
assertThat(searcherWithSomeValue, not(is(searcherWithSomeOtherValue)));
I think the following feels less natural:
assertThat(toTest, not(is(toCompare)));
However, in my production code, I would probably use names similar to the ones you suggest.