rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
804 stars 277 forks source link

Cop idea: Long example description #247

Open andyw8 opened 7 years ago

andyw8 commented 7 years ago

A block such as it "..." do containing a very long description string is likely a sign that either the test or the implementation is too complex.

backus commented 7 years ago

Interesting. Would this be conditional on anything like whether the test is inside of a context? Also how long is too long? Maybe a better heuristic is if the description string contains conjunctions? I grepped out most of the description strings in our project out of curiosity:

composes sets
has a valid URL
ignores comments
recreates source
flags an offense
compares by value
has one assertion
ignores view specs
ignores subclasses
approves of should
allows bang method
checks class specs
has two assertions
flags it { should }
checks class methods
registers an offense
ignores feature specs
builds a node pattern
flags nested contexts
ignores request specs
ignores routing specs
returns first element
allows a short example
flags an empty context
approves of should_not
checks instance methods
ends with a punctuation
ignores non-spec blocks
detects :each for hooks
ignores non-rspec hooks
allows an empty example
supports RSpec.describe
flags it { should_not }
permits two expectations
flags three expectations
checks subject below let
ignores instance_doubles
handles ACRONYMClassNames
alphabetizes cop requires
flags focused block types
handles subjects in tests
enforces non-method names
ignores an empty describe
detects :example for hooks
ignores non-example blocks
has an assertion on line 2
approves of is_expected.to
flags nested subject stubs
fails if the list is empty
ignores :context and :suite
flags allow(...).to receive
handles ALLCAPS class names
flags it { is_expected.to }
flags multiple expectations
detects the `not_to` offense
handles subjects in contexts
flags expect(...).to receive
detects the `to_not` offense
does not flag unfocused specs
has descriptions for all cops
builds a hash of descriptions
approves of subject above let
handles CamelCaps class names
finds old `stub_chain` syntax
finds `receive_message_chain`
approves of is_expected.to_not
does not flag include_examples
has configuration for all cops
ignores doubles without a name
registers the offense on line 1
flags it { is_expected.not_to }
flags it { is_expected.to_not }
skips single top level describe
has assertions on lines 1 and 2
complains when subject is mocked
does not start with a lower case
handles alphanumeric class names
allows subdirs for class methods
ignores non-spec context methods
only flags third level of nesting
approves of allow(...).to receive
flags deeply nested subject stubs
flags expect(...).to receive with
ignores described class as string
ignores let! when used in example
flags an empty top level describe
flags normal metadata in describe
allows flexibility with operators
ignores the file if it is ignored
complains when subject is stubbed
flags violations within all blocks
has assertions on column range 1-3
ignores nested describe statements
allows flexibility with predicates
ignores let! when used in `before`
approves of expect(...).to receive
flags expect(...).to have_received
flags specs with non :type metadata
ignores describes with only a class
find doubles whose name is a symbol
ignores non-alphanumeric characters
allows subdirs for instance methods
has an issue number prefixed with #
has an assertion on column range 1-3
flags expect(...).to receive at_most
ignores if namespace is not matching
skips methods starting with a . or #
includes Enabled: true for every cop
ends every description with a period
checks first-line describe statements
returns true for selectors in the set
detects no offense when using `to_not`
ignores doubles whose name is a symbol
ignores class if the scope is changing
detects no offense when using `not_to`
approves of one expectation per example
flags expect(...).to have_received with
has a colon and a whitespace at the end
doesn't blow up on single-line describes
rewrites "should miss me" as "misses me"
does not care about the described method
approves of expect(...).to have_received
only takes class from top level describes
ignores hooks with more than one argument
ignores non-rspec code with :focus blocks
has a link to the contributors at the end
checks for the use of the described class
autocorrects `before { }` to `before { }`
allows flat hierarchies for class methods
checks describe statements after a require
returns false for selectors not in the set
does not flag hooks without default scopes
flags literal nil value within expect(...)
does not have newlines in cop descriptions
flags expect(...).to have_received at_most
still flags doubles whose name is a string
finds a `spy` instead of an `instance_spy`
respects custom module name transformation
has a whitespace between the * and the body
has link definitions for all implicit links
ignores instance variables outside of specs
fails if the list has more than one element
generates a todo based on the detected style
does not flag a method that is focused twice
builds a YAML dump with spacing between cops
allows flat hierarchies for instance methods
finds an instance variable inside a describe
generates a todo based on the worst violation
rewrites "should have sweets" as "has sweets"
ignores an instance variable without describe
rewrites "should amass debt" as "amasses debt"
complains when let! is used and not referenced
rewrites "should do nothing" as "does nothing"
does not flag violations within a scope change
does not flag an otherwise empty example group
ignores subject when not wrapped inside a test
flags numeric literal values within expect(...)
has an assertion with correct violation message
checks around(:each) for explicit subject usage
skips specs not having a string second argument
flags boolean literal values within expect(...)
does not flag dynamic values within expect(...)
finds description with `should` at the beginning
does not flag violations within non-rspec blocks
autocorrects `before { }` to `before(:each) { }`
autocorrects `before(:each) { }` to `before { }`
finds description with `Should` at the beginning
finds a `double` instead of an `instance_double`
ignores describe that do not reference to a class
ignores feature specs when RSpec.describe is used
checks for the use of described class with module
ignores feature specs - also with complex options
skips specs that do not describe a class / method
checks highlights the first argument of a describe
finds an instance variable inside a shared example
rewrites "should wish me luck" as "wishes me luck"
checks before and after for explicit subject usage
finds multiple top level describes only with class
finds description with `shouldn't` at the beginning
autocorrects `before(:example) { }` to `before { }`
adds a message saying what the path should end with
ignores the source when the path is not a spec file
registers an offense for `eql` when argument is nil
autocorrects `before { }` to `before(:example) { }`
flags an instance variable when it is also assigned
rewrites "should pay for pizza" as "pays for pizza"
checks `it` and `specify` for explicit subject usage
adds a message saying the example has too many lines
flags all rspec example blocks that include `:focus`
ignores an instance variable when it is not assigned
does not recognize custom include methods by default
skips descriptions without `should` at the beginning
checks for the use of described class with namespace
rewrites "should deploy the app" as "deploys the app"
ignores nested stubs when nested subject is anonymous
rewrites "should obey my orders" as "obeys my orders"
skips specs that do have multiple top level describes
ignores stub within context where subject name changed
rewrites "should echo the input" as "echoes the input"
rewrites "should buy the product" as "buys the product"
registers an offense for `eql` when argument is a float
autocorrects `before(:each) { }` to `before(:each) { }`
generates a todo based on the usage of the correct style
registers an offense for `eql` when argument is a symbol
finds multiple top level describes with class and method
registers an offense for `eql` when argument is a boolean
rewrites "should return something" as "returns something"
flags string and symbol literal values within expect(...)
generates a todo based on the usage of the alternate style
autocorrects `before(:example) { }` to `before(:each) { }`
rewrites "should alias the method" as "aliases the method"
autocorrects `before(:each) { }` to `before(:example) { }`
registers an offense for `eql` when argument is an integer
rewrites "should fax the document" as "faxes the document"
flags nested subject stubs when adjacent context redefines
ignores the source when the path is not a specified pattern
finds `allow_any_instance_of` instead of an instance double
finds `expect_any_instance_of` instead of an instance double
flags nested subject stubs when nested subject uses same name
autocorrects `before(:example) { }` to `before(:example) { }`
finds old `any_instance` syntax instead of an instance double
flags hashes containing only literal values within expect(...)
flags ranges containing only literal values within expect(...)
flags all rspec example blocks with that include `focus: true`
flags arrays containing only literal values within expect(...)
flags regexps containing only literal values within expect(...)
rewrites "should search the internet" as "searches the internet"
does not register an offense for `eql` when argument is a string
complains when let! is used and not referenced within nested group
does not flag complex values with dynamic parts within expect(...)
does not register an offense for `eql` when expectation is negated
registers offenses when the path matches a custom specified pattern
rewrites "shouldn't return something" as "does not return something"
flags nested subject stubs when example group does not define subject
rewrites "should not return something" as "does not return something"
rewrites "should worry about the future" as "worries about the future"
autocorrects `it "should only fail" do end` to `it "onlies fail" do end`
autocorrects `it "should have trait" do end` to `it "haves trait" do end`
autocorrects `it { should be_truthy }` to `it { is_expected.to be_truthy }`
autocorrects `it { is_expected.to be_truthy }` to `it { should be_truthy }`
autocorrects `it { expect(foo).to eql(1) }` to `it { expect(foo).to be(1) }`
rewrites "should really only return one item" as "really only returns one item"
autocorrects `it "should only have trait" do end` to `it "only has trait" do end`
autocorrects `it { should_not be_truthy }` to `it { is_expected.to_not be_truthy }`
autocorrects `it { expect(0).to_not equal 1 }` to `it { expect(0).not_to equal 1 }`
autocorrects `it { is_expected.to_not be_truthy }` to `it { should_not be_truthy }`
autocorrects `it { expect(0).not_to equal 1 }` to `it { expect(0).to_not equal 1 }`
autocorrects `it { is_expected.not_to be_truthy }` to `it { should_not be_truthy }`
autocorrects `describe(Foo) { include Foo }` to `describe(Foo) { include described_class }`
autocorrects `describe(Foo) { before { Foo.do_something } }` to `describe(Foo) { before { described_class.do_something } }`
autocorrects `describe(Foo) { subject { Foo.do_something } }` to `describe(Foo) { subject { described_class.do_something } }`

