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

Drop RSpec/EmptyExampleGroup customization #1007

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

pirj
Copy link
Member

@pirj pirj commented Aug 19, 2020

We're providing a flexible way of customizing RSpec syntax thanks to #956

This is not based on #956, but rather includes it (can't specify a base branch from another fork).
Those are the changes 42f7708
This is the change needed to avoid mutating the default config between the examples 718b398

This way it's possible to drop customization of this cop instead customize via .rubocop.yml configuration, i.e.:

AllCops:
  RSpec:
    Language:
      Includes:
        Example:
        - it_has_special_behavior

There's a problem with specs, though, specifically with this:

RuboCop::ConfigLoader.default_configuration.for_all_cops.dig('RSpec', 'Language')

for_all_cops is memoized in Config:

# rubocop-0.89.1/lib/rubocop/config.rb:132:

def for_all_cops
  @for_all_cops ||= self['AllCops'] || {}
end

It's not so obvious to notice, since in this cop the example that is modifying the language is in its own context, and it runs last.
Even if you add an example after it, due to how RSpec orders examples, it will still be executed last.
However, if you add a context with an example, you'll see that the language changes leaked.

We need to figure out a way to reset the config between examples, and do it in a performant way.
Thoughts? dup in before and assign back in after?

Fixes #969


Before submitting the PR make sure the following are checked:

  • [no] Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md 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 modified an existing cop's configuration options:

  • Set VersionChanged in default/config.yml to the next major version.

@pirj pirj self-assigned this Aug 19, 2020
@pirj pirj requested review from Darhazer and bquorning August 19, 2020 16:20
@pirj pirj added this to the 2.0 milestone Aug 19, 2020
@pirj pirj force-pushed the drop-empty-example-group-customization branch from 62dba1c to 3eb0340 Compare August 19, 2020 16:40
Copy link
Contributor

@sl4vr sl4vr left a comment

Choose a reason for hiding this comment

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

This might work

spec/shared/default_rspec_language_config_context.rb Outdated Show resolved Hide resolved
spec/shared/default_rspec_language_config_context.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/empty_example_group_spec.rb Outdated Show resolved Hide resolved
@pirj
Copy link
Member Author

pirj commented Aug 21, 2020

Found a way around. Please take a look.

@pirj pirj marked this pull request as ready for review August 21, 2020 10:36
@pirj pirj changed the title [draft] [please take a look] Drop RSpec/EmptyExampleGroup customization Drop RSpec/EmptyExampleGroup customization Aug 21, 2020
@pirj pirj force-pushed the drop-empty-example-group-customization branch from 7e6f968 to 718b398 Compare August 21, 2020 10:40
@pirj pirj changed the base branch from master to release-2.0 October 22, 2020 17:32
@bquorning bquorning mentioned this pull request Oct 22, 2020
@pirj
Copy link
Member Author

pirj commented Oct 22, 2020

I'll rebase this one.

@pirj
Copy link
Member Author

pirj commented Oct 22, 2020

I'll rebase this one.

Nevermind, #956 comes first.

@pirj
Copy link
Member Author

pirj commented Oct 22, 2020

It's time. I'll rebase this one.

PS Once #956 is merged 🤦

@pirj pirj force-pushed the drop-empty-example-group-customization branch from 718b398 to 0cf00f4 Compare October 24, 2020 01:15
@pirj pirj force-pushed the drop-empty-example-group-customization branch from 0cf00f4 to 8ae6bc1 Compare November 1, 2020 20:27
@pirj
Copy link
Member Author

pirj commented Nov 1, 2020

Rebased on top of the current #956

@pirj pirj force-pushed the drop-empty-example-group-customization branch from 8ae6bc1 to d5ee738 Compare November 2, 2020 16:26
@pirj
Copy link
Member Author

pirj commented Nov 2, 2020

JRuby crashed on rake internal_investigation, which is quite interesting. I guess I'll have to add :config to all Concept specs.

@pirj pirj force-pushed the drop-empty-example-group-customization branch from d5ee738 to 98f4b78 Compare November 2, 2020 16:59
@pirj
Copy link
Member Author

pirj commented Nov 2, 2020

I can reliably reproduce the issue with --seed 37363:

Failure/Error: Language.config['HookScopes'].include?(element.to_s)
undefined method `[]' for nil:NilClass
./lib/rubocop/rspec/language.rb:127:in `all'

@pirj pirj force-pushed the drop-empty-example-group-customization branch 2 times, most recently from 56e8db8 to 9cd9efe Compare November 2, 2020 17:39
@pirj pirj mentioned this pull request Nov 2, 2020
16 tasks
@pirj pirj force-pushed the drop-empty-example-group-customization branch 3 times, most recently from 685e0e2 to e0c648a Compare November 2, 2020 22:39
@pirj pirj force-pushed the drop-empty-example-group-customization branch from e0c648a to 962939e Compare November 3, 2020 13:51
The customization is now made via .rubocop.yml configuration, i.e.:

    RSpec:
      Language:
        Includes:
          Example:
          - it_has_special_behavior
@pirj pirj force-pushed the drop-empty-example-group-customization branch from 962939e to 2e3ed04 Compare November 3, 2020 13:54
@pirj
Copy link
Member Author

pirj commented Nov 3, 2020

There seems to be something broken in the edge RuboCop, I'm looking into it.

@bquorning
Copy link
Collaborator

bquorning commented Nov 3, 2020

There seems to be something broken in the edge RuboCop

I this is the change we’re looking for: rubocop/rubocop@51ff1d7#diff-0b9b1074946b724df02392b4d1fac4ccbc9ecff5c71166921d20c41a72ec1c14

If you want to merge, we can fix compatibility with edge in a later PR.

@pirj
Copy link
Member Author

pirj commented Nov 3, 2020

The failure on edge-rubocop is irrelevant. Notified RuboCop guys about a breaking API change here rubocop/rubocop@51ff1d7#r43837211

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.

Remove RSpec/EmptyExampleGroup's CustomIncludeMethods config option
3 participants