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

Conversation

@pirj
Copy link
Member

@pirj pirj 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.

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.

The 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.
Copy link
Contributor

@Darhazer Darhazer 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

Loading

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

@pirj pirj 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.

Loading

@pirj

This comment has been hidden.

@palkan

This comment has been hidden.

@pirj

This comment has been hidden.

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

This comment has been hidden.

@pirj

This comment has been hidden.

@pirj
Copy link
Member Author

@pirj pirj 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.

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jan 21, 2019

Loading

Copy link
Contributor

@Darhazer Darhazer 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 :)

Loading

config/default.yml Outdated Show resolved Hide resolved
Loading
config/default.yml Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Show resolved Hide resolved
Loading
@pirj pirj force-pushed the add-aggregate_examples-cop branch from b5296c0 to 3cd095a Jan 31, 2019
@pirj
Copy link
Member Author

@pirj pirj commented Jan 31, 2019

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

Loading

Copy link
Member Author

@pirj pirj 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.

Loading

spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
@pirj
Copy link
Member Author

@pirj pirj commented Feb 5, 2019

Bump.

Loading

Copy link
Collaborator

@bquorning bquorning 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.

Loading

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
@pirj
Copy link
Member Author

@pirj pirj 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.

Loading

@pirj pirj force-pushed the add-aggregate_examples-cop branch from 6083287 to 7c34013 Feb 6, 2019
@pirj
Copy link
Member Author

@pirj pirj 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?

Loading

@bquorning bquorning mentioned this pull request Mar 8, 2019
5 tasks
@pirj pirj force-pushed the add-aggregate_examples-cop branch from 7c34013 to dfdc73a Mar 8, 2019
@pirj
Copy link
Member Author

@pirj pirj commented Mar 8, 2019

Updated to use the Language::RUNNERS from master.

Loading

- validate_absence_of
- validate_length_of
- validate_inclusion_of
- validates_exclusion_of
Copy link
Collaborator

@bquorning bquorning Mar 8, 2019

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?

Loading

Copy link
Collaborator

@bquorning bquorning Mar 8, 2019

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.

Loading

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?

Loading

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
Loading
lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
@bquorning
Copy link
Collaborator

@bquorning bquorning 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 :-)

Loading

Copy link
Member Author

@pirj pirj left a comment

Thanks for the review!

Loading

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
- validate_absence_of
- validate_length_of
- validate_inclusion_of
- validates_exclusion_of
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?

Loading

lib/rubocop/cop/rspec/aggregate_examples.rb Outdated Show resolved Hide resolved
Loading
Copy link
Member Author

@pirj pirj left a comment

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

Loading

spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
spec/rubocop/cop/rspec/aggregate_examples_spec.rb Outdated Show resolved Hide resolved
Loading
@pirj pirj force-pushed the add-aggregate_examples-cop branch from d07d025 to 421add6 Apr 7, 2019
pirj added 15 commits Apr 24, 2019
Matchers with side effects with qualifiers were still aggregated, e.g.:

    is_expected.to allow_value('green').for(:color)
Previously, auto-correct run pretended it corrected the offenses, even
though they were not.
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 add-aggregate_examples-cop branch from b34933a to bfbe3f2 Apr 24, 2019
@pirj
Copy link
Member Author

@pirj pirj 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.

Loading

@bolshakov
Copy link

@bolshakov bolshakov 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

Loading

@bolshakov
Copy link

@bolshakov bolshakov 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

Loading

@pirj
Copy link
Member Author

@pirj pirj 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.

Loading

# @internal
# Support for taking special care of the matchers that have side
# effects, i.e. leave the subject in a modified stte.
module MatchersWithSideEffects
Copy link
Member Author

@pirj pirj Aug 17, 2019

Choose a reason for hiding this comment

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

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?

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Nov 26, 2019

@Darhazer

The main reason someone may use such a cop is to improve spec performance. This is good to be noted in the docs.

Agree, will add such a note.

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

I believe those two are mutually exclusive cops.

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

AggregateExamples then will suggest combining several examples into one, but MatcherWithSideEffects will suggest separating them.
How would AggregateExamples know which examples to ignore?
@dgollahon You had a perspective on cop responsibilities separation, what do you think about this case?

Or should I better keep this part in, but:

  • mark the cop as unsafe
  • remove shoulda-matchers' matchers from default conf and recommend adding them in the doc

WDYT? This one is hanging for a while, I'd love to move it forward or close if there's no option to merge.

Loading

@pirj pirj closed this Dec 5, 2019
pirj added a commit to pirj/test-prof that referenced this issue Jan 29, 2020
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 added a commit to pirj/test-prof that referenced this issue Feb 8, 2020
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 added a commit to pirj/test-prof that referenced this issue Feb 9, 2020
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 pirj deleted the add-aggregate_examples-cop branch Apr 13, 2020
@pirj
Copy link
Member Author

@pirj pirj commented Apr 13, 2020

Just in case if someone would like to use this, the cop has found a place in test-prof.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants