Jake Herringbone

Making the case for unit tests to live in the code they test

This is perhaps the most controversial idea in Noop. I had an interesting discussion that included Cédric Beust, in which I defended this idea from a lot of doubts, mostly around why it would be needed, and the challenges with tools that expect a conventional code layout. So, I went to spec out exactly how it could work. And I’m happy to report that I think it totally works.

First, what’s wrong with writing unit tests the conventional way?

• Create a separate src-test or src/test source root, which is to contain just tests, so that we can compile the production code independently and enforce that prod code doesn’t have deps on tests.
• Create a new source file in that source root, in a package/namespace mirroring the location of the prod file you want to test.
• Name the new class with a convention like “*Test” so our team understands where to find the unit tests for a file.
• In the test fixture, create an instance of the class-under-test, supplying fakes or mocks for some of the constructor/setter dependencies as needed.
• Start calling methods in the class and sensing whether they did the right thing. Modify the code under test to make some fields or methods package-protected or friendly to the test class so it can white-box test where you need to.
• Optionally mark that change with a comment or annotation like @VisibleForTesting so your team can understand why this field or method isn’t private as they’d expect.
• Get the tests green, and send off your code review. The reviewer needs to flip between the test file and the production file, so they can use the test-as-documentation to understand what you intend the code to do, and help you find corner cases you missed in your test.

Here’s what’s wrong.

• You want to change code in the prod class with the corresponding unit test visible. Maybe you have an IDE plugin like TestDox that can navigate between the two classes based on the naming convention. If not, you have to navigate manually, and maybe change your window layout to show the two side-by-side.
• Even when you can see both prod and test code, you have to scroll around the two files to see the tests that cover the method you’re working on.
• You refactor the prod class, changing the name or package, and the test class now is misnamed or in the wrong place. Maybe the package-protected access breaks so the compiler tells you, or maybe your code reviewer notices. But your refactoring tool probably doesn’t help you, and it’s easy to check in this mistake.
• You changed the production code just to expose some fields for testing. You really want the test to have special access into the class internals that wouldn’t be allowed at production runtime, but the language doesn’t distinguish these two runtimes.
• As the code grows, you refactor the class by extracting some methods and pulling out some responsibilities into their own classes. You have to read through all the tests to understand what behavior should be extracted into a new test. Some tests are no longer unit tests, since they test behavior that’s now an interaction between the original class and the new class, so you might move those tests to a third location.
• It’s really annoying in the code review to correlate the test and prod changes, especially if your code review tool has a high latency when navigating between files.
• The test needs to be written as a class with methods, even though you should never create an instance of that class yourself, nor call any of the methods yourself.
• The test provides fantastic documentation, because it’s executable. The test always tells you the truth, even when comments get outdated. Sadly, that documentation is not found in your generated docs, because your doc tool doesn’t know where to find the tests. Some BDD frameworks have a separate tool to create the “spec” for the class, but it still doesn’t appear in places where the class API appears.

Sucky! Ok, so what are we going to do about it? Here’s what I propose in Noop:

Unit tests are a special entity, declared with a unittest keyword. The keyword is followed by a string literal, which allows you to write your intent in a normal sentence rather than “escaped” as a camel-case method name. They may appear either in a file dedicated to testing, or better, in the class being tested.

class TestThis() {
  String printHello(String name) {
    return "Hello %s!" % name;
  }
 
  unittest "It should print hello" {
    printHello("Fred") should equal("Hello Fred!");
  }
}

The unittest is a member of the class, just like a field or a method, so it naturally has intimate access to any fields or methods. And the test fixture is naturally provided as the “this” reference in the test, allowing you to simply call methods in the class.

Let’s deal with the objections now.

“Um, where does the ‘this’ instance come from?” -> I’m assuming here that the language has built-in dependency injection, so it’s normal to expect instances to be created for you. In the example above, it’s obvious how to make an instance of TestThis, so why should you have to make one yourself?

