diff options
author | Vitali Tatarintev <vtatarintev@gitlab.com> | 2019-08-22 11:42:16 +0200 |
---|---|---|
committer | Vitali Tatarintev <vtatarintev@gitlab.com> | 2019-08-28 08:43:47 +0200 |
commit | 4f2ac51644754f9a4d4df697ac8984180b0bfdca (patch) | |
tree | 9d23512fd38c993e44ca91354727613e85a52eda | |
parent | be3ec70556b5c514d479e443b4141005ea0365d3 (diff) | |
download | gitlab-ce-4f2ac51644754f9a4d4df697ac8984180b0bfdca.tar.gz |
Add Rubocop check to avoid using `be_success`
Prevent using `be_success` call in controller specs to avoid
getting following deprecation warning:
```
DEPRECATION WARNING: The success? predicate is deprecated and
will be removed in Rails 6.0.
Please use successful? as provided by Rack::Response::Helpers.
```
-rw-r--r-- | rubocop/cop/rspec/be_success_matcher.rb | 50 | ||||
-rw-r--r-- | rubocop/rubocop.rb | 1 | ||||
-rw-r--r-- | rubocop/spec_helpers.rb | 46 | ||||
-rw-r--r-- | spec/rubocop/cop/rspec/be_success_matcher_spec.rb | 77 |
4 files changed, 174 insertions, 0 deletions
diff --git a/rubocop/cop/rspec/be_success_matcher.rb b/rubocop/cop/rspec/be_success_matcher.rb new file mode 100644 index 00000000000..a137e2dba69 --- /dev/null +++ b/rubocop/cop/rspec/be_success_matcher.rb @@ -0,0 +1,50 @@ +require_relative '../../spec_helpers' + +module RuboCop + module Cop + module RSpec + # This cop checks for `be_success` usage in controller specs + # + # @example + # + # # bad + # it "responds with success" do + # expect(response).to be_success + # end + # + # it { is_expected.to be_success } + # + # # good + # it "responds with success" do + # expect(response).to be_successful + # end + # + # it { is_expected.to be_successful } + # + class BeSuccessMatcher < RuboCop::Cop::Cop + include SpecHelpers + + MESSAGE = "Don't use deprecated `success?` method, use `successful?` instead.".freeze + + def_node_search :expect_to_be_success?, <<~PATTERN + (send (send nil? :expect (send nil? ...)) :to (send nil? :be_success)) + PATTERN + + def_node_search :is_expected_to_be_success?, <<~PATTERN + (send (send nil? :is_expected) :to (send nil? :be_success)) + PATTERN + + def be_success_usage?(node) + expect_to_be_success?(node) || is_expected_to_be_success?(node) + end + + def on_send(node) + return unless in_controller_spec?(node) + return unless be_success_usage?(node) + + add_offense(node, location: :expression, message: MESSAGE) + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index d1328c4eb38..c342df6d6c9 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -31,6 +31,7 @@ require_relative 'cop/migration/timestamps' require_relative 'cop/migration/update_column_in_batches' require_relative 'cop/migration/update_large_table' require_relative 'cop/project_path_helper' +require_relative 'cop/rspec/be_success_matcher' require_relative 'cop/rspec/env_assignment' require_relative 'cop/rspec/factories_in_migration_specs' require_relative 'cop/rspec/top_level_describe_path' diff --git a/rubocop/spec_helpers.rb b/rubocop/spec_helpers.rb new file mode 100644 index 00000000000..b38dc3f8cdb --- /dev/null +++ b/rubocop/spec_helpers.rb @@ -0,0 +1,46 @@ +module RuboCop + module SpecHelpers + SPEC_HELPERS = %w[fast_spec_helper.rb rails_helper.rb spec_helper.rb].freeze + MIGRATION_SPEC_DIRECTORIES = ['spec/migrations', 'spec/lib/gitlab/background_migration'].freeze + CONTROLLER_SPEC_DIRECTORIES = ['spec/controllers', 'spec/support/shared_examples/controllers'].freeze + + # Returns true if the given node originated from the spec directory. + def in_spec?(node) + path = node.location.expression.source_buffer.name + pwd = RuboCop::PathUtil.pwd + + !SPEC_HELPERS.include?(File.basename(path)) && + path.start_with?(File.join(pwd, 'spec'), File.join(pwd, 'ee', 'spec')) + end + + def migration_directories + @migration_directories ||= MIGRATION_SPEC_DIRECTORIES.map do |dir| + pwd = RuboCop::PathUtil.pwd + [File.join(pwd, dir), File.join(pwd, 'ee', dir)] + end.flatten + end + + # Returns true if the given node originated from a migration spec. + def in_migration_spec?(node) + path = node.location.expression.source_buffer.name + + in_spec?(node) && + path.start_with?(*migration_directories) + end + + def controller_directories + @controller_directories ||= CONTROLLER_SPEC_DIRECTORIES.map do |dir| + pwd = RuboCop::PathUtil.pwd + [File.join(pwd, dir), File.join(pwd, 'ee', dir)] + end.flatten + end + + # Returns true if the given node originated from a controller spec. + def in_controller_spec?(node) + path = node.location.expression.source_buffer.name + + in_spec?(node) && + path.start_with?(*controller_directories) + end + end +end diff --git a/spec/rubocop/cop/rspec/be_success_matcher_spec.rb b/spec/rubocop/cop/rspec/be_success_matcher_spec.rb new file mode 100644 index 00000000000..fcb5791ef57 --- /dev/null +++ b/spec/rubocop/cop/rspec/be_success_matcher_spec.rb @@ -0,0 +1,77 @@ +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/rspec/be_success_matcher' + +describe RuboCop::Cop::RSpec::BeSuccessMatcher do + include CopHelper + + OFFENSE_CALL_EXPECT_TO_BE_SUCCESS = %(expect(response).to be_success).freeze + OFFENSE_CALL_IS_EXPECTED_TO_BE_SUCCESS = %(is_expected.to be_success).freeze + CALL_EXPECT_TO_BE_SUCCESSFUL = %(expect(response).to be_successful).freeze + CALL_IS_EXPECTED_TO_BE_SUCCESSFUL = %(is_expected.to be_successful).freeze + + let(:source_file) { 'spec/foo_spec.rb' } + + subject(:cop) { described_class.new } + + shared_examples 'an offensive be_success call' do |content| + it "registers an offense for `#{content}`" do + inspect_source(content, source_file) + + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + expect(cop.highlights).to eq([content]) + end + end + + context 'in a controller spec file' do + before do + allow(cop).to receive(:in_controller_spec?).and_return(true) + end + + context "using expect(response).to be_success call" do + it_behaves_like 'an offensive be_success call', OFFENSE_CALL_EXPECT_TO_BE_SUCCESS + end + + context "using is_expected.to be_success call" do + it_behaves_like 'an offensive be_success call', OFFENSE_CALL_IS_EXPECTED_TO_BE_SUCCESS + end + + context "using expect(response).to be_successful" do + it "does not register an offense" do + inspect_source(CALL_EXPECT_TO_BE_SUCCESSFUL) + + expect(cop.offenses.size).to eq(0) + end + end + + context "using is_expected.to be_successful" do + it "does not register an offense" do + inspect_source(CALL_IS_EXPECTED_TO_BE_SUCCESSFUL) + + expect(cop.offenses.size).to eq(0) + end + end + end + + context 'outside of a controller spec file' do + context "using expect(response).to be_success call" do + it "does not register an offense" do + inspect_source(OFFENSE_CALL_EXPECT_TO_BE_SUCCESS) + + expect(cop.offenses.size).to eq(0) + end + end + + context "using is_expected.to be_success call" do + it "does not register an offense" do + inspect_source(OFFENSE_CALL_IS_EXPECTED_TO_BE_SUCCESS) + + expect(cop.offenses.size).to eq(0) + end + end + end +end |