Thursday 20 August 2015

Please stop writing tests for POJOs (AKA "JaCoCo-driven development is not a thing")

Every time we write a test like this:

public class ModelObjectTest
{
    @Test
    public void testModelObject() {
        ModelObject mo = new ModelObject();
        mo.setA(1);
        mo.setB(“wharrgarrbl”);
        assertThat(mo.getA()).isEqualTo(1);
        assertThat(mo.getB()).isEqualTo(“wharrgarrbl");
    }
}

Kent Beck sheds a single, solitary tear.

Every time we do this:

public class ModelObjectTest
  extends PojoTest // where PojoTest is some kind of reflection-magic Pojo set/get test
{
    @Test
    public void testModelObject() {
        testPojo(new ModelObject()); // haha, JaCoCo! now we have coverage!
    }
}

*I* get upset. That thing up there tests nothing, and is worse than useless.

But why does it upset my delicate sensibilities? We need to get our code coverage figure up to 90%, don’t we?

Sure we do. But what does code coverage mean, in a world of Test-Driven Development? Wait, let’s take a step back and remind ourselves what Test-Driven Development actually is. In its simplest form, it looks like this.
  1. Decide what the logic should do.
  2. Write a unit test that describes what the code (which does not exist yet) should do.
  3. Run the unit test and watch it fail.
  4. Write some code that passes the test.
Note that none of this mentions POJOs ("Plain Old Java Objects" - what we used to call beans, things that just have a bunch of fields, and methods that set and get those fields). We make POJOs as a way of passing data around. And we make them because “logic” code, code that does stuff and is described by unit tests, needs them.

If our logic code is being created by TDD, that means we only ever create or add to a POJO because a test of some other class is failing and we need to change it to make the test pass.

So this means the POJO should always have 100% test coverage already. It doesn’t need a test class of its own.

Aha! But. Sometimes we have these model classes that only exist to be serialized into JSON and returned by web services, or deserialized and consumed. They’re not covered by our logic, so they won’t get tested.

This is fine.

If they really are all just data transfer objects, their job is simply to have a certain structure. They effectively have no logic and - this is going to sound heretical but bear with me - they do not need unit tests.

But what about JaCoCo? They’ll bring our coverage metrics down! 90% or die, right?

Put them in their own Maven submodule - you should be doing this anyway, so the API model can be shared as a JAR. Set this module’s JaCoCo limit accordingly. Be happy.

Having a huge number of model classes with 100% coverage in the same module as classes with real logic is actually bad, because it artificially inflates the coverage numbers. I recently moved the model classes out of a project so they could be shared, and was surprised to find that the JaCoCo check started failing. It turns out some of the logic was completely untested, but the 90% mark was being met because of all the useless POJO test coverage.

Let’s talk about JaCoCo for a second, by the way.

This is not TDD:
  1. Write some code.
  2. Run JaCoCo and watch it fail.
  3. Write some tests that increase the coverage metric until JaCoCo passes.
JaCoCo is actually a decent tool, but we are abusing it. It exists because we are fallible, because writing good tests is sometimes quite difficult, TDD is a hard discipline, and it’s good to have backup.

JaCoCo produces an HTML report, and I like to look at it after my test run and make sure there are no surprises. “Hey, this looks weird - that line is not covered, but my test passes. What’s it doing there? Either my test or my code is wrong, let’s investigate."


Friday 3 September 2010

TypeErrors in to_json make me briefly sad

TypeError Exception: wrong argument type JSON::Pure::Generator::State (expected Data)


Sometimes, the ability to monkey-patch things in Ruby is great. It leads people to add globally useful functionality, like a to_json method for all objects.

Sometimes though, you're scratching an itch somebody else has scratched already. And when you do, you mess with people's verbs, so you'd better make sure you do it consistently.

It turns out that ActiveSupport and json(_pure) have both scratched that to_json itch. And they've done it in incompatible ways. Specifically, if you are in the middle of an ActiveSupport to_json call and you call one added by the json gem, you end up passing it the wrong arguments. I'm far from the only person to run into this, and people solve it in different ways.

I'm not in a position to Just Pick One, because couchrest and facebooker depend on the json gem, and other stuff depends on ActiveSupport. My problem is specifically when serializing Arrays, but I don't like the idea (suggested in the linked thread) of klumping together a hybrid Array/Enumerable mixin that will shamble about intercepting each() calls. But a bit of poking at specifics reveals that my issue is maybe more restricted than I thought:


>?> [].to_json
=> "[]"
>> ["1"].to_json
=> "[\"1\"]"
>> [1].to_json
TypeError: wrong argument type JSON::Pure::Generator::State (expected Data)
from /opt/local/lib/ruby/gems/1.8/gems/json-1.4.6/lib/json/pure/generator.rb:318:in `to_json'
from /opt/local/lib/ruby/gems/1.8/gems/json-1.4.6/lib/json/pure/generator.rb:318:in `json_transform'
from /opt/local/lib/ruby/gems/1.8/gems/json-1.4.6/lib/json/pure/generator.rb:315:in `each'
from /opt/local/lib/ruby/gems/1.8/gems/json-1.4.6/lib/json/pure/generator.rb:315:in `json_transform'
from /opt/local/lib/ruby/gems/1.8/gems/json-1.4.6/lib/json/pure/generator.rb:302:in `to_json'
from (irb):12
>> 1.to_json
TypeError: wrong argument type JSON::Pure::Generator::State (expected Data)
from (irb):13:in `to_json'
from (irb):13


It turns out it's only happening when I'm dealing with a Fixnum. Maybe this will turn out to be a simple thing...

In the activesupport gem directory, there are a whole bunch of files that do the monkeypatching of various classes to support to_json. One of them, numeric.rb, is full of entries like this:


class Integer
def to_json(options = nil) #:nodoc:
to_s
end
end


Could it be that ActiveSupport just needs to add one for Fixnum, since the json gem seems to have put one in so that ActiveSupport's calls don't just fall through to (the version added by ActiveSupport::JSON to) Object?

Actually, yes.

Putting


class Fixnum
def to_json(options = nil)
to_s
end
end


In my initializer setup makes it all go away.