“Ok, but what about a less trivial constructor? What if the constructor needs some service that we want to mock?” -> Again, dependency injection to the rescue. You need a way to add “bindings” to the DI runtime, so that it understands how to provide instances of whatever objects you need in the constructor. These bindings need be declared in the unittest declaration, before the block that contains the logic and assertions, so that the ‘this’ reference is available in the first line of the block.

“I like having the tests in their own source root, like it’s been since the old days!” -> I think this is just an artifact of language tools that don’t understand tests, so the easiest way to compile the right set of sources, search for tests, and deal with dependencies was to create a separate source root. Starting with a fresh slate today, I don’t see why it’s necessary to continue to appease language tools at the cost of maintaining a separate package hierarchy and all the rest.

“But the production code shouldn’t depend on the tests, and the tests have extra dependencies that the production code doesn’t!” -> the first half is solved because the unittests aren’t methods, so there is no way to reference them in production code. The second half might require that there’s a way to make an import statement that only imports something for use by the tests. Maybe “test import junit.Asserts”? And the language tools need to provide a different set of dependency libraries in the classpath when running the tests vs. running production code

“But you don’t want to ship those tests to production!” -> well, I’m not sure why that’s a big deal, as there’s no way to call into that code without a test runner. But sure, the compiler or other language tools can support a “test” mode, just as many compilers have a “debug” mode to dictate whether the full symbol table is emitted.

“But this mixes testing and development!” -> You should really start doing Test-Driven Development. Testing is part of developing. They should be mixed.

I’m very keen on your comments on this one. Don’t leave me hanging!

Code style: too many choices

Q. Should a method parameter list continue on the next line aligned with the open paren, or 4 spaces from the left?

Every time I deal with code style, it annoys me. That’s because I have to do some annoying busy work to make my pretty text file serialization of the program conform to the group consensus in my team.

Don’t get me wrong, I think having a common code style is important and I’m glad all my teams do it. I just want to have it taken care of for me, I’d rather think in terms of editing the AST rather than pushing around ascii art in a bunch of files.

So why don’t we have the perfect code formatter? There are too many options. Look at this dialog for my Scala plugin in IDEA:

And that’s just the “Spaces” tab. If I have the tool in my IDE, along with a command line tool to tidy up pre-submit, there’s no way I’ll get all of those settings right for the N projects I work on using M machines.

So if I’m right, the only way to stop spending this annoying time is either to stop editing text files (Jesse Wilson Jesse Wilson is starting to convince me of that), or just pick one global style so there’s no need to configure my tools. Sun proposed a style for Java, but somehow every Java shop I work in has a slightly different guideline, so that didn’t work.

The funny thing is, unless you’re a zealot about your personal preferences, you agree that it’s an arbitrary choice. The method parameters can continue onto the next line in any reasonable way. We just want to make a consensus so we can stop talking about style. In Noop, I’m going to push that we provide a non-configurable lint tool with the language, and bake it into the tooling in some way so it’s the natural choice.

What code reviews ought to be

Google is the first place I’ve worked where code reviews are a formal part of every change. And I’m really sold on them. I will always make code reviews a part of committing code from now on, because it guarantees that someone else understands my code. Sure, it’s also handy if the reviewer notices a bug, or suggests a nicer API I could have used, or has some good refactoring ideas. But the most important result for me is “shared code stewardship” – code I write isn’t “my” code, it’s part of the team’s product and it’s going to need maintenance by other people. The code review is often the first chance for someone else on the team to really read through the code and understand my changes.

But, I think it sucks that no one read the code earlier. Code reviews are like a less effective form of pair programming.

The more you pair program, the more often you find that the code review is already done. Your pair can be your reviewer, they have already signed off on the implementation, and they’re a great co-steward of the code for the future. Or, as one team I’ve worked with does, you can send the review to a third person, just to have some extra familiarity with the changes that the pair has made.

By pairing on the change as you make it, the pair may have some input near the beginning of the coding session. As soon as you start to create a new class, and you give it a name, they might say, “hey, I think there’s this other pattern we might use for this,” and you change course immediately. Contrast that with a typical code review – you’ve got to have some pretty serious objections to someone’s code before you tell them to go back to the beginning and use a different design. You’ll never do it if the improvement is minor – it’s not worth changing it at that point.

