Tag Archives: testing

Running SimpleCov when COVERAGE=true

1 Reply

I use this trick every time I start a new project, which is just often enough to go look it up but not often enough to commit it to memory, so here it is in blog form:

spec_helper.rb
if ENV['COVERAGE']
    require 'simplecov'
    SimpleCov.start 'rails'
end

Of course if you’re writing a non-rails app omit the ‘rails’ argument. If you’re not using RSpec, put it in test/minitest_helper.rb. (And, of course, if you’re using Test::Unit, stop using Test::Unit and switch to MiniTest!)

Now when you run your specs or tests as normal with ‘rspec spec’ or ‘rake test’ your regular tests will run normally. To run them and get a coverage graph, however, just run

COVERAGE=true rspec spec

or

COVERAGE=true rake test

And there you go. If memory serves me correctly, this trick is credit due to Jake Mallory, from a project we worked on together last year.

Yes You Should Test Private Methods (Sometimes)

4 Replies

Lasse Koskela recently wrote Test Everything, But Not Private Methods on his blog. Thank you for writing this, Lasse. This is one of the most intelligent, albeit still wrong, posts I’ve read on the topic. Lasse specifically addresses one of my greatest concerns, which is simply that “if it can possibly break, it should be tested.”

It turns out that on the face, we are in agreement: you should not leave complicated code untested. Lasse’s answer (which is repeated by several folks, including Michael Feathers and Mike Bria) is simply to not write complicated private methods. If you have a complicated private method, you should make it public and test it. If it doesn’t belong on the public interface of the class, then move it to another class or create a new one where it can be public. As Michael Feathers puts it in his book Working With Legacy Code: “the real answer is that if you have the urge to test a private method, the method shouldn’t be private; if making the method public bothers you, chances are, it is because it is part of a separate responsibility: it should be on another class.”

I’m a huge fan of Michael Feathers, bordering on Fanboy lunacy, so I’m a bit hesitant to disagree with him. Also, to be fair, I agree with Lasse one hundred percent–about 90% of the time. However, Mike and Lasse present “extract method to class” as a panacea when it is in itself merely a tradeoff with serious consequences of its own.

Extracting a class dilutes and adds complexity to your namespace. Code should struggle very hard to earn a spot at this level. I spend a lot of mental energy on a project trying to understand the class hierarchy, so much so that it is worth my effort to spent time trying to keep it well-organized to assist this understanding. A method that was meant to be private and to serve a class at a specific point in time suddenly becomes a public object. Care must be taken to either make the new class generic enough and provably safe to be used by arbitrary clients, or to prevent other classes from accessing the class at all. If you find yourself trying to figure out how to control access to the class, chances are the code you are trying to extract really should be private after all.

If you are going to extract the private method to a class, considerable care should be taken that it does not, in fact, still fall under the responsibility of the class that contained it. If your new class ends up with an “-er” name, especially “-Manager” or “-Controller”, it’s probably just a Functor–a method call masquerading as an object. If the moved method has many side effects, especially ones that modify instance variables, the moved method will end up taking many arguments and returning many values that the original class must then use to modify itself. You may just end up passing in the original class to the method. Now what you’ve done is replaced the “this” keyword with a variable named “that”. This is a code smell; it’s called Feature Envy, and the solution is to move the method into the envied class. If the new class has the original class’s name in its own, you’ve all but conceded that the new class is useless in isolation from its progenitor; all you’ve really accomplished in this case is cluttering up the object namespace in order to achieve one outcome: making the private method testable. Lasse eschews inelegant workarounds like reflection nonsense or package hackery, but complicating the object map is just as inelegant if it achieves nothing more than making a private method public.

Another counterargument is–and though this is a situation that I wish did not actually exist, every developer has in fact faced it–extract class is not a free refactoring, and on large legacy projects it may be prohibitively expensive. Suddenly “change the design” becomes “they shouldn’t have designed it that way in the first place” and the only solution is to invent a time machine, go back in time, and not have done it wrong. This isn’t practical.

Sometimes you have a method that is, and should be, both private and complex enough to require testing. When that happens, you should test it.

I agree with Lasse and Mike about the problems inherent with testing private methods; I just don’t think their proposed solution is always workable.

This post is getting long, so I’m going to breaak it into to two separate entries. Next up: I want to present some cases when I think I should, even using TDD, design and write private methods that should be tested. I’m also going to address what I think is the primary concern of anti-private-testers, that testing private methods is testing implementation. (Teaser: I completely agree–but I hope to show you why, in some cases, that’s not a problem or at least an acceptable tradeoff.)