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

Open
wants to merge 56 commits into
base: master
from

Conversation

@pirj
Copy link
Member

commented Jan 9, 2019

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 by @palkan
https://github.com/palkan/test-prof/blob/master/lib/test_prof/cops/rspec/aggregate_failures.rb


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
@Darhazer
Copy link
Member

left a comment

Few thoughts:

  • The main reason someone may use such a cop is to improve spec performance. This is good to be noted in the docs.
  • This can easily lead to breaking the max example length or max expectations count. Should the cop be aware of those? Maybe the cop should wrap the example in aggregate_failures
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch 2 times, most recently from 76bc4a1 to e3ec166 Jan 9, 2019

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
@pirj

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

This can easily lead to breaking the max example length or max expectations count. Should the cop be aware of those?

Not sure what is considered "too many", and if there are more expectations than the limitation setting, how to divide them across the examples.
The referenced betterspecs says:

it's fine to specify more than one isolated behavior.

and does not dictate any limitation on the number of the expectations.
I would leave this up to Metrics/BlockLength.

Maybe the cop should wrap the example in aggregate_failures

Agree, it completely slipped my mind that it's not the default.

@pirj

This comment was marked as outdated.

Copy link
Member Author

commented Jan 9, 2019

However, if we would add metadata to the example, e.g.

specify aggregate_failures: true do

this example will no longer be detected as aggregatable as the others with the metadata.
I would prefer mentioning this in the docs, especially since I don't see a good reason not to aggregate failures by default.

@palkan

This comment was marked as resolved.

Copy link
Member

commented Jan 9, 2019

@pirj

if we would add metadata to the example

Btw, why did you decide not to add it (as in the original implementation)?

@pirj

This comment was marked as resolved.

Copy link
Member Author

commented Jan 10, 2019

@palkan On the project where this cop is mostly used, aggregate_failures is on globally with a few exceptions.
I'll add this to the documentation.

      config.define_derived_metadata do |metadata|
        metadata[:aggregate_failures] = true unless metadata.key?(:aggregate_failures)
      end

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch from e3ec166 to cb8a1d0 Jan 10, 2019

@palkan

This comment was marked as resolved.

Copy link
Member

commented Jan 10, 2019

On the project where this cop is mostly used, aggregate_failures is on globally with a few exceptions.

What do you think about adding a configuration parameter to add the tag automatically?

I'm not sure that enabling aggregate_failures globally fits everyone; sometimes it's better to fail fast)

So, there could be two strategies: fail-fast (default RSpec behaviour) + explicit aggregate_failures and global aggregation + in-place disabling. And for the first one it would be great to have an ability to add the tag automatically.

@pirj

This comment was marked as resolved.

Copy link
Member Author

commented Jan 13, 2019

Added aggregation of the examples with identical metadata.

@palkan Loving the configuration idea, will implement shortly.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@Darhazer @palkan
Added configuration option to add :aggregate_failures to metadata of the aggregated example.
Please take another look. Appreciate if you run it against the codebase of your choice.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

@Darhazer
Copy link
Member

left a comment

@bquorning I would like to know what do you think for having such a cop.

Since we are dealing with large spec suite and we have a lot of specs on some slow operations, I saw myself how useful an aggregation could be, especially if mixed with custom matchers to improve test readability.

But it is somehow specific, performance-related cop. That why it's disabled by default. It will be not the first disabled-by-default cop and I'm 🆗 with accepting it in the RuboCop-RSpec base. It's nice to see actually rubocop-rspec being used for more and more things and one day it may give birth to other sub-projects :)

config/default.yml Outdated Show resolved Hide resolved
config/default.yml Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch from b5296c0 to 3cd095a Jan 31, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

@Darhazer I believe I've addressed your concerns, appreciate another round of code review.
cc @bquorning @palkan

@pirj
Copy link
Member Author

left a comment

Added some self-criticism and highlighted things I'm not completely certain of.

One thing that isn't in the specs is a trailing newline, e.g. the example "with several examples separated by newlines". AFAIR, in case when the first two examples are aggregatable, while the third is not, and all are separated by newlines, aggregation leaves out two newlines between the first and second aggregated and the third one. I'll either need a hand with that, or we can disregard if the other cop is usually autocorrecting that.

spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
@pirj

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

