Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a cop to enforce aggregating examples #726

Closed
wants to merge 56 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
916eb9b
Add a cop to enforce aggregating examples
pirj Jan 9, 2019
4ad3e68
Clarify the purpose of the cop
pirj Jan 9, 2019
4aa83a9
Mention aggregate_failures in the documentation
pirj Jan 9, 2019
0adfd01
Extract matchers with side effects to configuration
pirj Jan 9, 2019
e2a5b18
Use Language module
pirj Jan 9, 2019
a9b2983
Use navigation instead of manual deconstruction
pirj Jan 9, 2019
5e3a001
Simplify heredoc detection
pirj Jan 10, 2019
b2c86c1
Update manual
pirj Jan 10, 2019
3a756e2
Fix docstring
pirj Jan 10, 2019
00f99ce
Aggregate examples with identical metadata
pirj Jan 10, 2019
e32b9a6
fixup! Convert to symbol for proper comparison
pirj Jan 15, 2019
4dad154
Add config option to add aggregate_failures metadata
pirj Jan 15, 2019
dd47c96
Fix documentation issues
pirj Jan 15, 2019
9928939
fixup! Fix doc formatting
pirj Jan 15, 2019
b9de045
fixup! Simplify node patter by reusing the existing helper
pirj Jan 31, 2019
3d5f62a
fixup! Use node properties instead of a node pattern
pirj Jan 31, 2019
9655c29
fixup! Simplify heredoc detection and fix no-autocorrect shared example
pirj Jan 31, 2019
b2d5e60
fixup! Extract config option from Style
pirj Jan 31, 2019
ef3382a
fixup! Fix typo
pirj Feb 6, 2019
0c04a79
Apply suggestions from code review
bquorning Mar 15, 2019
e394a6a
Get rid of a shared example
pirj Mar 10, 2019
a7510b9
Get rid of another shared example
pirj Mar 10, 2019
46553f1
Simplify shared example by aggregating it
pirj Mar 10, 2019
458fbf3
Change implicit subject to explicit in code examples
pirj Mar 19, 2019
81c81c1
Group examples related to matchers with side effects
pirj Mar 19, 2019
1e740ab
Add line numbers to offenses, indicate all offenses
pirj Mar 19, 2019
318c77c
Remove redundant good source checks
pirj Mar 19, 2019
8906bf3
Add details to a mixed side-effect matchers example
pirj Mar 19, 2019
2a93dfd
Simplify handling of aggregate_failures
pirj Apr 1, 2019
4b0b0cc
Remove excessive test coverage
pirj Apr 1, 2019
367c7a6
Fix some typos and add clarification to examples
pirj Apr 1, 2019
536fb90
Add support for its' are expected
pirj Apr 7, 2019
5745c03
Clarify the reasoning behind not supporting array its syntax
pirj Apr 7, 2019
df23cf5
Remove excessive test coverage
pirj Apr 7, 2019
3522224
Add support for single-element array `its` argument
pirj Apr 7, 2019
c38d4f6
Add support for `its` with metadata
pirj Apr 7, 2019
c0f67ec
Remove redundant docstring
pirj Apr 7, 2019
a1ebfad
Extract `its` support to a module
pirj Apr 7, 2019
0263d7f
Flip filter condition to improve readability
pirj Apr 7, 2019
3e0fbfc
Fix outdated doc for autocorrect examples matcher
pirj Apr 7, 2019
30e3463
Extract matchers with side effects support
pirj Apr 8, 2019
b133213
Fix typo, remove redundant comment and redundant matcher
pirj Apr 9, 2019
38b1e73
Extract metadata and line ranges
pirj Apr 9, 2019
82df911
Simplify node pattern
pirj Apr 11, 2019
9d613a7
Detect side effect matcher with qualifiers
pirj Apr 13, 2019
aa2fd4a
Do not pretend to correct uncorrectable offenses
pirj Apr 13, 2019
0237824
Extract autocorrect clustering method
pirj Apr 13, 2019
d6c2c37
Fix spelling mistake
pirj Apr 14, 2019
9e00508
[DO NOT SQUASH] Add support for adding docs from modules
pirj Apr 14, 2019
f1c0a23
Adjust docs to new parsing schema
pirj Apr 14, 2019
ad16b40
Move included modules require to upfront
pirj Apr 14, 2019
abe9f0e
Make internally used methods private
pirj Apr 14, 2019
2bba4e4
Extract node matchers into separate module
pirj Apr 14, 2019
6c3afc1
Extract aggregation logic
pirj Apr 15, 2019
a5542f5
Add code documentation
pirj Apr 15, 2019
bfbe3f2
Fix documentation for `its` support
pirj Apr 16, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* Fixed `RSpec/Focus` not flagging some cases of `RSpec.describe` with `focus: true`. ([@Darhazer][])
* Fixed `RSpec/Pending` not flagging some cases of `RSpec.describe` with `:skip`. ([@Darhazer][])
* Fix false positive in `RSpec/ReceiveCounts` when method name `exactly`, `at_least` or `at_most` is used along with `times`, without being an RSpec API. ([@Darhazer][])
* Add `RSpec/AggregateExamples` cop. ([@pirj][], [@palkan][])

