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

Improve RSpec/AggregateExamples cop #170

Merged
merged 1 commit into from Feb 9, 2020

Conversation

pirj
Copy link
Contributor

@pirj pirj commented Jan 29, 2020

What is the purpose of this pull request?

Improve RSpec/AggregateExamples cop (formerly RSpec/AggregateFailures).
Name change is semantic, since what cop suggests is to aggregate examples, and the feature of RSpec that allows for doing this painlessly is known as Aggregate Failures, but it's not directly related to what the cop does.

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. Test only public interface!

Matchers with side effects might affect subsequent expectations when aggregated. A warning is added to the offence.

What changes did you make? (overview)

  • significantly improved its support;
  • fixed --auto-correct showing all detected offenses as corrected;
  • fixed offense detection of matchers with side effects with qualifiers;
  • removed redundant spec coverage;
  • divided the code into logical parts (more on that below), that allowed removing disabling of Metrics/ClassLength on the cop;
  • extracted specs for its and matchers with side effects to dedicated spec files;
  • ... and much more!

Is there anything you'd like reviewers to focus on?

This cop was forked from RSpec/AggregateFailures and developed internally at @toptal (and part of that work was generously sponsored by them), but it might have diverged since then.
I've overwritten the existing cop, so please pay close attention to features that might have been added after forking, and happen to miss from this implementation.

The cop was originally submitted to RuboCop RSpec (rubocop/rubocop-rspec#726) but rejected due to its primary focus on spec performance, and also its unwieldiness.
There are some nice comments there.

This rubocop/rubocop-rspec#726 (comment) highlights some of the design decisions.

Not Done

The cop seems to be more or less feature-complete and bug-free. It was tested and regularly used on a 1M+ LoC codebase for almost a year. All edge cases and feedback found its way in the cop.
However, some of the things might be surprising, like lack of auto-correction for the block expectation syntax, unsafe matchers, and impaired document formatter output due to removals of example description along correction.

  1. For the latter, example descriptions, there is a solution used to extract expectations to matchers:
def be_cool
  be_cold
    .and be_fresh
end

Native RSpec's composing capabilities will provide a proper description for failures and for example doc.

  1. Not compounded by default
is_expected.to be_cold
is_expected.to be_fresh

are not automatically aggregated to a compound expectation even though it could:

is_expected
  .to be_cold
  .and be_fresh

There's a space for improving this, or a dedicated cop (probably more generic and less performance-focused) could do that. The compoind notation allows extracting parts of expectation to methods without sacrificing self-describability.

  1. It would be possible to extract UnsafeMatchers cop, but there are edge cases of using several cops cross-concerns that arise with that extraction.
    Some thoughts on this topic Add a cop to enforce aggregating examples rubocop/rubocop-rspec#726 (comment) Add a cop to enforce aggregating examples rubocop/rubocop-rspec#726 (comment)

  2. Found it in the last moment.

Add an example where a variable/method is an argument to its:

its(something) { is_expected.to ... }

I'm not certain if all those cases can/should be auto-corrected to

expect(subject.public_send(something)).to ...

This can be tackled in the future. Meanwhile added a spec to ignore this.

Checklist

  • I've added tests for this change
  • I've added a Changelog entry
  • I've updated a documentation
  • Leftover: remove unnecessary parts of language.rb
  • all_expectations? can be rewritten
  • I've squashed commits

lib/test_prof/factory_bot.rb Outdated Show resolved Hide resolved
# # - and arbitrary type: `hash[element1, element2, ...]`
# # It is impossible to infer the type to propose a proper correction.
#
# its(['ambiguous', 'elements']) { ... }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This dig auto-correction (in action in the previous implementation) seems to be an incorrect correction for cases outside arrays and hashes.

# Works for nested arrays and hashes
expect(subject.dig('ambiguous', 'elements')).to eq [100, 100]
"sdds"[1, 2] # => "dd"
"sdds".dig(1, 2) # NoMethodError: undefined method `dig' for "sdds":String

The following passes:

require 'rspec/its'
RSpec.describe do
  subject { 'qwer' }
  its([1,2]) { is_expected.to eq 'we' }
end

@pirj pirj changed the title ### What is the purpose of this pull request? Improve RSpec/AggregateExamples cop Jan 30, 2020
@pirj

This comment has been minimized.

Copy link
Contributor Author

@pirj pirj left a comment

Choose a reason for hiding this comment

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

I'm going to code review it myself then!

lib/test_prof/cops/rspec/aggregate_examples/its.rb Outdated Show resolved Hide resolved
lib/test_prof/cops/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
spec/test_prof/cops/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/test_prof/cops/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/test_prof/cops/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/test_prof/cops/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@palkan palkan left a comment

Choose a reason for hiding this comment

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

Awesome!

Left a few comments.

My main concern is backward compatibility. Can we add an alias cop with the old name and mark it as deprecated (to be removed in v1.0)?

lib/test_prof/cops/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/test_prof/cops/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
@pirj
Copy link
Contributor Author

pirj commented Feb 8, 2020

Rearranged includes indicating their purposes.
Added back runtime RuboCop version check.
Moved default config injection code to cops/inject.

Kept AggregateFailures as an alias of AggregateExamples with a warning on usage. Tried to use RuboCop's obsoletion for that, but it's hard to inject there, and a bit scary, since it doesn't look like something that is expected to be played with and may be a subject to change.

Appreciate if you take another look @palkan

Copy link
Collaborator

@palkan palkan left a comment

Choose a reason for hiding this comment

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

Perfect!

Let's squash and merge 😈

include MetadataHelpers
include NodeMatchers

# Methods from the following modules override and extend methods of this
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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.

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. Test only public interface!

Matchers with side effects might affects following expectations when aggregated.

Originally submitted as rubocop/rubocop-rspec#726
@pirj
Copy link
Contributor Author

pirj commented Feb 9, 2020

Squashed and green 👹 🙌

BTW for the not done: I've omitted "Eat your own cat food" commit, since for some reason unless you specify Enable: true for the cop in .rubocop.yml explicitly, it's disabled. When I plug it into another project, it works without explicit enabling.
Also, just two minor offences were found ¯\_(ツ)_/¯

@palkan palkan merged commit db5afe1 into test-prof:master Feb 9, 2020
@pirj pirj deleted the aggregate-examples-cop branch February 26, 2020 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants