summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2018-03-30 17:18:12 +0200
committerRémy Coutable <remy@rymai.me>2018-04-06 17:41:52 +0200
commit4b035896c4ed188c0b4ab0e0b5d1d97e4fef9886 (patch)
treefe2c72f9a850a1d16f05e05b85f82cd090b57c7f
parent0fff9db5eac32ae42cc06e31447104ae15c93675 (diff)
downloadgitlab-ce-4b035896c4ed188c0b4ab0e0b5d1d97e4fef9886.tar.gz
Introduce a new FactoriesInMigrationSpecs cop
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r--rubocop/cop/rspec/factories_in_migration_specs.rb40
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--rubocop/spec_helpers.rb13
-rw-r--r--spec/rubocop/cop/rspec/factories_in_migration_specs_spec.rb48
-rw-r--r--spec/spec_helper.rb1
-rw-r--r--spec/support/helpers/expect_offense.rb20
6 files changed, 122 insertions, 1 deletions
diff --git a/rubocop/cop/rspec/factories_in_migration_specs.rb b/rubocop/cop/rspec/factories_in_migration_specs.rb
new file mode 100644
index 00000000000..0c5aa838a2c
--- /dev/null
+++ b/rubocop/cop/rspec/factories_in_migration_specs.rb
@@ -0,0 +1,40 @@
+require_relative '../../spec_helpers'
+
+module RuboCop
+ module Cop
+ module RSpec
+ # This cop checks for the usage of factories in migration specs
+ #
+ # @example
+ #
+ # # bad
+ # let(:user) { create(:user) }
+ #
+ # # good
+ # let(:users) { table(:users) }
+ # let(:user) { users.create!(name: 'User 1', username: 'user1') }
+ class FactoriesInMigrationSpecs < RuboCop::Cop::Cop
+ include SpecHelpers
+
+ MESSAGE = "Don't use FactoryBot.%s in migration specs, use `table` instead.".freeze
+ FORBIDDEN_METHODS = %i[build build_list create create_list].freeze
+
+ def_node_search :forbidden_factory_usage?, <<~PATTERN
+ (send {(const nil? :FactoryBot) nil?} {#{FORBIDDEN_METHODS.map(&:inspect).join(' ')}} ...)
+ PATTERN
+
+ # Following is what node.children looks like on a match:
+ # - Without FactoryBot namespace: [nil, :build, s(:sym, :user)]
+ # - With FactoryBot namespace: [s(:const, nil, :FactoryBot), :build, s(:sym, :user)]
+ def on_send(node)
+ return unless in_migration_spec?(node)
+ return unless forbidden_factory_usage?(node)
+
+ method = node.children[1]
+
+ add_offense(node, location: :expression, message: MESSAGE % method)
+ end
+ end
+ end
+ end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 0b4c7d31442..406ec95ffc9 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -21,4 +21,5 @@ 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/env_assignment'
+require_relative 'cop/rspec/factories_in_migration_specs'
require_relative 'cop/sidekiq_options_queue'
diff --git a/rubocop/spec_helpers.rb b/rubocop/spec_helpers.rb
index a702a083958..6c0f0193b1a 100644
--- a/rubocop/spec_helpers.rb
+++ b/rubocop/spec_helpers.rb
@@ -6,7 +6,18 @@ module RuboCop
def in_spec?(node)
path = node.location.expression.source_buffer.name
- !SPEC_HELPERS.include?(File.basename(path)) && path.start_with?(File.join(Dir.pwd, 'spec'))
+ !SPEC_HELPERS.include?(File.basename(path)) &&
+ path.start_with?(File.join(Dir.pwd, 'spec'), File.join(Dir.pwd, 'ee', 'spec'))
+ 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?(
+ File.join(Dir.pwd, 'spec', 'migrations'),
+ File.join(Dir.pwd, 'ee', 'spec', 'migrations'))
end
end
end
diff --git a/spec/rubocop/cop/rspec/factories_in_migration_specs_spec.rb b/spec/rubocop/cop/rspec/factories_in_migration_specs_spec.rb
new file mode 100644
index 00000000000..2763f2bda21
--- /dev/null
+++ b/spec/rubocop/cop/rspec/factories_in_migration_specs_spec.rb
@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/rspec/factories_in_migration_specs'
+
+describe RuboCop::Cop::RSpec::FactoriesInMigrationSpecs do
+ include CopHelper
+
+ let(:source_file) { 'spec/migrations/foo_spec.rb' }
+
+ subject(:cop) { described_class.new }
+
+ shared_examples 'an offensive factory call' do |namespace|
+ %i[build build_list create create_list].each do |forbidden_method|
+ namespaced_forbidden_method = "#{namespace}#{forbidden_method}(:user)"
+
+ it "registers an offense for #{namespaced_forbidden_method}" do
+ expect_offense(<<-RUBY)
+ describe 'foo' do
+ let(:user) { #{namespaced_forbidden_method} }
+ #{'^' * namespaced_forbidden_method.size} Don't use FactoryBot.#{forbidden_method} in migration specs, use `table` instead.
+ end
+ RUBY
+ end
+ end
+ end
+
+ context 'in a migration spec file' do
+ before do
+ allow(cop).to receive(:in_migration_spec?).and_return(true)
+ end
+
+ it_behaves_like 'an offensive factory call', ''
+ it_behaves_like 'an offensive factory call', 'FactoryBot.'
+ end
+
+ context 'outside of a migration spec file' do
+ it "does not register an offense" do
+ expect_no_offenses(<<-RUBY)
+ describe 'foo' do
+ let(:user) { create(:user) }
+ end
+ RUBY
+ end
+ end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index beabba99cf5..83664bae046 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -66,6 +66,7 @@ RSpec.configure do |config|
config.include MigrationsHelpers, :migration
config.include StubFeatureFlags
config.include StubENV
+ config.include ExpectOffense
config.infer_spec_type_from_file_location!
diff --git a/spec/support/helpers/expect_offense.rb b/spec/support/helpers/expect_offense.rb
new file mode 100644
index 00000000000..35718ba90c5
--- /dev/null
+++ b/spec/support/helpers/expect_offense.rb
@@ -0,0 +1,20 @@
+require 'rubocop/rspec/support'
+
+# https://github.com/backus/rubocop-rspec/blob/master/spec/support/expect_offense.rb
+# rubocop-rspec gem extension of RuboCop's ExpectOffense module.
+#
+# This mixin is the same as rubocop's ExpectOffense except the default
+# filename ends with `_spec.rb`
+module ExpectOffense
+ include RuboCop::RSpec::ExpectOffense
+
+ DEFAULT_FILENAME = 'example_spec.rb'.freeze
+
+ def expect_offense(source, filename = DEFAULT_FILENAME)
+ super
+ end
+
+ def expect_no_offenses(source, filename = DEFAULT_FILENAME)
+ super
+ end
+end