## 1.31.0 (2019-01-02)

Expand Down Expand Up @@ -414,3 +415,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@mkenyon]: https://github.com/mkenyon
[@gsamokovarov]: https://github.com/gsamokovarov
[@schmijos]: https://github.com/schmijos
[@palkan]: https://github.com/palkan
14 changes: 14 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ AllCops:
- spec/factories/**/*.rb
- features/support/factories/**/*.rb

RSpec/AggregateExamples:
Description: Checks if example groups contain two or more aggregatable examples.
Enabled: false
StyleGuide: http://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/AggregateExamples
AddAggregateFailuresMetadata: false
MatchersWithSideEffects:
pirj marked this conversation as resolved.
Show resolved Hide resolved
- allow_value
- allow_values
- validate_presence_of
- validate_absence_of
- validate_length_of
- validate_inclusion_of
- validates_exclusion_of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this list come from? They look a bit like Rails validators, but they include an s on “validates”, and not all are listed:

  • validates_absence_of
  • validates_acceptance_of
  • validates_associated
  • validates_confirmation_of
  • validates_exclusion_of
  • validates_format_of
  • validates_inclusion_of
  • validates_length_of
  • validates_numericality_of
  • validates_presence_of
  • validates_size_of
  • validates_uniqueness_of

Where does allow_value and allow_values come from?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, they are Shoulda matchers, aren’t they? I’m not sure how I feel about Shoulda sneaking into our default configuration. Not all rubocop-rspec users use Shoulda, let alone Rails.

Copy link
Member Author

@pirj pirj Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but honestly, I'm not sure how to make this safe and not tiresome to configure by default at the same time.

A couple of arguments for keeping this in the default config:

  • this is a generic cop, but it also supports Rails. An option to make UsingShouldaMatchers configurable and off by default, but what if side effects are eventually fixed in some later version of shoulda-matchers?
  • shoulda-matchers also support non-Rails apps that use ActiveModel or ActiveRecord
  • we already have Rails/HttpStatus in our default config, which is for one of the few rspec-rails matchers. Do you think there's a critical mass already to extract those to a separate gem?


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

require_relative 'support/aggregate_examples/aggregation'
require_relative 'support/aggregate_examples/its'
require_relative 'support/aggregate_examples/line_range'
require_relative 'support/aggregate_examples/matchers_with_side_effects'
require_relative 'support/aggregate_examples/metadata'
require_relative 'support/aggregate_examples/node_matchers'

module RuboCop
module Cop
module RSpec
# Checks if example groups contain two or more aggregatable examples.
#
# @see https://github.com/rubocop-hq/rspec-style-guide#expectations-per-example
#
# This cop is primarily for reducing the cost of repeated expensive
# context initialization.
#
# @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
pirj marked this conversation as resolved.
Show resolved Hide resolved
# 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
#
# Block expectation syntax is deliberately not supported due to:
#
# 1. `subject { -> { ... } }` syntax being hard to detect, e.g. the
# following looks like an example with non-block syntax, but it might
# be, depending on how the subject is defined:
#
# it { is_expected.to do_something }
#
# 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.
#
# RSpec [comes with an `aggregate_failures` helper](https://relishapp.com/rspec/rspec-expectations/docs/aggregating-failures)
# not to fail the example on first unmet expectation that might come
# handy with aggregated examples.
# It can be [used in metadata form](https://relishapp.com/rspec/rspec-core/docs/expectation-framework-integration/aggregating-failures#use-%60:aggregate-failures%60-metadata),
# or [enabled globally](https://relishapp.com/rspec/rspec-core/docs/expectation-framework-integration/aggregating-failures#enable-failure-aggregation-globally-using-%60define-derived-metadata%60).
#
# @example Globally enable `aggregate_failures`
#
# # spec/spec_helper.rb
# config.define_derived_metadata do |metadata|
# unless metadata.key?(:aggregate_failures)
# metadata[:aggregate_failures] = true
# end
# end
#
# To match the style being used in the spec suite, AggregateExamples
# can be configured to add `:aggregate_failures` metadata to the
# example or not. The option not to add metadata can be also used
# when it's not desired to make expectations after previously failed
# ones, commonly known as fail-fast.
#
# The terms "aggregate examples" and "aggregate failures" not to be
# confused. The former stands for putting several expectations to
# a single example. The latter means to run all the expectations in
# the example instead of aborting on the first one.
#
# @example AddAggregateFailuresMetadata: false (default)
#
# specify do
# expect(number).to be_positive
# expect(number).to be_odd
# end
#
# @example AddAggregateFailuresMetadata: true
#
# # Metadata set using a symbol
# specify(:aggregate_failures) do
# expect(number).to be_positive
# expect(number).to be_odd
# end
#
class AggregateExamples < Cop
include Aggregation
prepend Its
include LineRange
prepend MatchersWithSideEffects
include Metadata
include NodeMatchers

