Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Rubocop 25 line block size and RSpec tests

A typical RSpec unit test makes extensive use of nested Ruby blocks in order to structure the code and make use of DSL "magic" to have specs read like BDD statements:

describe Foo do
  context "with a bar" do
    before :each do
      subject { Foo.new().add_bar }
    end

    it "looks like a baz" do
      expect # etc

In an ideal spec, each example can be relatively short and precise. However it seems usual to have outer blocks grow to 100 lines plus, because the RSpec structure works this way, and doesn't take many spec examples, each of which might have a few lines of specific setup, to get to describe blocks that are the same size or larger than the code for the subject being described.

A recent upgrade of Rubocop brought a new rule into play, that blocks should be no longer than 25 lines. I'm not sure of the rationale for it, because it is not listed in the Ruby style guide. I can see why it could be a good thing, and added to default ruleset. However, after the upgrade, our Rubocop test fails multiple times with messages like tests/component_spec.rb:151:3: C: Block has too many lines. [68/25]

With code metric tools such as Rubocop, I like to have a policy of "Use the defaults, link to the style guide, job done." (mainly because debating tabs vs spaces and other minutiae is wasting time, and IME never gets resolved) Here that is clearly not possible, two of our core data quality tools disagree about code layout approach - or at least that is how I interpret the results, I don't see anything intrinsically wrong with how we've written the specs.

In response, we have simply set the Rubocop block size rule to a high threshold. But that makes me wonder - what am I missing? Is RSpec using a now discredited approach for code layout, and what reasonable options do I have to reduce block sizes in our RSpec tests? I can see ways to restructure the code to avoid large blocks, but they are without exception ugly hacks intended purely to meet Rubocop's rule e.g. break out all the blocks into helper functions:

def looks_like_a_baz
  it "looks like a baz" do
         expect # etc
  end
end

def bar_context
  context "with a bar" do
    before :each do
      subject { Foo.new().add_bar }
    end
    looks_like_a_baz
  end
end


describe Foo do
  bar_context
  # etc

. . . I mean, that's do-able, but turning bunches of spec examples into helper functions in this way seems to be the opposite of the readable approach encouraged by RSpec design.

Is there anything else I can do other than find ways to ignore it?


The closest existing question I could find about this topic here was RSpec & Rubocop / Ruby Style Guide and this looked solvable by editing test templates.

like image 833
Neil Slater Avatar asked Dec 02 '16 14:12

Neil Slater


2 Answers

If a specific block is usually too long, I specify it rather than the files

Metrics/BlockLength:
  IgnoredMethods: ['describe', 'context']
like image 158
J. Willette Avatar answered Oct 17 '22 22:10

J. Willette


A recent upgrade of Rubocop brought a new rule into play, that blocks should be no longer than 25 lines. I'm not sure of the rationale for it, because it is not listed in the Ruby style guide.

It used to be that all the cops were based on The Ruby Style Guide, and RuboCop was a way to adhere to the practices set out by the community.

Direction has changed since then, and the scope of RuboCop has expanded to help developers ensure consistency in their code bases in general. This has lead to two things:

  1. Cops (even those based on The Ruby Style Guide) are now mostly configuratble.
  2. There are cops for things that aren't mentioned in the Ruby Style Guide, but are still useful for enforcing consistency in a project.

This cop falls in the second category.

Is RSpec using a now discredited approach for code layout, and what reasonable options do I have to reduce block sizes in our RSpec tests?

Short answer is no. DSLs are still cool. :-)

This cop is aimed at large block in the imperative programming sense. As a general guide, it does not apply to DSLs, which are often declarative. For example, having a long routes.rb file in Rails is perfectly benign. It's just a natural result of a large application, rather than a style violation. (And having a lot of tests is purely awesome.)

Now, RuboCop is pretty smart, but it doesn't know what is a DSL and not, so we can't automatically ignore them. One could argue that we could exclude DSL entry methods of popular frameworks, like Rails routes and RSpec specs. The reasons for not doing so are primarily:

  1. False negatives. Any class can implement a method, taking a block, with the same name.
  2. RuboCop is a Ruby analysis tool, and shouldn't really know about external libraries. (Excluding the /spec directory is a courtesy until we have a proper extension system, and this can be handled by the rubocop-rspec gem.)

I mean, that's do-able, but turning bunches of spec examples into helper functions in this way seems to be the opposite of the readable approach encouraged by RSpec design.

The bottom line is: RuboCop is there to help us write better code. If our application design is otherwise sound, and we find ourselves making things less readable merely to please RuboCop, then we should filter, configure, or disable the cop. :-)

In response, we have simply set the Rubocop block size rule to a high threshold. But that makes me wonder - what am I missing?

This is a rather blunt tool and, as you're hinting at, you will probably have some false negatives because of it. There are two types of false positives for this cop:

  1. Files that contain purely declarative DSLs, e.g. Rails routes, RSpec specs.
  2. Files that have a declarative DSL mixed into mostly imperative code, e.g. an aasm state machine declaration in a Rails model.

In the first case, the best solution is to exclude the file or directory, and in the second to use an inline disable.

In your case, you should update your .rubocop.yml with:

Metrics/BlockLength:
  Exclude:
    - 'Rakefile'
    - '**/*.rake'
    - 'test/**/*.rb'

(Note that you need to re-iterate the basic excludes from the default configuration, since the list will be overwritten.)

like image 23
Drenmi Avatar answered Oct 17 '22 22:10

Drenmi