Skip to content

Commit

Permalink
Add a cop to enforce aggregating examples
Browse files Browse the repository at this point in the history
The "one expectation per example" rule has been relaxed and allows for
several expectations to be set in the same example.
https://github.com/rubocop-hq/rspec-style-guide#expectations-per-example

In cases the examples don't have any setup, metadata, or even a
docstring, and may be aggregated into one thus saving on sometimes
expensive context setup.

The cop still does report the cases when the examples might be
aggregated, but with some risk, e.g. when matchers with side effects are
used. It makes sense to do so, since eventually those matchers might be
fixed to leave the subject in its original state.

Block expectation syntax is deliberately not supported due to:
 - `subject { -> { ... } }` syntax being hard to detect
 - aggregation should use composition with `.and`
 - aggregation of the `not_to` is barely possible when a matcher doesn't
 provide a negated variant
 - aggregation of block syntax with non-block syntax should be in a
 specific order

Known caveats:
The usages if `its` that are testing private methods/readers will result
in spec failure. It's up to the user whether to replace with `__send__`,
or test only public interface.

Original idea and initial implementation:
https://github.com/palkan/test-prof/blob/master/lib/test_prof/cops/rspec/aggregate_failures.rb
  • Loading branch information
pirj committed Jan 9, 2019
1 parent d84e29c commit e3ec166
Showing 7 changed files with 975 additions and 0 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@
## Master (Unreleased)

* Add `RSpec/Yield` cop, suggesting using the `and_yield` method when stubbing a method, accepting a block. ([@Darhazer][])
* Add `RSpec/AggregateExamples` cop. ([@pirj][], [@palkan][])

## 1.31.0 (2019-01-02)

@@ -401,3 +402,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@RST-J]: https://github.com/RST-J
[@ypresto]: https://github.com/ypresto
[@mkenyon]: https://github.com/mkenyon
[@palkan]: https://github.com/palkan
5 changes: 5 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
@@ -10,6 +10,11 @@ AllCops:
- spec/factories/**/*.rb
- features/support/factories/**/*.rb

RSpec/AggregateExamples:
Description: Checks if example groups contain more than one aggregateable example.
Enabled: false
StyleGuide: http://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/AggregateExamples

RSpec/AlignLeftLetBrace:
Description: Checks that left braces for adjacent single line lets are aligned.
Enabled: false
294 changes: 294 additions & 0 deletions lib/rubocop/cop/rspec/aggregate_examples.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,294 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpec
# Checks if example groups contain more than one aggregateable example.
#
# @example
#
# # bad
# describe do
# specify do
# expect(number).to be_positive
# expect(number).to be_odd
# end
#
# it { is_expected.to be_prime }
# end
#
# # good
# describe do
# specify do
# expect(number).to be_positive
# expect(number).to be_odd
# is_expected.to be_prime
# end
# end
#
# # fair - subject has side effects
# describe do
# specify do
# expect(multiply_by(2)).to be_multiple_of(2)
# end
#
# specify do
# expect(multiply_by(3)).to be_multiple_of(3)
# end
# end
#
# # The following example will fail if aggregated due to the side
# # effects of the `validate_presence_of` matcher as it leaves an empty
# # comment after itself on the subject making it invalid and the
# # subsequent expectation to fail.
#
# # bad, but should not be automatically correctable
# describe do
# it { is_expected.to validate_presence_of(:comment) }
# it { is_expected.to be_valid }
# end
#
# # Block expectation syntax is deliberately not supported due to:
# # 1. `subject { -> { ... } }` syntax being hard to detect
# # E.g.:
# it { is_expected.to do_something }
# # looks like an example with non-block syntax, but it might be
# # depending on how the subject is defined. If the subject is defined
# # in a `shared_context`, it's impossible to detect that at all.
# #
# # 2. Aggregation should use composition with an `.and`. Also,
# # aggregation of the `not_to` expectations is barely possible when a
# # matcher doesn't provide a negated variant.
# #
# # 3. Aggregation of block syntax with non-block syntax should be in a
# # specific order.
#
class AggregateExamples < Cop # rubocop:disable Metrics/ClassLength
include RangeHelp

MSG = 'Aggregate with the example above.'.freeze
MSG_FOR_EXPECTATIONS_WITH_SIDE_EFFECTS =
"#{MSG} IMPORTANT! Pay attention to the expectation order, some " \
'of the matchers have side effects.'.freeze

MATCHERS_WITH_SIDE_EFFECTS = %w[
:validate_presence_of
:validate_absence_of
:validate_length_of
:validate_inclusion_of
:validates_exclusion_of
].freeze

def on_block(node)
example_group_with_several_examples(node) do |all_examples|
examples = aggregateable_examples(all_examples)
next if examples.count < 2

message = message_for(examples)
add_offense(examples[1], location: :expression, message: message)
end
end

def autocorrect(example_node)
node = example_node.parent
examples = aggregateable_examples(node.each_child_node(:block))
.select { |child| examples_for_autocorrect?(child) }
return if examples.count < 2

replacement = aggregated_example(examples)
lambda do |corrector|
range = range_for_replace(examples)
corrector.replace(range, replacement)
examples[1..-1].each { |example| drop_example(corrector, example) }
end
end

private

def_node_matcher :example_group_with_several_examples, <<-PATTERN
(block
(send { nil? (const nil? :RSpec) } { :describe :context :feature :example_group } ...)
_
(begin $...)
)
PATTERN

def aggregateable_examples(examples)
examples
.reject { |example| has_metadata?(example) }
.select { |example| example_with_expectations_solely?(example) }
end