MSG = 'Aggregate with the example at line %d.'.freeze

def on_block(node)
example_group_with_several_examples(node) do |all_examples|
example_clusters(all_examples).each do |_, examples|
examples[1..-1].each do |example|
add_offense(example,
location: :expression,
message: message_for(example, examples[0]))
end
end
end
end

def autocorrect(example_node)
clusters = example_clusters_for_autocorrect(example_node)
return if clusters.empty?

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

private

# Clusters of examples in the same example group, on the same nesting
# level that can be aggregated.
def example_clusters(all_examples)
all_examples
.select { |example| example_with_expectations_only?(example) }
.group_by { |example| metadata_without_aggregate_failures(example) }
.select { |_, examples| examples.count > 1 }
end

# Clusters of examples that can be aggregated without losing any
# information (e.g. metadata or docstrings)
def example_clusters_for_autocorrect(example_node)
examples_in_group = example_node.parent.each_child_node(:block)
.select { |example| example_for_autocorrect?(example) }
example_clusters(examples_in_group)
end

def message_for(_example, first_example)
MSG % first_example.loc.line
end
end
end
end
end
53 changes: 53 additions & 0 deletions lib/rubocop/cop/rspec/support/aggregate_examples/aggregation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
module RuboCop
module Cop
module RSpec
class AggregateExamples < Cop
# @internal
# Aggregation helpers.
module Aggregation
private

def drop_example(corrector, example)
aggregated_range = range_by_whole_lines(example.source_range,
include_final_newline: true)
corrector.remove(aggregated_range)
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)
if adjacent?(range, next_range)
range.resize(range.length + 1)
else
range
end
end

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

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

# Extracts and transforms the body, keeping proper indentation.
def transform_body(node, base_indent)
"#{base_indent} #{new_body(node)}"
end

def new_body(node)
node.body.source
end
end
end
end
end
end
89 changes: 89 additions & 0 deletions lib/rubocop/cop/rspec/support/aggregate_examples/its.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
module RuboCop
module Cop
module RSpec
class AggregateExamples < Cop
# @example `its`
#
# # Supports regular `its` call with an attribute/method name,
# # or a chain of methods expressed as a string with dots.
#
# its(:one) { is_expected.to be(true) }
# its('two') { is_expected.to be(false) }
# its('phone_numbers.size') { is_expected.to be 2 }
#
# @example `its` with single-element array argument
#
# # Also supports single-element array argument.
#
# its(['headers']) { is_expected.to include(encoding: 'text') }
#
# @example `its` with multi-element array argument is ambiguous
#
# # Does not support `its` with multi-element array argument due to
# # an ambiguity. Transformation depends on the type of the subject:
# # - a Hash: `hash[element1][element2]...`
# # - and arbitrary type: `hash[element1, element2, ...]`
# # It is impossible to infer the type to propose a proper correction.
#
# its(['ambiguous', 'elements']) { ... }
#
# @example `its` with metadata
#
# its('header', html: true) { is_expected.to include(text: 'hello') }
#
module Its
extend RuboCop::NodePattern::Macros

private

# It's impossible to aggregate `its` body as is, it needs to be
# converted to `expect(subject.something).to ...`
def new_body(node)
return super unless its?(node)

transform_its(node.body, node.send_node.arguments)
end

def transform_its(body, arguments)
argument = arguments.first
replacement = case argument.type
when :array
key = argument.values.first
"expect(subject[#{key.source}])"
else
property = argument.value
"expect(subject.#{property})"
end
body.source.gsub(/is_expected|are_expected/, replacement)
end

def example_metadata(example)
return super unless its?(example.send_node)

# First parameter to `its` is not metadata.
example.send_node.arguments[1..-1]
end

def its?(node)
node.method_name == :its
end

# In addition to base definition, matches examples with:
# - no `its` with an multiple-element array argument due to
# an ambiguity, when SUT can be a hash, and result will be defined
# by calling `[]` on SUT subsequently, e.g. `subject[one][two]`,
# or any other type of object implementing `[]`, and then all the
# array arguments are passed to `[]`, e.g. `subject[one, two]`.
def_node_matcher :example_for_autocorrect?, <<-PATTERN
[ #super !#its_with_multiple_element_array_argument? ]
PATTERN

def_node_matcher :its_with_multiple_element_array_argument?,
<<-PATTERN
(block (send nil? :its (array _ _ ...)) ...)
PATTERN
end
end
end
end
end
29 changes: 29 additions & 0 deletions lib/rubocop/cop/rspec/support/aggregate_examples/line_range.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module RuboCop
module Cop
module RSpec
class AggregateExamples < Cop
# @internal Support methods for keeping newlines around examples.
module LineRange
include RangeHelp

private

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)
if adjacent?(range, next_range)
range.resize(range.length + 1)
else
range
end
end

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