some of the big ones have description strings we interpolate values into so rubocop-rspec wouldn't see these as big.

Regarding the conjunction solution I mentioned, here are matching descriptions from our project:

has assertions on lines 1 and 2
has a colon and a whitespace at the end
flags string and symbol literal values within expect(...)
complains when let! is used and not referenced
complains when let! is used and not referenced within nested group
finds multiple top level describes with class and method
checks before and after for explicit subject usage
has assertions on lines 1 and 2
has a colon and a whitespace at the end
flags string and symbol literal values within expect(...)
complains when let! is used and not referenced
complains when let! is used and not referenced within nested group
finds multiple top level describes with class and method
checks before and after for explicit subject usage
andyw8 commented 7 years ago

Nice idea about parsing for conjunctions but i think that may be difficult to get right.

I didn't consider whether it's inside of a context or not – perhaps this could be a future enhancement.

I think a reasonable default to start with would be 80 characters. The intention is to catch the extremes. The only rubocop-rspec examples which violate this are the ones containing code samples, which wouldn't be a concern for most projects.

pirj commented 5 years ago

I'm looking at the example docstrings John provided, from the longest, and they fall into several categories:

  1. Longest, verbose
    autocorrects `it "should only fail" do end` to `it "onlies fail" do end`

    I personally can't figure out why it belongs to the docstring in the first place, as it doesn't add much over the expectation in the example itself, which presumably is:

    include_examples 'autocorrect',
    'it "should only fail" do end',
    'it "onlies fail" do end'

And don't think this rule is applicable to generated docstrings e.g.

it %(rewrites "#{old}" as "#{new}") do
  1. The ones with "when", "with", "without", and "for" that are extractable into a context. Don't think it makes sense to advise to extract to context an example that would be the only example in the context just to make example docstrings shorter.

  2. "finds expect_any_instance_of instead of an instance double" sounds totally legit, and is 60 characters long.

WDYT of relaxing this to 60, and adding notes that it doesn't apply to cases when

the example will be the only one in this context (except the case when docstring is significantly longer than 60) and generated docstrings ?

Side observation:

"onlies fail"

doesn't sound right.

bquorning commented 5 years ago

"onlies fail"

doesn't sound right.

The same goes for

"haves trait"

Both test the context “when configuration is empty”, which I guess is expected to give weird results.