So code reviews end up being more about style, minor nits, and reminders to fill in the documentation. That’s great, but it’s a poor substitute for having that person onboard sooner. Here’s another reason: if you want to suggest a design change in a code review, you’ve got to explain it in your comments. If you want to make a small nit like changing the order of import/require/include statements, you add a comment and make work for the author to make your change. In both cases, it would be better to just grab the keyboard and make the change, explaining your reasoning as you go. There’s no separate prose you need to write in a comment explaining the change.

So, here’s my proposal for an awesome code review tool:

• Pick the reviewer before you start making changes. They’re like your pair, but don’t have to be in your timezone.
• You commit to a throw-away branch as you work on your change. The branch is automatically set up for your review, and allows commits by the reviewer(s).
• When you commit on your branch, the reviewer gets notified, and can browse your code, in a read/write mode. Ideally it should be easy for the reviewer to load an IDE project from your review branch.
• The reviewer can add inline comments in the code, which don’t appear in the source file, but in some extra metadata file. They can also change the code directly. They commit their changes, with a commit comment, and the author is notified.
• The author considers the suggested changes, and can accept them or roll them back, with further comments if desired.
• Bonus: the tool facilitates easy screen sharing, so if you IM with the reviewer and don’t agree or understand, you just remotely pair on the spot.
• When there is some consensus and the change is done, the author does a merge back to the dev branch or trunk, and the review tool archives the review branch.

My top 8 reasons to use IDEA over Eclipse

I use IntelliJ IDEA as my primary IDE, although I’ve used Eclipse on and off in the past. The last time I switched back to Eclipse was to maintain the Eclipse plugin for Testability Explorer, and I was reminded of some things that I really missed. Here are my killer features:

Diff editor is full-featured

You’ve finished making some changes to a codebase you care about, so before you commit, you take care to look through the delta you’re about to commit and make sure it includes the right stuff, and to double-check your work. Often, I notice little things while I’m doing this – like missing documentation, some statements that could be refactored to a shorter form, or such. So, I begin editing the right side of the diff, and I need to have my real code editor there, so I can get completion in my {@link} javadocs, code completion, and so on.

I’ve seen people using Eclipse who get to this step and have to go back to the normal IDE window and then go find the file they were reading in the diff view. Too much context switching for me.

Completion shows compatible types

You start coding:

Widget w = new |

At the cursor, you’d like to get a completion list that shows the Widget class and any subtypes. This is especially important for some APIs that you don’t know well or don’t use often.

java.io.Writer w = new |

Man, I love getting suggestions like FileWriter there. I think Eclipse can only attempt to sort the list of completions in order of some heuristic of how likely you are to pick it.

Apple-arrow mapped to line home/end

It’s a minor thing on the face of it: on Mac, the home and end keys take you to the beginning and end of the document. Since Eclipse is on SWT, it’s not surprising that you get the native Mac behavior. In IDEA, maybe because it’s Swing, I get beginning and end of line instead. Whenever I use Eclipse, this majorly screws me up, because I forget to re-map my brain and lose my place in my editor.

I’ve tried to fix this in the OS, but it was too painful.

Understands prod/test code

I often feel like IDEA is made for people who write code, and Eclipse is made for people to write Eclipse plugins. This is an example of IDEA understanding how coding works – you always have a prod folder and a test folder. Instead of labelling these folders as just Source folders as in Eclipse, you tell IDEA whether they contain prod or test resources (or get it from an external model like a Maven POM). Now, when you do a search, you can search just prod code or just test code. When you make a new Test with the testdox plugin, it knows which source folder the new class belongs in.

Save on blur

We always work from a version control system. So, in my mind, the “buffer” I’m editing is my working copy. If I want to revert my changes, I use the VCS to do it. So, I hate the notion of “saving.” I don’t see how it’s ever useful to have different contents in the in-memory editor than you have on the disk. When I make changes to a file, I want the changes to get flushed out to disk. IDEA can do it when the IDE loses focus, which is a nice time. That way, my shell always sees what I typed. When I use Eclipse, I run something and it seems like my changes didn’t take effect. Then I realize there’s an editor tab with a star on it. The star makes me really frustrated, because it’s just the IDE telling me that it *knows* my disk has stale content, and asking me to do the busy work for it.

