Skip to content

Commit

Permalink
Add new RSpecRails/SettingsOverwritten cop
Browse files Browse the repository at this point in the history
One problem I encountered recently was that I was directly rewriting `Settings` in one test, resulting in flaky tests in which other tests affected by the rewritten `Settings` succeeded or failed depending on the order of execution, making it difficult to determine the cause.
Therefore, in this PR, I would like to propose the addition of cop to encourage the use of mocks if there are any overwriting `Settings`.
  • Loading branch information
ydah committed Apr 9, 2024
1 parent e568e14 commit 53e6a6b
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 0 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Master (Unreleased)

- Add new `RSpecRails/SettingsOverwritten` cop. ([@ydah])

## 2.28.2 (2024-03-31)

- Fix a `NameError` by Cross-Referencing. ([@ydah])
Expand Down
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ RSpecRails/NegationBeValid:
VersionAdded: '2.23'
Reference: https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/NegationBeValid

RSpecRails/SettingsOverwritten:
Description: Do not overwrite Settings directly, use mock instead.
Enabled: pending
VersionAdded: "<<next>>"
Reference: https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/SettingsOverwritten

RSpecRails/TravelAround:
Description: Prefer to travel in `before` rather than `around`.
Enabled: pending
Expand Down
45 changes: 45 additions & 0 deletions lib/rubocop/cop/rspec_rails/settings_overwritten.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpecRails
# Do not overwrite Settings directly, use mock instead.
#
# @example
# # bad
# Settings.foo = 'bar'
# Settings[:foo] = 'bar'
# Settings.foo[:bar] = 'baz'
# Settings.foo += 'bar'
# Settings.foo << 'bar'
#
# # good
# allow(Settings).to receive(:foo).and_return('bar')
# allow(Settings).to receive(:[]).with(:foo).and_return('bar')
# allow(Settings.foo).to receive(:[]).with(:bar).and_return('baz')
# allow(Settings).to receive(:foo).and_return(Settings.foo + "bar")
#
class SettingsOverwritten < ::RuboCop::Cop::Base
MSG = 'Do not overwrite Settings directly, use mock instead.'

# @!method settings?(node)
def_node_matcher :settings?, <<~PATTERN
(const nil? :Settings)
PATTERN

def on_const(node)
return unless settings?(node)
return unless (assignment = first_assignment(node))

add_offense(assignment)
end

private

def first_assignment(node)
node.each_ancestor.find(&:assignment_or_similar?)
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rspec_rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@
require_relative 'rspec_rails/inferred_spec_type'
require_relative 'rspec_rails/minitest_assertions'
require_relative 'rspec_rails/negation_be_valid'
require_relative 'rspec_rails/settings_overwritten'
require_relative 'rspec_rails/travel_around'
44 changes: 44 additions & 0 deletions spec/rubocop/cop/rspec_rails/settings_overwritten_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpecRails::SettingsOverwritten, :config do
it 'registers an offense when using `Settings.foo = "bar"`' do
expect_offense(<<~RUBY)
Settings.foo = "bar"
^^^^^^^^^^^^^^^^^^^^ Do not overwrite Settings directly, use mock instead.
RUBY
end

it 'registers an offense when using `Settings[:foo] = "bar"`' do
expect_offense(<<~RUBY)
Settings[:foo] = "bar"
^^^^^^^^^^^^^^^^^^^^^^ Do not overwrite Settings directly, use mock instead.
RUBY
end

it 'registers an offense when using `Settings.foo[:bar] = "baz"`' do
expect_offense(<<~RUBY)
Settings.foo[:bar] = "baz"
^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not overwrite Settings directly, use mock instead.
RUBY
end

it 'registers an offense when using `Settings.foo += "bar"`' do
expect_offense(<<~RUBY)
Settings.foo += "bar"
^^^^^^^^^^^^^^^^^^^^^ Do not overwrite Settings directly, use mock instead.
RUBY
end

it 'registers an offense when using `Settings.foo << "bar"`' do
expect_offense(<<~RUBY)
Settings.foo << "bar"
^^^^^^^^^^^^^^^^^^^^^ Do not overwrite Settings directly, use mock instead.
RUBY
end

it 'does not register an offense when using mock' do
expect_no_offenses(<<~RUBY)
allow(Settings).to receive(:foo).and_return("bar")
RUBY
end
end

0 comments on commit 53e6a6b

Please sign in to comment.