Bump.

@bquorning
Copy link
Member

left a comment

@bquorning I would like to know what do you think for having such a cop.

Almost all of the specs use specify with implicit subject, e.g.

specify do
  is_expected.to be_positive
  is_expected.to be_enthusiastic
end

which is a style I rarely use. If I use it, it’s usually in unit tests, which are fast enough that I wouldn’t use aggregate_failures – at least not for performance reasons.

In the large projects I have been working on, we use aggregate_failures in the slower integration / full stack tests. But these tests are more explicit, and typically with a couple of “exercise” steps before each assertion. I don’t know if the proposed cop will be able to find candidates for aggregate_failures in such situations.

I think this cop may find its uses, but I agree that it should be disabled by default. But we probably need some more visibility into which disabled cops we have available, as otherwise they may never be used by anyone.

I am a bit concerned about the size and complexity of this cop. I haven’t yet managed to read and understand all the code… (shame on me I guess) But once it’s merged, there comes a certain responsibility for maintaining it.

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
@pirj

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@bquorning Thanks for the review.

Actually, explicit subject [is supported as well]
(https://github.com/rubocop-hq/rubocop-rspec/pull/726/files#diff-65c1681945efcb2fb8eb9ebe1afd5bd6R325) (spec):

      describe do
        it { expect(division.result).to eq(5) }
        it { expect(division.modulo).to eq(3) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Aggregate with the example above.
      end

I used implicit subject in examples to avoid additional subject(...) { ... } line.

I completely get your point, unit tests are already fast enough usually to aggregate them, however sometimes aggregation might be used to reduce the size of the spec file.

The usual suspect of this cop are integration tests, if the setup is shared across several examples, and it's only the expectations differ, those can actually be aggregated. In the codebase I'm mostly working on it's was a rather frequent case.

This started off relaxing the "one expectation per example" rule, as it wasn't that strict in betterspecs compared to rspec-style-guide, then active development of this cop, and I believe is helping to reduce the effort to change the code, since if some part of the functionality is extracted, fewer examples should be changed, and smaller the chances to leave some legacy examples in the code.

I feel responsible for it, and Maxim is familiar with the code as well, as he was doing a number of internal code reviews.

I really hope that the size of the cop will be reduced eventually as its node matchers are extracted when the other cops find those parts useful.

Let's pair and I will walk you through the code if you like.

I'm not sure what is the best approach to highlight the disabled cops, just mention them in the docs? Inject into project's .rubocop-todo.yml somehow? The enabled/disabled distinction looks way too strict, I would rather go for recommended/optional, and would ask users to explicitly disable even those that are optional.

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch from 6083287 to 7c34013 Feb 6, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@bquorning I think I understand what you mean concerning the disabling by default.
There was a convention regarding cop versioning and enabling previously introduced disabled cops in next minor versions.

There are different styles of tests, and for some, it might not be a good fit, as it pivots the way the tests are written. However, while applying it to a big codebase I didn't see any really big issues with it (apart from numerous that I have addressed already, e.g. deliberately skipped block syntax and matchers with side effects).

Amongst codebases I've seen, there were always a good amount of disabled cops.

On the other hand, AggregateExamples is mutually exclusive with MultipleExpectations. However, it might be perceived as a single cop with two styles (like MessageExpectation that has two opposite style options), but split. I don't consider joining the two together as a good option though, as the former has grown huge already.

That being said, do you think this cop should remain disabled forever, or until the next minor rubocop-rspec release?

@bquorning bquorning referenced this pull request Mar 8, 2019
2 of 5 tasks complete

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch from 7c34013 to dfdc73a Mar 8, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

Updated to use the Language::RUNNERS from master.

- validate_absence_of
- validate_length_of
- validate_inclusion_of
- validates_exclusion_of

This comment has been minimized.

Copy link
@bquorning

bquorning Mar 8, 2019

Member

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?

This comment has been minimized.

Copy link
@bquorning

bquorning Mar 8, 2019

Member

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.

This comment has been minimized.

Copy link
@pirj

pirj Mar 15, 2019

Author Member

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?
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
@bquorning

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

I’ve read most of the code now, but as mentioned above I’m still struggling with the actual cop implementation. It may well be that I just need to pull myself together and focus :-)

@pirj
Copy link
Member Author

left a comment

Thanks for the review!

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
- validate_absence_of
- validate_length_of
- validate_inclusion_of
- validates_exclusion_of

This comment has been minimized.

Copy link
@pirj

pirj Mar 15, 2019

Author Member

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?
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
@pirj
Copy link
Member Author

left a comment

Thanks for the review again!
Added notes to self on what needs to be done.

spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch from d07d025 to 421add6 Apr 7, 2019

pirj added some commits Apr 7, 2019

Detect side effect matcher with qualifiers
Matchers with side effects with qualifiers were still aggregated, e.g.:

    is_expected.to allow_value('green').for(:color)
Do not pretend to correct uncorrectable offenses
Previously, auto-correct run pretended it corrected the offenses, even
though they were not.
[DO NOT SQUASH] Add support for adding docs from modules
Due to yard's override of docs when the docs are defined in several
files for the same class ([see this
issue](lsegal/yard#1173)), it's impossible to
spread the docs to several files so that they are compiled into one
document (e.g. a manual).

Comparison using:

    RuboCop::Cop::Badge.for(code_object.to_s) == cop.badge

makes it impossible to put the docs on the included module level without
filtering out those modules for which a badge can't be inferred, because
`RuboCop::Cop::Badge.for` raises an error.

@pirj pirj force-pushed the pirj:add-aggregate_examples-cop branch from b34933a to bfbe3f2 Apr 24, 2019

@pirj

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Updated with the latest master.
Please take a look.
I'm all for addressing your concerns, including making required systematic changes e.g. to how we recommend new cops.

@bolshakov

This comment has been minimized.

Copy link

commented Apr 25, 2019

@pirj could you explain how it's expected to aggregate these two examples:

subject { -> { do_side_effect } }

it 'has effect on this' do
  is_expected.to change {
    check_effect_this_way
  }.from(42).to(43)
end

it 'has effect on that' do
  is_expected.to change {
    check_effect_that_way
  }.from('foo').to('bar')
end
@bolshakov

This comment has been minimized.

Copy link

commented Apr 25, 2019

Consider I managed to aggregate two examples into one. Now I want to test the second one in isolation (because it has a bug). How to do this?

For me it's the same anti-pattern as iterationg overt tests like this

['foo', 'bar', 'baz'].each do |subject|
  test_it(subject)
end

because you can't run single example in isolation

@pirj

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@bolshakov Thanks for asking. Yes, that one would be easy:

def has_effect_on_this
  change { check_effect_this_way}
    .from(42)
    .to(43)
end

def has_effect_on_that
  change { check_effect_that_way }
    .from('foo')
    .to('bar')
end

it 'does this and that' do
  expect { do_side_effect }
    .to has_effect_on_this
    .and has_effect_on_that
end

You are testing side effects of calling do_side_effects. What do you mean by testing it in isolation, what would be the reason for that isolation? I'm not sure I correctly understand your question.
With aggregate_failures turned on, you will see all the expectation failures, even if there are several of them in the same example.

I'm not going to reason about to SRP, since the tests are usually just indicators of the production code flaws, and if the code does this and that doesn't necessarily mean that testing those two side effects is a benefit.

You might be interested in an open rspec-style-guide ticket to recommend against implicit block syntax.

The reasoning behind recommending against using iterators is different than lack of ability to test the expectations separately, it's:

When another developer adds a feature to one of the items in the iteration, they must then break it out into a separate test - they are forced to edit code that has nothing to do with their pull request.

@pirj pirj referenced this pull request May 16, 2019
# @internal
# Support for taking special care of the matchers that have side
# effects, i.e. leave the subject in a modified stte.
module MatchersWithSideEffects

This comment has been minimized.

Copy link
@pirj

pirj Aug 17, 2019

Author Member

I'm trying to reduce the scope of this pull request, and one of the things I plan to extract is a MatcherWithSideEffects cop that would prevent from using the matchers that leave the dirty subject after them together with the other matchers.

However, if I would remove this part from AggregateExamples, and it will suggest combining several examples into one, but MatcherWithSideEffects will suggest separating them.
I don't see that those two cops are in any way mutually exclusive in general, but this how would AggregateExamples know which examples to ignore?

@dgollahon You had a perspective on cop responsibilities separation, what do you think about this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.