Diff annotations in gutter

Since we’re always coding against version control, our job is often to craft incremental changes to the codebase. I want to see that delta at all times, especially in the editor. That delta is my actual work product and it’s annoying to have to invoke another view to see what I’ve done so far, and revert changes or copy the previous content.

I’ve noticed that I’m more exploratory in my coding with this feature – if I don’t like the path I’m going down, it’s trivial to remove the last few changes and start a different change. I think there’s an Eclipse plugin for this, I guess that would work if it’s truly universal on all editor views and consistent with the VCS plugin, but I really don’t install many plugins.

SVN support built-in

It kills me to watch Eclipse users install a Subversion client plugin. It’s not easy to do, since you have to choose between JNI and pure Java, and the JNI version seems to have problems locating the native libraries in my experience. Maybe the install has gotten better, but still, this is another example where Eclipse should really support what I do out of the box. I think Subversion is popular enough that I should be able to check out a project with a freshly installed IDE.

Understands multi-changelists

I have more than one change happening in the same working copy a lot of the time. I like moving files into different lists, so when it’s time to commit some work, I have the right set of changes already grouped together. In Eclipse, I would usually toggle the checkboxes on the commit dialog to choose the files to include in the commit, and it was really hard to remember which classes belonged in the change. And I made a lot of mistakes.

Java – final variable to constructor parameter

The quickest way to type code is not to type, instead, let the IDE fix things as you go. Setting up the dependencies of a class is a common place where there’s a lot of typing, at least in a language with no properties like Java. I like to create my fields first, and if I make them final, IDEA will offer to add them as constructor parameters. Eclipse doesn’t – you must go the other way around, creating the constructor and adding parameters, then using the auto-fixer to create the fields.

$SwitchMap$

I recently became the maintainer of the Testability Explorer project. It’s a library that inspects Java bytecode, assessing how difficult the code will be to unit test. Check out the author’s blog: Misko Hevery. For more info about the project, see http://testability-explorer.googlecode.com.

Testability explorer looks for a few things in your code. One of them is some mutable object that’s globally accessible. For example, the Evil Singleton makes it hard to unit test anything that uses it, but a public static final String is fine, since String is immutable.

So I was investigating a strange issue where a large mutable global cost was assigned to some classes, due to an anonymous inner class. It seemed that there was some globally mutable state hidden in SomethingOrOther$1, whenever SomethingOrOther has a switch statement with a variable of type Enum as the argument. That doesn’t make your code hard to test, so what’s going on?

Try it at home! Compile this with javac:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

public class HasStaticCost {
  public int compare() {
    Fruit fruit = Fruit.APPLE;
    switch(fruit) {
      case APPLE:
        return 1;
      case ORANGE:
        return -1;
      default:
        return 0;
    }
  }
  public enum Fruit {
    APPLE, ORANGE
  }
}

And run javac HasStaticCost. The outputs are:

• HasStaticCost$1.class
• HasStaticCost$Fruit.class
• HasStaticCost.class

Three classes? What’s in that first one?? We’ll have to look at the bytecode:

class com.google.test.metric.collection.HasStaticCost$1 extends java.lang.Object {
static final int[] $SwitchMap$com$google$test$metric$collection$HasStaticCost$Fruit;
static {};
Code:
0:    invokestatic    #1; //Method com/google/test/metric/collection/HasStaticCost$Fruit.values:()[Lcom/google/test/metric/collection/HasStaticCost$Fruit;
3:    arraylength
4:    newarray int
6:    putstatic    #2; //Field $SwitchMap$com$google$test$metric$collection$HasStaticCost$Fruit:[I
9:    getstatic    #2; //Field $SwitchMap$com$google$test$metric$collection$HasStaticCost$Fruit:[I
12:    getstatic    #3; //Field com/google/test/metric/collection/HasStaticCost$Fruit.APPLE:Lcom/google/test/metric/collection/HasStaticCost$Fruit;
15:    invokevirtual    #4; //Method com/google/test/metric/collection/HasStaticCost$Fruit.ordinal:()I
18:    iconst_1
19:    iastore
20:    goto    24
23:    astore_0
// same for Fruit.ORANGE
39:    return
}

