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 new RSpec/AvoidSetupHook cop #1133

Merged
merged 1 commit into from May 25, 2021

Conversation

paydaylight
Copy link
Contributor

@paydaylight paydaylight commented Feb 22, 2021

Added new cop RSpec/Rails/AvoidSetupHook that checks that rspec's before method is used over rspec-rails' setup.

  • It helps tests be consistent by eliminating setup calls without arguments that are basically before method calls.
  • Setup allows expectations inside a block that are counterintuitive and make tests do two things at a time which is bad

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.
  • 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 created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in 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.
  • Set VersionAdded in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

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

Copy link
Member

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

Setup allows expectations inside a block

before allows that as well. 🤫

Thanks for the contribution. A few small tweaks and I'm happy to accept it!

lib/rubocop/cop/rspec/prefer_before_over_setup.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/prefer_before_over_setup.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/prefer_before_over_setup.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/prefer_before_over_setup_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/prefer_before_over_setup_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/prefer_before_over_setup_spec.rb Outdated Show resolved Hide resolved
@pirj
Copy link
Member

pirj commented Feb 22, 2021

CircleCI status reports for partially degraded service.

@pirj pirj changed the title add RSpec/PreferBeforeOverSetup cop add RSpec/AvoidSetupHook cop Feb 26, 2021
@pirj pirj changed the title add RSpec/AvoidSetupHook cop Add new RSpec/AvoidSetupHook cop Feb 26, 2021
@paydaylight paydaylight requested a review from pirj March 2, 2021 16:24
Copy link
Member

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

Looks great, thank you!

CHANGELOG.md Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@pirj
Copy link
Member

pirj commented May 24, 2021

Rebased, moved to Rails namespace, changed VersionAdded to 2.4, adjusted the Changelog, squashed commits.

@pirj pirj force-pushed the add-prefer-before-over-setup-cop branch from f13f8d2 to 077754e Compare May 24, 2021 19:50
@pirj
Copy link
Member

pirj commented May 24, 2021

  • ~Setup allows expectations inside a block that are counterintuitive and make tests do two things at a time which is bad.

We're more relying on the Community RSpec style guide that has this rule with single expectation relaxed. See rubocop/rspec-style-guide#47 for more info.

Also, before hook also allows for expectations inside a block.

@pirj
Copy link
Member

pirj commented May 24, 2021

Checked against real-world-rspec:

54136 files inspected, 47 offenses detected, 47 offenses auto-correctable

Two offences were off:

/Users/pirj/source/real-world-rspec/huginn/spec/lib/huginn_scheduler_spec.rb:8:5: C: [Correctable] RSpec/Rails/AvoidSetupHook: Use before instead of setup.
    stub(@scheduler).setup {}
/Users/pirj/source/real-world-rspec/rspec-rails/spec/rspec/rails/setup_and_teardown_adapter_spec.rb:15:7: C: [Correctable] RSpec/Rails/AvoidSetupHook: Use before instead of setup.
      group.setup { "baz" }

Fixed.

@pirj pirj force-pushed the add-prefer-before-over-setup-cop branch 2 times, most recently from 14eda53 to 2f92ed8 Compare May 24, 2021 20:49
@pirj pirj force-pushed the add-prefer-before-over-setup-cop branch from 2f92ed8 to e94b463 Compare May 25, 2021 18:06
@pirj pirj merged commit 9e55194 into rubocop:master May 25, 2021
@pirj
Copy link
Member

pirj commented May 25, 2021

Thank you, @paydaylight !

@pirj
Copy link
Member

pirj commented May 25, 2021

BTW would you like to hack on RSpec/Rails/AvoidTeardownHook or extend and rename AvoidSetupHook to something more generic before we even released it?

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

3 participants