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.

Sign up for GitHub

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

Jump to bottom

Improved shared context. Refactor & simplify some specs. #7970

Merged
merged 2 commits into from May 21, 2020
Merged

Improved shared context. Refactor & simplify some specs. #7970

merged 2 commits into from May 21, 2020

Conversation

marcandre
Copy link
Contributor

I feel there are ways to clean some specs up with default rspec let groups.

How does this look for a start?

Stumbling upon this when working on #7868 because some specs are really abusing the API for autocorrection.

@marcandre marcandre force-pushed the cop_helper branch 2 times, most recently from e5cf9b6 to 866bfe1 Compare May 13, 2020 21:16
@marcandre
Copy link
Contributor Author

marcandre commented May 13, 2020 •
edited

Or maybe I could put these with the :config shared environment instead? Looks like my draft is actually conflicting with it, should be easy to fix

@Drenmi
Copy link
Collaborator

Drenmi commented May 14, 2020

I think this looks like a great improvement to the spec seen in this PR. 👍

@marcandre
Copy link
Contributor Author

I have changed my PR. I believe this shared context is very flexible and would be a good replacement for :config.

@marcandre marcandre changed the title Add default let to cop_helper. Refactor & simplify some specs. Improved shared context. Refactor & simplify some specs. May 17, 2020
@marcandre marcandre marked this pull request as ready for review May 17, 2020 06:58
@marcandre marcandre added this to the 1.0.0 milestone May 17, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented May 17, 2020

I like the idea overall, but I'm not sure all the cops need so many things - you've picked to apply the suggest approach to two specs that are exactly typical cop specs. Perhaps we can get away with a smaller and more focused context for the actual cops.

This was referenced May 20, 2020
Closed
Merged
@marcandre marcandre force-pushed the cop_helper branch 3 times, most recently from a2952c0 to 66bf72f Compare May 21, 2020 02:41
@marcandre
Copy link
Contributor Author

Now that I'm more familiar with the shared_context, here's a revised PR.

It streamlines the :config shared context by:

  1. Adding a let(:cop)
  2. Removing the logic of respond_to?(:cop_config). This could lead to cases where let(:cop_config) {{}} would have specs fail (see Explicit config #8001).
  3. Overrides autocorrect default setting (e.g. JSONLoad), not just the 'enabled' setting.

I simplified a few other specs. I also removed ~150 identical and redundant let(:cop), so the PR might be a bit noisy. The main commit is the first one.

I'll open an different issue for things I dislike about our current cop_helper and expect_offense.

@marcandre marcandre mentioned this pull request May 21, 2020
Open
@bbatsov bbatsov merged commit c7fe5da into rubocop:master May 21, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented May 21, 2020

Looks good! Thanks!

bquorning added a commit to rubocop/rubocop-rspec that referenced this pull request May 24, 2020
@bquorning
Remove default configuration from specs
b9befd3 
After rubocop/rubocop#7970, all specs with
the `:config` metadata by default use the default configuration. So now,
there is often no reason to add a cop's default configuration in specs.
bquorning added a commit to rubocop/rubocop-rspec that referenced this pull request May 24, 2020
@bquorning
Remove explicit subject(:cop) from specs
4e7272b 
In rubocop/rubocop#7970 the shared context
`config` was updated to include the following:

    let(:cop) do
      cop_class.new(config, cop_options)
               .tap { |cop| cop.processed_source = processed_source }
    end

This is functionally equivalent to what we have all over our specs:

    subject(:cop) { described_class.new(config) }

The downside is that our specs become a less explicit. Actually they now
have a mystery guest, which may be considered a smell.
@bquorning bquorning mentioned this pull request May 24, 2020
Merged
4 tasks
@eugeneius eugeneius mentioned this pull request May 24, 2020
Merged
8 tasks
pirj added a commit to pirj/test-prof that referenced this pull request May 26, 2020
@pirj
Explicitly specify the config
524fc04 
rubocop/rubocop#7970 in RuboCop 0.84
mixes in the default config, which changes the config options from
"defult" (missing) `false` to actual `true` from config/defaults.yml
@pirj pirj mentioned this pull request May 26, 2020
Merged
palkan pushed a commit to test-prof/test-prof that referenced this pull request May 28, 2020
@pirj @palkan
Explicitly specify the config
4d6a1d0 
rubocop/rubocop#7970 in RuboCop 0.84
mixes in the default config, which changes the config options from
"defult" (missing) `false` to actual `true` from config/defaults.yml
mockdeep pushed a commit to mockdeep/rubocop-rspec that referenced this pull request Jun 14, 2020
@bquorning @mockdeep
Remove default configuration from specs
bf5a084 
After rubocop/rubocop#7970, all specs with
the `:config` metadata by default use the default configuration. So now,
there is often no reason to add a cop's default configuration in specs.
mockdeep pushed a commit to mockdeep/rubocop-rspec that referenced this pull request Jun 14, 2020
@bquorning @mockdeep
Remove explicit subject(:cop) from specs
7b43684 
In rubocop/rubocop#7970 the shared context
`config` was updated to include the following:

    let(:cop) do
      cop_class.new(config, cop_options)
               .tap { |cop| cop.processed_source = processed_source }
    end

This is functionally equivalent to what we have all over our specs:

    subject(:cop) { described_class.new(config) }

The downside is that our specs become a less explicit. Actually they now
have a mystery guest, which may be considered a smell.
ydah pushed a commit to rubocop/rubocop-factory_bot that referenced this pull request Apr 13, 2023
@bquorning
Remove explicit subject(:cop) from specs
203310b 
In rubocop/rubocop#7970 the shared context
`config` was updated to include the following:

    let(:cop) do
      cop_class.new(config, cop_options)
               .tap { |cop| cop.processed_source = processed_source }
    end

This is functionally equivalent to what we have all over our specs:

    subject(:cop) { described_class.new(config) }

The downside is that our specs become a less explicit. Actually they now
have a mystery guest, which may be considered a smell.
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
@bquorning
Remove explicit subject(:cop) from specs
28431ac 
In rubocop/rubocop#7970 the shared context
`config` was updated to include the following:

    let(:cop) do
      cop_class.new(config, cop_options)
               .tap { |cop| cop.processed_source = processed_source }
    end

This is functionally equivalent to what we have all over our specs:

    subject(:cop) { described_class.new(config) }

The downside is that our specs become a less explicit. Actually they now
have a mystery guest, which may be considered a smell.
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
@bquorning
Remove explicit subject(:cop) from specs
bf12fbd 
In rubocop/rubocop#7970 the shared context
`config` was updated to include the following:

    let(:cop) do
      cop_class.new(config, cop_options)
               .tap { |cop| cop.processed_source = processed_source }
    end

This is functionally equivalent to what we have all over our specs:

    subject(:cop) { described_class.new(config) }

The downside is that our specs become a less explicit. Actually they now
have a mystery guest, which may be considered a smell.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marcandre @Drenmi @bbatsov