This is pretty crazy. The ordinal values of the Fruit enum are stored into a static array, named $SwitchMap$<enumname-encoded-with-dollars>, and stored in this synthesized inner class, and accessible to other classes in the same package. And that synthesized class isn’t stored with the enum, instead, each class that switches on the enum gets blessed with some ugly global state. Why? There was a release of the JVM in Java 1.5, along with the new language features, so you’d think the switch statement would understand an enum type. But this bytecode looks like a complete hack to make the switch statement think it’s operating on an int instead. Here’s how the switch is implemented:

0:    getstatic    #2; //Field com/google/test/metric/collection/HasStaticCost$Fruit.APPLE:Lcom/google/test/metric/collection/HasStaticCost$Fruit;
3:    astore_1
4:    getstatic    #3; //Field com/google/test/metric/collection/HasStaticCost$1.$SwitchMap$com$google$test$metric$collection$HasStaticCost$Fruit:[I
7:    aload_1
8:    invokevirtual    #4; //Method com/google/test/metric/collection/HasStaticCost$Fruit.ordinal:()I
11:    iaload
12:    lookupswitch{ //2
1: 40;
2: 42;
default: 44 }

At instruction 12, I would think the value 1 would naturally refer to the first ordinal value of the enum without needing to load the constant from the static array as it does.

It turns out this was just bad decision making by the Java 1.5 committee. They intended to implement Java 1.5 language features in a way that would execute on the 1.4 JVM, I bet because they knew that BigCorp wasn’t going to upgrade their production environments and the language features would sit on the shelf for a few years unless they could be deployed with little risk. So we got stuck with big things like generic type erasure, and small things like this hack for the switch statement… but hey, at least adoption would be easy.

I fixed testability explorer by whitelisting field names matching /\$SwitchMap\$.*/. Gross. Although that’s straightened out, it still leaves that bad taste in my mouth. Because it turned out that there was some feature added later on in Java 1.5 that did require a change to the VM, and so all these hacks are in there for no good reason. And, of course, BigCorp stayed on 1.4 for years.

Structuring Java code that assembles a UI

I’m dong some GWT coding today, which is a lot like writing Swing. It’s very procedural. Create a new UI element, then call methods on it to set up the state. Even when you do the right thing and split your UI into many Composite classes, you get something like this:

1
2
3
4
5
6
7
8
9
10
11
12
13

StackPanel stackPanel = new StackPanel();
stackPanel.add(new Label("Recently Eaten"));
stackPanel.add(new Label("Manual Entry"));
stackPanel.add(new Label("Search the database"));
 
DockPanel panel = new DockPanel();
panel.add(stackPanel, DockPanel.CENTER);
 
FlowPanel buttonsPanel = new FlowPanel();
buttonsPanel.add(new Button("Done"));
buttonsPanel.add(new Button("Next"));
 
panel.add(buttonsPanel, DockPanel.SOUTH);

This is really annoying. The order of assembly is important, but it’s also very arbitrary. I’m creating my DockPanel before my FlowPanel, but you should always declare variables in the narrowest scope possible (item 45 in Effective Java). On the other hand, maybe I want to finish referring to my StackPanel, adding it to the parent, before I create my FlowPanel. Also, it’s easy to make a mistake here. I might accidentally add something to the wrong parent container, if the variable names are similar, and there isn’t a good way to test this code aside from inspecting the UI visually.

Maybe Java is just not a good language to express this UI layout, but for now I considered an under-used construct, inline instance initialization blocks:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

final StackPanel stackPanel = new StackPanel() {{
  add(new RecentlyEatenPanel(), "Recently Eaten");
  add(new ManualEntryPanel(), "Manual Entry");
  add(new DatabaseSearchPanel(), "Search the database");
}};
 
final FlowPanel buttonsPanel = new FlowPanel() {{
  add(nextButton);
  add(doneButton);
}};
 
