Is refactoring of tests/spec a good idea?

When I see in my test code specs like this for every controller:

it "#new displays input controls for topic title and keywords" do

  ensure_im_signed_in
  get :new

  assert_response :success
  assert_select "input#topic_title"
  assert_select "input#topic_keywords_input"
  assert assigns :topic
end

I want to refactor it and replace with some one-liner like this:

its_new_action_displays_input_form :topic, %w(input#topic_title input#topic_keywords_input)

and implementation:

def its_new_Action_di开发者_高级运维splays_input_form field, inputs
  it "#new displays input controls for #{inputs.join ", "}" do
    ensure_im_signed_in
    get :new
    assert_response :success
    for css in inputs
      assert_select css
    end
    assert assigns field
  end
end

What are the advantages of either keeping verbose version or refactoring to terser version?

I see only problem with refactored version is that RSpec does not show backtrace for the failed test.

The biggest question we always ask is "who is testing the tests?" You should definately go with refactoring your unit tests where ever you are and at whatever stage you are as it helps reduce the complexity. Reducing the complexity within your unit tests will yield the same benefits as reducing the complexity within your code base. I can see very little reason not to do it and many reasons to do it.

My very first job out of college was updating a set of unit tests. They were about 7 years old, with many deprecated methods being used. It took me eight months (there were a lot of them!) and I still didn't finish the job. However, I did manage to reduce the file size to about 1/3rd the original size and greatly simplify the work by refactoring much of it, and that helped speed up the work.

So, I would definitely encourage refactoring!

You refactor the spec because of the same reasons that you refactor everything else: It will make your application – be it you production app or tests suit – runs faster.

Also, refactoring a piece of code makes you think about the intention that you had to writing such a thing. This increases your comprehension of the problem and will make you more capable of solving other problems in the same application.

Yet I would strongly disagree about your refactoring. You see, when you are watching a spec you want to see fast and clear what are you testing. If you move the asserts to other place, you are hitting your self, because you need to go and find where did you put that thing.

I suggest you that never move the asserts out of your spec. Even if they are repeated. That's fine. It remarks the intention, so you never forget what you want to test.

Instead, focus your refactoring on boiler code.

For example, you are testing a form and to get to that page you need to click many links. A first approach would be to put everything in the spec. A lot of click_link.

I refactoring of such code will be put the whole bunch of click_link to a before(:each). Use Context as well, to clarify.

Like this:

feature "Course" do
  context "Loged in" do
    before(:each) do
      school = School.make!
      switch_to_subdomain(school)
    end

    context "In the new course form" do
      before(:each) do
        click_link("Asignaturas")
        click_link("Nueva asignatura")
      end

      scenario "New course" do               
        fill_in(:name, :with => "Matematicas")
        click_button("Create Course")
        page.has_content?("Asignatura creada").should == true
        dbSchool = School.find(school.id)         
        dbSchool.courses.count.should == 1
      end
          scenario "New course" do               
        fill_in(:name, :with => "")
        click_button("Create Course")
        page.has_content?("Asignatura creada").should == false
        dbSchool = School.find(school.id)         
        dbSchool.courses.count.should == 0
      end


    end
  end  
end

You see? The boiler code is out of the specific spec. But let enough on the spec so that 7 month after that you can understand what you are testing. Never take away the asserts.

So my answer: Refactor Spec is not only a good idea, but a necessity. But don't mix refactoring with hiding code.

You should indeed refactor your tests to remove duplication. However, there are likely ways to do it that that keep the failure backtrace.

Rather than pulling everything out to a single method, you might do well to have a shared before(:each) section for the common setup, and write your own matchers and/or assertions to combine the checks.