DEV Community

Discussion on: Why test POJOs?

Collapse
 
scottshipp profile image
scottshipp

If I read you correctly, your logic is that since you know what your getter does you don't need to test it. That's the same logic that people use to not test any of their code. "I know what the code does. I don't need to test it."

Tests prove that your code does what you think it does, though.

Another question I have is can you guarantee that if anyone else is going to touch that code they'll write a test for it? In a shared codebase, that's a nice idea, but not the practical outcome usually. Especially if there's no code coverage requirement on that package, so you'll see people neglect adding any tests there.

Collapse
 
sakfa profile image
sakfa • Edited

I get your point about tests prove your code does what you think it does, but as always common sense applies - you already mentioned one of the arguments I would make here "Why test that the language does what the language does?". I would always write a test for any method that consists of at least one actual operation, so method like getFullName() { return firstName + " " + lastName } would always warrant a assertEquals("John Smith", new Person("John", "Smith").getFullName()) test.

Also earlier you made a strong point about testing constructor contract, I obviously agree that sloppy developer could write this code:

public String getLastName() {
  return firstName; // ack!
}

where he clearly thinks his trivial code that doesn't need test does something else than what it actually does. A trivial assertEquals("Smith", new Person("John", "Smith").getLastName()) test would catch it.

But is that enough to convince me to write these tests? Not really, for 2 reasons:

  • entity classes are never mocked away meaning some other test will catch this anyway. If none of your test catches it, you probably have way more serious problems in your code base than untested POJOs so you can probably spend your precious time fixing these more serious problems first. Once you do, you no longer need POJO tests because now none of your entities have unused fields anymore and all services operating on these POJOs have at least their happy paths tested meaning all your getters and setters should either be covered or not exist.

  • any sane Java developer would generate their constructor, setters and getters for data classes. If I advocated in my team to write test to POJOs it would probably drive everyone crazy - "I just generated this 8 args constructor and 8 getters using just 2 keypresses, do you really want me now to write a test and then copy paste it 7 times with small modifications?". On the other hand if a developer doesn't generate their setters and getters he will probably spend his time way more productively learning and actually start using tools to do that, than writing these tests. Using generators will increase their productivity forever. Writing tests won't.

As for

Another question I have is can you guarantee that if anyone else is going to touch that code they'll write a test for it?

First thing to say is of course not, just as you can't guarantee that anyone else is not going to delete your tests. We can't guarantee much about behaviour of future devs, but there has to be some basic trust in organization. We shouldn't write code to protect from future misbehaving developers. What we should do though is minimize the risk of someone misbehaving by scrutinizing each change made. In my organization for instance we have following process:

  1. Any change has to have a code review published. No excuses, publishing CR is as easy as running publish-cr, if you have time to run git push you also have time to run publish-cr. So there's no reason to not do that (barring code review tool failure, perhaps)
  2. Any change should be approved by a team member or subject matter expert. This is not enforced strictly, but see next point.
  3. If a change without a CR or without an approval is pushed anyway, continuous integration approval subprocess we call "code review police" will fail to approve (and thus automatically deploy) this change.
  4. Of course we don't want to slow people down - change author (anyone actually) can still override this failure, literally saying "yes, I don't have approval. I know what I'm doing. Deploy". When he does that though, an e-mail is sent to entire team owning the package saying he did so.
  5. Team members would usually scrutinize such e-mail and review it even more thoroughly than usual (after all this code is probably already running in production). At this phase crappy code would likely be called out, but to be fair I have never witnessed anyone force-deploying crappy code. Psychology works here, if you know your "force deploy" is scrutinized you won't do this for ugly changes.

And to aid in detecting missing tests our CR tool is quite sophisticated - when change is published, it displays diff side by side, runs tests with coverage and on that side-by-side diff marks in light red lines not covered by any test. Engineers doing CR would usually raise a concern if you made a non-trivial change on a red line. Unless common sense says this line doesn't really need test, and if common sense of both author and reviewer says that - then yes, you probably do not need test.

So no, I can't guarantee future dev will write test, but with proper tooling and processes in place I can minimize the risk of having crappy code reaching production. IMHO this is way more important than any kind of artificial rules "at least 80% coverage" or "all new code has to be covered by test". To me common sense always wins against dogma. I witnessed too many changes which covered 100% lines, including all their catch blocks to reach coverage goal but failed to write a basic "Serialize/Deserialize` test ("if I serialize this object to DB and then read it back, is it equal to what I wrote?") to believe in threshold-based rules

Anyway, I understand your position and don't see anything particular wrong with it. Just wanted to share my 3 cents.