DockPanel panel = new DockPanel() {{
  add(stackPanel, DockPanel.CENTER);
  add(buttonsPanel, DockPanel.SOUTH);
}};

This is kind of cool – the structure is certainly more evident in this code. The extra braces define an initializer that is copied into each constructor – see the Sun tutorial. I assume that the code in the initializer block executes after the constructor has finished its work, so we can depend on the add() method having a place to do its work.

The limitations: this creates a subclass, so you can’t use it to initialize fields in a final class. Also, as you see, any variables must be declared final to be used in the initializer block, just like with any anonymous inner class.

Links:

Here’s a clever DSL to build the GUI in a “fluent” way: http://www.pathf.com/blogs/2007/09/expressing-rich/

And a project to use Flex-style XML to configure the layout: http://code.google.com/p/gwt-ui/

Chrome, the first mainstream Site-Specific Browser

In a previous post, I got into my passion about the misuse of web browsers. They’re intended for browsing content on the web, and not really intended for running applications like we do so much of today.

The problems with normal browsers can be summed up:

• The back button. In general, the browser’s controls only make sense in a browsing context.
• Apps running inside the browser don’t show up in Alt-Tab/Cmd-Tab, they’re hidden in a browser tab.
• Webapps can’t interact with the desktop in the usual ways, such as drag-and-drop, system tray/menubar integration, and running processes in the background like notifications.
• Dialogs and pop-ups lose the association with the application that originated them.
• One misbehaving webapp can take down the browser, along with other non-related apps.
• Offline storage is not generally available, other than what can fit in cookies.
• Security is a lot harder when state like your current session is expoitable by a site in another tab.

Work smarter, not harder… and faster!

Maybe it’s obvious. When you try to do things really fast, it can actually be slower.

I always thought “A stitch in time saves nine” meant that you should stitch as fast as possible. I think I just assumed that, which I need to be careful of, because when you ASSUME, you make an ASS out of U and ME.

It’s certainly true with programming. When it comes down to it, if you look at a finished application, it’s conceivable you could just type the code for it in an afternoon, especially if you have good tools that let you express your intent quickly. Of course, you don’t know exactly what the code will look like until you start to create it, but let’s ignore that for a sec.

So… taking my time and thinking for a second to save time. I was too frantic for the last couple years, and wrote a lot of Log object instantiations.

public static final Log log = LogFactory.getLog(NiceObject.class);

I wrote that a lot, and got a Das Keyboard so I could type it even faster. I knew that Eclipse could do this for me, but there’s that nagging startup cost, where the first time I set it up, it would take longer, and I might go looking around for more of the same (or blog about it), making it much longer than typing it out.

Well, lo and behold, I am happy I took the 30 seconds to:

Correction!

Well, I learned that what I said yesterday isn’t quite true. It IS true that the geocoder.us site does a bad job geocoding on this street. You can see their map here:
http://geocoder.us/demo.cgi?address=4050+BEN+LOMOND+DR+PALO+ALTO%2C+CA+94306

BUT, if you zoom in, you see the shape of the street is right, they just have the pin in the wrong place. The maps come from the TIGER/Line data as well as the geocoding. The problem is that the Perl module, Geo::Coder::US, doesn’t use one of the data files in the TIGER set – the RT2 “type 2″ data. This file is Complete Chain Shape Coordinates which means it has midpoints for some street segments. I found the two midpoints for the street I showed yesterday, and put some pushpins at those spots – and behold, the curvy street problem would go away!

Now, too bad I can’t find any libraries (in any language!) that do the same thing as the Perl library, but use the RT2 data…

Why TIGER/Line can be a bad source of geolocations

I came up with this neat map in Google Earth, showing the difference between Google’s coordinates for some addresses in Palo Alto, CA, and a line connecting those to the TIGER/Line-backed geocoder.us coordinate, colored blue-to-red based on how far off it is. The TIGER/Line data only has endpoints for a range of addresses, and the geocoder.us Perl module tries to do a linear interpolation to get addresses in the range. Obviously this doesn’t work well for non-linear streets like this one…