def range_for_replace(examples)
range = range_by_whole_lines(examples.first.source_range,
include_final_newline: true)
next_range = range_by_whole_lines(examples[1].source_range)
range = range.resize(range.length + 1) if adjacent?(range, next_range)
range
end

def adjacent?(range, another_range)
range.end_pos + 1 == another_range.begin_pos
end

def aggregated_example(examples)
base_indent = ' ' * examples.first.source_range.column
[
"#{base_indent}specify do",
*examples.map { |example| transform_body(example, base_indent) },
"#{base_indent}end\n"
].join("\n")
end

def drop_example(corrector, example)
aggregated_range = range_by_whole_lines(example.source_range,
include_final_newline: true)
corrector.remove(aggregated_range)
end

# Extracts and transforms the body.
# `its(:something) { is_expected.to ... }` is a special case, since
# it's impossible to aggregate its body as is,
# it needs to be converted to `expect(subject.something).to ...`
# Additionally indents the example code properly.
def transform_body(node, base_indent)
method, _args, body = *node
_receiver, method_name, arguments = *method
new_body = if method_name == :its
transform_its(body, arguments)
else
body.source
end
"#{base_indent} #{new_body}"
end

def transform_its(body, arguments)
property = arguments.children.first
body.source.gsub(/is_expected|are_expected/,
"expect(subject.#{property})")
end

def message_for(examples)
if examples.any? { |example| example_with_side_effects?(example) }
MSG_FOR_EXPECTATIONS_WITH_SIDE_EFFECTS
else
MSG
end
end

def example_method?(method_name)
%i[it specify example scenario].include?(method_name)
end

# Checks if the example:
# - exclusively contains expectation statements
# - has no metadata (e.g. `freeze: :today`)
def_node_matcher :example_without_metadata_and_expectations_solely?,
<<-PATTERN
[#example_with_expectations_solely? !#has_metadata?]
PATTERN

def_node_matcher :example_with_expectations_solely?, <<-PATTERN
(block #example_block? _
{ #single_expectation? #all_expectations? }
)
PATTERN

# Checks if an example has metadata (e.g. `freeze: :today`)
def_node_matcher :has_metadata?, <<-PATTERN
(block
{
(send nil? #example_method? str _ ...)
(send nil? #example_method? !str ...)
}
...
)
PATTERN

# Matchers examples with:
# - expectation statements exclusively
# - no metadata (e.g. `freeze: :today`)
# - no title (e.g. `it('jumps over the lazy dog')`)
# and skips `its` with an array argument due to ambiguous conversion
# e.g. the SUT can be an object (`expect(object.property)`)
# or a hash/array (`expect(hash['property'])`)
# and also skips matchers with known side-effects
def_node_matcher :examples_for_autocorrect?, <<-PATTERN
[
#example_without_metadata_and_expectations_solely?
!#example_has_title?
!#its_with_array_argument?
!#contains_heredoc?
!#example_with_side_effects?
]
PATTERN

# Matchees the example with a title (e.g. `it('is valid')`)
def_node_matcher :example_has_title?, <<-PATTERN
(block
(send nil? #example_method? str ...)
...
)
PATTERN

def_node_matcher :its_with_array_argument?, <<-PATTERN
(block (send nil? :its array) ...)
PATTERN

# Searches for HEREDOC in examples. It can be tricky to aggregate,
# especially when interleaved with parenthesis or curly braces.
def contains_heredoc?(node)
return unless node.is_a? AST::Node
return true if (node.dstr_type? || node.str_type?) && node.heredoc?

node.children.compact.any? { |child| contains_heredoc?(child) }
end

def_node_matcher :subject_with_no_args?, <<-PATTERN
(send _ _)
PATTERN

def_node_matcher :expectation?, <<-PATTERN
{
(send nil? :is_expected)
(send nil? :expect #subject_with_no_args?)
}
PATTERN

# Matches the matcher with side effects
def_node_matcher :matcher_with_side_effects?, <<-PATTERN
(send nil? { #{MATCHERS_WITH_SIDE_EFFECTS.join(' ')} } ...)
PATTERN

# Matches the expectation with matcher with side effects
def_node_matcher :expectation_with_side_effects?, <<-PATTERN
(send #expectation? { :to :to_not :not_to } #matcher_with_side_effects?)
PATTERN

# Matches the example with matcher with side effects
def_node_matcher :example_with_side_effects?, <<-PATTERN
(block #example_block? _ #expectation_with_side_effects?)
PATTERN

def all_expectations?(node)
return unless node && node.begin_type?

node.children.all? { |statement| single_expectation?(statement) }
end

def_node_matcher :single_expectation?, <<-PATTERN
(send #expectation? { :to :to_not :not_to } _)
PATTERN

# Matches example block
def_node_matcher :example_block?, <<-PATTERN
(send nil? { :it :specify :example :scenario :its } ...)
PATTERN

def_node_matcher :example_node?, <<-PATTERN
(block #example_block? ...)
PATTERN
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rspec_cops.rb
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@
# Rails/HttpStatus cannot be loaded if rack/utils is unavailable.
end

require_relative 'rspec/aggregate_examples'
require_relative 'rspec/align_left_let_brace'
require_relative 'rspec/align_right_let_brace'
require_relative 'rspec/any_instance'
1 change: 1 addition & 0 deletions manual/cops.md
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@

#### Department [RSpec](cops_rspec.md)

* [RSpec/AggregateExamples](cops_rspec.md#rspecaggregateexamples)
* [RSpec/AlignLeftLetBrace](cops_rspec.md#rspecalignleftletbrace)
* [RSpec/AlignRightLetBrace](cops_rspec.md#rspecalignrightletbrace)
* [RSpec/AnyInstance](cops_rspec.md#rspecanyinstance)
Loading

0 comments on commit e3ec166

Please sign in to comment.