Extracting a Helper Class – How to deal with tests?

[Andreas Altendorfer]

I have a dilema and wonder how you guys think about this situation ...

Suggesting a class which is complet and 100% test-covered.

Then, when I refactor this class and extract a 'Helper-Class' into a separate source-file, I wonder if I should extract/move related tests as well.

Pro: 

1. When browsing the source (of the new class) it will not be clear where the test-coverage comes from.
   
2. When I remove the origin class but leave the extracted helper, then it might loose it's coverage (unless it is involved from another part of the app).

Contra:

1. By extracting the class I didn't harm coverage. So why I should spend more time as necessary? In scenario (2) above, my coverage-tool will blame the missed lines anyway.
2. When I remove the origin class but leave the extracted helper, then my coverage tool will not blame unused code because the tests still covers (and uses) the helper-class.

My dilema is, Pro(2) is also Contra(2). Sometimes it happens that code becomes unused while refactoring. I'm using SimpleCov and it will highlight this code as 'not covered'. Since I'm consequently writing the tests first, this usually means, I can remove this uncovered code, it's just not used anymore. 

From this point of view I would not move the tests. But, on the other hand, I see the tests being an important part of my app's documentation, and therefore a class should have such a documentation.

What would you do?

[Sebastian Larsson]

Uncle Bob answers this exact question in his latest video - E21. He says that he normally does not move the tests and that this kind of refactoring often results in inner classes for him.

My answer to your question is that it depends on the amount of job to move the tests. If the tests are grouped together and you can just extract a class and place it appropriately then do it. If the "helper class" is an inner class then I would expect to find it in the test of the outer class.

BRs

Sebastian

[Andreas Altendorfer]

Hello Sebastian,

Thx for your response. Yes, the video covered this issue. For inner classes, it's all clear, but for classes which will live there on it's own I still found no (generell) answer.

So, I will have to make individual decisions, depending on the purpose of the class. I'd appreciate a tool telling me either a LOC is called by tests only or from any other production code.

best

- Andreas

[Uncle Bob]

If the extracted class is of general utility, and will be used by many others, then refactoring the tests is probably the right choice.  If, however, the extracted class is an implementation detail -- part of the mechanism instead of the interface, then it's likely best to leave the tests as they are.  

[Andreas Altendorfer]

Thank you for your response. It's clear (as always)

Though I'm used to follow this strategy, I feel uncomfortable when I have a source-file and I can't see where the spec, covering this code, is located.

I'm using Guard which runs the tests for changed files automatically. Guard then has the same problem as I do, it doesn't know where the tests are and does nothing when the file is modified. 

Although, running the full test-suite, of course will cover the "helper-class".

Today I found a way out of this dilemma. I'm not sure if I'll do this from now on, but I'll give it a try for a while.

When I extract a helper-class which is fully covered by an existing test, I create a spec-file for this class at the place where I (and Guard) will expect it to be.

But I don't move the tests, I just add one spec to it:

describe "HelperClass" do
  it "is covered by SomeController" do
    expect( cover_source_by( "some_controller_spec.rb" ) ).to be_true
  end
end

cover_source_by touches 'some_controller_spec.rb' if it's older than 'helper_class.rb'.

Now, I can easily find the related spec_file for my helper-class and guard will run the right spec immediately whenever helper_class.rb is modified.

Creating the helper-spec can be appended to the refactoring-script which I use to extract the class itself; so I'm not wasting a second on this.

And even more. Should I ever delete "some_controller" SimpleCov will mark the source of HelperClass as not covered and give me the hint to remove this, now unused, code.

Which wouldn't happen if I move the tests too.

I think that's a cool approach. Or do you see any smell?

[Uncle Bob]

I guess the only smell that bothers me about this is that the structure of your source code is being put under pressure by some support tools.  I'd be looking for ways to get those support tools to behave well with properly structured code instead.

But then we all live with tools that we wish were better.  So I wouldn't abandon your approach if it's working for you.  I'd just be aware of the smell and perhaps look into getting those tools to work a little better.