Merge branch '42090-create-cop-for-factorybot-methods-used-in-migration-specs' into 'master'

Resolve "Create cop for FactoryBot methods used in migration specs"

Closes #42090

See merge request gitlab-org/gitlab-ce!18108

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

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'

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

spec/migrations/add_foreign_keys_to_todos_spec.rb

@@ -4,8 +4,8 @@ require Rails.root.join('db', 'migrate', '20180201110056_add_foreign_keys_to_tod
 describe AddForeignKeysToTodos, :migration do
   let(:todos) { table(:todos) }
 
-  let(:project) { create(:project) }
-  let(:user) { create(:user) }
+  let(:project) { create(:project) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let(:user) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   context 'add foreign key on user_id' do
     let!(:todo_with_user) { create_todo(user_id: user.id) }
@@ -34,7 +34,7 @@ describe AddForeignKeysToTodos, :migration do
   end
 
   context 'add foreign key on note_id' do
-    let(:note) { create(:note) }
+    let(:note) { create(:note) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
     let!(:todo_with_note) { create_todo(note_id: note.id) }
     let!(:todo_with_invalid_note) { create_todo(note_id: 4711) }
     let!(:todo_without_note) { create_todo(note_id: nil) }

spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb

@@ -6,18 +6,18 @@ describe AddHeadPipelineForEachMergeRequest, :delete do
 
   let(:migration) { described_class.new }
 
-  let!(:project) { create(:project) }
+  let!(:project) { create(:project) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
   let!(:other_project) { fork_project(project) }
 
-  let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: "branch_1") }
-  let!(:pipeline_2) { create(:ci_pipeline, project: other_project, ref: "branch_1") }
-  let!(:pipeline_3) { create(:ci_pipeline, project: other_project, ref: "branch_1") }
-  let!(:pipeline_4) { create(:ci_pipeline, project: project, ref: "branch_2") }
+  let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: "branch_1") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:pipeline_2) { create(:ci_pipeline, project: other_project, ref: "branch_1") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:pipeline_3) { create(:ci_pipeline, project: other_project, ref: "branch_1") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:pipeline_4) { create(:ci_pipeline, project: project, ref: "branch_2") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
-  let!(:mr_1) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_1", target_branch: "target_1") }
-  let!(:mr_2) { create(:merge_request, source_project: other_project, target_project: project, source_branch: "branch_1", target_branch: "target_2") }
-  let!(:mr_3) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_2", target_branch: "master") }
-  let!(:mr_4) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_3", target_branch: "master") }
+  let!(:mr_1) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_1", target_branch: "target_1") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:mr_2) { create(:merge_request, source_project: other_project, target_project: project, source_branch: "branch_1", target_branch: "target_2") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:mr_3) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_2", target_branch: "master") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:mr_4) { create(:merge_request, source_project: project, target_project: project, source_branch: "branch_3", target_branch: "master") } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   context "#up" do
     context "when source_project and source_branch of pipeline are the same of merge request" do

spec/migrations/calculate_conv_dev_index_percentages_spec.rb

@@ -6,7 +6,7 @@ require Rails.root.join('db', 'post_migrate', '20170803090603_calculate_conv_dev
 describe CalculateConvDevIndexPercentages, :delete do
   let(:migration) { described_class.new }
   let!(:conv_dev_index) do
-    create(:conversational_development_index_metric,
+    create(:conversational_development_index_metric, # rubocop:disable RSpec/FactoriesInMigrationSpecs
       leader_notes: 0,
       instance_milestones: 0,
       percentage_issues: 0,

spec/migrations/cleanup_namespaceless_pending_delete_projects_spec.rb

@@ -10,9 +10,9 @@ describe CleanupNamespacelessPendingDeleteProjects, :migration, schema: 20180222
 
   describe '#up' do
     it 'only cleans up pending delete projects' do
-      create(:project)
-      create(:project, pending_delete: true)
-      project = build(:project, pending_delete: true, namespace_id: nil)
+      create(:project) # rubocop:disable RSpec/FactoriesInMigrationSpecs
+      create(:project, pending_delete: true) # rubocop:disable RSpec/FactoriesInMigrationSpecs
+      project = build(:project, pending_delete: true, namespace_id: nil) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       project.save(validate: false)
 
       expect(NamespacelessProjectDestroyWorker).to receive(:bulk_perform_async).with([[project.id]])
@@ -21,8 +21,8 @@ describe CleanupNamespacelessPendingDeleteProjects, :migration, schema: 20180222
     end
 
     it 'does nothing when no pending delete projects without namespace found' do
-      create(:project)
-      create(:project, pending_delete: true)
+      create(:project) # rubocop:disable RSpec/FactoriesInMigrationSpecs
+      create(:project, pending_delete: true) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       expect(NamespacelessProjectDestroyWorker).not_to receive(:bulk_perform_async)
 

spec/migrations/cleanup_nonexisting_namespace_pending_delete_projects_spec.rb

@@ -9,11 +9,11 @@ describe CleanupNonexistingNamespacePendingDeleteProjects do
   end
 
   describe '#up' do
-    set(:some_project) { create(:project) }
+    set(:some_project) { create(:project) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
     it 'only cleans up when namespace does not exist' do
-      create(:project, pending_delete: true)
-      project = build(:project, pending_delete: true, namespace: nil, namespace_id: Namespace.maximum(:id).to_i.succ)
+      create(:project, pending_delete: true) # rubocop:disable RSpec/FactoriesInMigrationSpecs
+      project = build(:project, pending_delete: true, namespace: nil, namespace_id: Namespace.maximum(:id).to_i.succ) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       project.save(validate: false)
 
       expect(NamespacelessProjectDestroyWorker).to receive(:bulk_perform_async).with([[project.id]])
@@ -22,7 +22,7 @@ describe CleanupNonexistingNamespacePendingDeleteProjects do
     end
 
     it 'does nothing when no pending delete projects without namespace found' do
-      create(:project, pending_delete: true, namespace: create(:namespace))
+      create(:project, pending_delete: true, namespace: create(:namespace)) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       expect(NamespacelessProjectDestroyWorker).not_to receive(:bulk_perform_async)
 

spec/migrations/issues_moved_to_id_foreign_key_spec.rb

@@ -5,9 +5,9 @@ require Rails.root.join('db', 'migrate', '20171106151218_issues_moved_to_id_fore
 # only_mirror_protected_branches column in the projects table to create a
 # project via FactoryBot.
 describe IssuesMovedToIdForeignKey, :migration, schema: 20171114150259 do
-  let!(:issue_first) { create(:issue, moved_to_id: issue_second.id) }
-  let!(:issue_second) { create(:issue, moved_to_id: issue_third.id) }
-  let!(:issue_third) { create(:issue) }
+  let!(:issue_first) { create(:issue, moved_to_id: issue_second.id) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:issue_second) { create(:issue, moved_to_id: issue_third.id) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:issue_third) { create(:issue) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   subject { described_class.new }
 

spec/migrations/migrate_old_artifacts_spec.rb

@@ -16,18 +16,18 @@ describe MigrateOldArtifacts do
   end
 
   context 'with migratable data' do
-    set(:project1) { create(:project, ci_id: 2) }
-    set(:project2) { create(:project, ci_id: 3) }
-    set(:project3) { create(:project) }
+    set(:project1) { create(:project, ci_id: 2) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    set(:project2) { create(:project, ci_id: 3) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    set(:project3) { create(:project) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
-    set(:pipeline1) { create(:ci_empty_pipeline, project: project1) }
-    set(:pipeline2) { create(:ci_empty_pipeline, project: project2) }
-    set(:pipeline3) { create(:ci_empty_pipeline, project: project3) }
+    set(:pipeline1) { create(:ci_empty_pipeline, project: project1) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    set(:pipeline2) { create(:ci_empty_pipeline, project: project2) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    set(:pipeline3) { create(:ci_empty_pipeline, project: project3) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
-    let!(:build_with_legacy_artifacts) { create(:ci_build, pipeline: pipeline1) }
-    let!(:build_without_artifacts) { create(:ci_build, pipeline: pipeline1) }
-    let!(:build2) { create(:ci_build, pipeline: pipeline2) }
-    let!(:build3) { create(:ci_build, pipeline: pipeline3) }
+    let!(:build_with_legacy_artifacts) { create(:ci_build, pipeline: pipeline1) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    let!(:build_without_artifacts) { create(:ci_build, pipeline: pipeline1) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    let!(:build2) { create(:ci_build, pipeline: pipeline2) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    let!(:build3) { create(:ci_build, pipeline: pipeline3) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
     before do
       setup_builds(build2, build3)

spec/migrations/migrate_process_commit_worker_jobs_spec.rb

@@ -4,8 +4,8 @@ require 'spec_helper'
 require Rails.root.join('db', 'migrate', '20161124141322_migrate_process_commit_worker_jobs.rb')
 
 describe MigrateProcessCommitWorkerJobs do
-  let(:project) { create(:project, :legacy_storage, :repository) }
-  let(:user) { create(:user) }
+  let(:project) { create(:project, :legacy_storage, :repository) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let(:user) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
   let(:commit) { project.commit.raw.rugged_commit }
 
   describe 'Project' do

spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb

@@ -5,8 +5,8 @@ require Rails.root.join('db', 'post_migrate', '20170324160416_migrate_user_activ
 
 describe MigrateUserActivitiesToUsersLastActivityOn, :clean_gitlab_redis_shared_state, :delete do
   let(:migration) { described_class.new }
-  let!(:user_active_1) { create(:user) }
-  let!(:user_active_2) { create(:user) }
+  let!(:user_active_1) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:user_active_2) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   def record_activity(user, time)
     Gitlab::Redis::SharedState.with do |redis|

spec/migrations/migrate_user_project_view_spec.rb

@@ -5,7 +5,7 @@ require Rails.root.join('db', 'post_migrate', '20170406142253_migrate_user_proje
 
 describe MigrateUserProjectView, :delete do
   let(:migration) { described_class.new }
-  let!(:user) { create(:user, project_view: 'readme') }
+  let!(:user) { create(:user, project_view: 'readme') } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   describe '#up' do
     it 'updates project view setting with new value' do

spec/migrations/move_personal_snippets_files_spec.rb

@@ -16,14 +16,14 @@ describe MovePersonalSnippetsFiles do
 
   describe "#up" do
     let(:snippet) do
-      snippet = create(:personal_snippet)
+      snippet = create(:personal_snippet) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       create_upload('picture.jpg', snippet)
       snippet.update(description: markdown_linking_file('picture.jpg', snippet))
       snippet
     end
 
     let(:snippet_with_missing_file) do
-      snippet = create(:snippet)
+      snippet = create(:snippet) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       create_upload('picture.jpg', snippet, create_file: false)
       snippet.update(description: markdown_linking_file('picture.jpg', snippet))
       snippet
@@ -62,7 +62,7 @@ describe MovePersonalSnippetsFiles do
         secret = "secret#{snippet.id}"
         file_location = "/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
         markdown = markdown_linking_file('picture.jpg', snippet)
-        note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
+        note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}") # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
         migration.up
 
@@ -73,14 +73,14 @@ describe MovePersonalSnippetsFiles do
 
   describe "#down" do
     let(:snippet) do
-      snippet = create(:personal_snippet)
+      snippet = create(:personal_snippet) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       create_upload('picture.jpg', snippet, in_new_path: true)
       snippet.update(description: markdown_linking_file('picture.jpg', snippet, in_new_path: true))
       snippet
     end
 
     let(:snippet_with_missing_file) do
-      snippet = create(:personal_snippet)
+      snippet = create(:personal_snippet) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       create_upload('picture.jpg', snippet, create_file: false, in_new_path: true)
       snippet.update(description: markdown_linking_file('picture.jpg', snippet, in_new_path: true))
       snippet
@@ -119,7 +119,7 @@ describe MovePersonalSnippetsFiles do
         markdown = markdown_linking_file('picture.jpg', snippet, in_new_path: true)
         secret = "secret#{snippet.id}"
         file_location = "/uploads/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
-        note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
+        note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}") # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
         migration.down
 
@@ -135,7 +135,7 @@ describe MovePersonalSnippetsFiles do
 
       secret = '123456789'
       filename = 'hello.jpg'
-      snippet = create(:personal_snippet)
+      snippet = create(:personal_snippet) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       path_before = "/uploads/personal_snippet/#{snippet.id}/#{secret}/#{filename}"
       path_after = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/#{filename}"
@@ -161,7 +161,7 @@ describe MovePersonalSnippetsFiles do
       FileUtils.touch(absolute_path)
     end
 
-    create(:upload, model: snippet, path: "#{secret}/#{filename}", uploader: PersonalFileUploader)
+    create(:upload, model: snippet, path: "#{secret}/#{filename}", uploader: PersonalFileUploader) # rubocop:disable RSpec/FactoriesInMigrationSpecs
   end
 
   def markdown_linking_file(filename, snippet, in_new_path: false)

spec/migrations/remove_dot_git_from_usernames_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 require Rails.root.join('db', 'migrate', '20161226122833_remove_dot_git_from_usernames.rb')
 
 describe RemoveDotGitFromUsernames do
-  let(:user) { create(:user) }
+  let(:user) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
   let(:migration) { described_class.new }
 
   describe '#up' do
@@ -23,7 +23,7 @@ describe RemoveDotGitFromUsernames do
 
   context 'when new path exists already' do
     describe '#up' do
-      let(:user2) { create(:user) }
+      let(:user2) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       before do
         update_namespace(user, 'test.git')

spec/migrations/remove_duplicate_mr_events_spec.rb

@@ -5,17 +5,17 @@ describe RemoveDuplicateMrEvents, :delete do
   let(:migration) { described_class.new }
 
   describe '#up' do
-    let(:user) { create(:user) }
-    let(:merge_requests) { create_list(:merge_request, 2) }
-    let(:issue) { create(:issue) }
+    let(:user) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    let(:merge_requests) { create_list(:merge_request, 2) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    let(:issue) { create(:issue) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
     let!(:events) do
       [
-        create(:event, :created, author: user, target: merge_requests.first),
-        create(:event, :created, author: user, target: merge_requests.first),
-        create(:event, :updated, author: user, target: merge_requests.first),
-        create(:event, :created, author: user, target: merge_requests.second),
-        create(:event, :created, author: user, target: issue),
-        create(:event, :created, author: user, target: issue)
+        create(:event, :created, author: user, target: merge_requests.first), # rubocop:disable RSpec/FactoriesInMigrationSpecs
+        create(:event, :created, author: user, target: merge_requests.first), # rubocop:disable RSpec/FactoriesInMigrationSpecs
+        create(:event, :updated, author: user, target: merge_requests.first), # rubocop:disable RSpec/FactoriesInMigrationSpecs
+        create(:event, :created, author: user, target: merge_requests.second), # rubocop:disable RSpec/FactoriesInMigrationSpecs
+        create(:event, :created, author: user, target: issue), # rubocop:disable RSpec/FactoriesInMigrationSpecs
+        create(:event, :created, author: user, target: issue) # rubocop:disable RSpec/FactoriesInMigrationSpecs
       ]
     end
 

spec/migrations/remove_project_labels_group_id_spec.rb

@@ -5,9 +5,9 @@ require Rails.root.join('db', 'post_migrate', '20180202111106_remove_project_lab
 
 describe RemoveProjectLabelsGroupId, :delete do
   let(:migration) { described_class.new }
-  let(:group) { create(:group) }
-  let!(:project_label) { create(:label, group_id: group.id) }
-  let!(:group_label) { create(:group_label) }
+  let(:group) { create(:group) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:project_label) { create(:label, group_id: group.id) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:group_label) { create(:group_label) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   describe '#up' do
     it 'updates the project labels group ID' do

spec/migrations/remove_soft_removed_objects_spec.rb

@@ -8,7 +8,7 @@ describe RemoveSoftRemovedObjects, :migration do
         create_with_deleted_at(:issue)
       end
 
-      regular_issue = create(:issue)
+      regular_issue = create(:issue) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       run_migration
 
@@ -28,7 +28,7 @@ describe RemoveSoftRemovedObjects, :migration do
 
     it 'removes routes of soft removed personal namespaces' do
       namespace = create_with_deleted_at(:namespace)
-      group = create(:group)
+      group = create(:group) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       expect(Route.where(source: namespace).exists?).to eq(true)
       expect(Route.where(source: group).exists?).to eq(true)
@@ -41,7 +41,7 @@ describe RemoveSoftRemovedObjects, :migration do
 
     it 'schedules the removal of soft removed groups' do
       group = create_with_deleted_at(:group)
-      admin = create(:user, admin: true)
+      admin = create(:user, admin: true) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
       expect_any_instance_of(GroupDestroyWorker)
         .to receive(:perform)
@@ -67,7 +67,7 @@ describe RemoveSoftRemovedObjects, :migration do
   end
 
   def create_with_deleted_at(*args)
-    row = create(*args)
+    row = create(*args) # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
     # We set "deleted_at" this way so we don't run into any column cache issues.
     row.class.where(id: row.id).update_all(deleted_at: 1.year.ago)

spec/migrations/rename_more_reserved_project_names_spec.rb

@@ -8,7 +8,7 @@ require Rails.root.join('db', 'post_migrate', '20170313133418_rename_more_reserv
 # around this we use the DELETE cleaning strategy.
 describe RenameMoreReservedProjectNames, :delete do
   let(:migration) { described_class.new }
-  let!(:project) { create(:project) }
+  let!(:project) { create(:project) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   before do
     project.path = 'artifacts'

spec/migrations/rename_reserved_project_names_spec.rb

@@ -12,7 +12,7 @@ require Rails.root.join('db', 'post_migrate', '20161221153951_rename_reserved_pr
 # Ideally, the test should not use factories and rely on the `table` helper instead.
 describe RenameReservedProjectNames, :migration, schema: :latest do
   let(:migration) { described_class.new }
-  let!(:project) { create(:project) }
+  let!(:project) { create(:project) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   before do
     project.path = 'projects'

spec/migrations/rename_users_with_renamed_namespace_spec.rb

@@ -3,12 +3,12 @@ require Rails.root.join('db', 'post_migrate', '20170518200835_rename_users_with_
 
 describe RenameUsersWithRenamedNamespace, :delete do
   it 'renames a user that had their namespace renamed to the namespace path' do
-    other_user = create(:user, username: 'kodingu')
-    other_user1 = create(:user, username: 'api0')
+    other_user = create(:user, username: 'kodingu') # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    other_user1 = create(:user, username: 'api0') # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
-    user = create(:user, username: "Users0")
+    user = create(:user, username: "Users0") # rubocop:disable RSpec/FactoriesInMigrationSpecs
     user.update_column(:username, 'Users')
-    user1 = create(:user, username: "import0")
+    user1 = create(:user, username: "import0") # rubocop:disable RSpec/FactoriesInMigrationSpecs
     user1.update_column(:username, 'import')
 
     described_class.new.up

spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb

@@ -3,8 +3,8 @@ require Rails.root.join('db', 'post_migrate', '20171005130944_schedule_create_gp
 
 describe ScheduleCreateGpgKeySubkeysFromGpgKeys, :migration, :sidekiq do
   before do
-    create(:gpg_key, id: 1, key: GpgHelpers::User1.public_key)
-    create(:gpg_key, id: 2, key: GpgHelpers::User3.public_key)
+    create(:gpg_key, id: 1, key: GpgHelpers::User1.public_key) # rubocop:disable RSpec/FactoriesInMigrationSpecs
+    create(:gpg_key, id: 2, key: GpgHelpers::User3.public_key) # rubocop:disable RSpec/FactoriesInMigrationSpecs
     # Delete all subkeys so they can be recreated
     GpgKeySubkey.destroy_all
   end

spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb

@@ -8,7 +8,7 @@ describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq
       .to receive(:commits_count=).and_return(nil)
   end
 
-  let!(:mrs) { create_list(:merge_request, 3) }
+  let!(:mrs) { create_list(:merge_request, 3) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   it 'correctly schedules background migrations' do
     stub_const("#{described_class.name}::BATCH_SIZE", 2)

spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb

@@ -2,10 +2,10 @@ require 'spec_helper'
 require Rails.root.join('db', 'migrate', '20170503140202_turn_nested_groups_into_regular_groups_for_mysql.rb')
 
 describe TurnNestedGroupsIntoRegularGroupsForMysql do
-  let!(:parent_group) { create(:group) }
-  let!(:child_group) { create(:group, parent: parent_group) }
-  let!(:project) { create(:project, :legacy_storage, :empty_repo, namespace: child_group) }
-  let!(:member) { create(:user) }
+  let!(:parent_group) { create(:group) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:child_group) { create(:group, parent: parent_group) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:project) { create(:project, :legacy_storage, :empty_repo, namespace: child_group) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:member) { create(:user) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
   let(:migration) { described_class.new }
 
   before do

spec/migrations/update_retried_for_ci_build_spec.rb

@@ -2,9 +2,9 @@ require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170503004427_update_retried_for_ci_build.rb')
 
 describe UpdateRetriedForCiBuild, :delete do
-  let(:pipeline) { create(:ci_pipeline) }
-  let!(:build_old) { create(:ci_build, pipeline: pipeline, name: 'test') }
-  let!(:build_new) { create(:ci_build, pipeline: pipeline, name: 'test') }
+  let(:pipeline) { create(:ci_pipeline) } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:build_old) { create(:ci_build, pipeline: pipeline, name: 'test') } # rubocop:disable RSpec/FactoriesInMigrationSpecs
+  let!(:build_new) { create(:ci_build, pipeline: pipeline, name: 'test') } # rubocop:disable RSpec/FactoriesInMigrationSpecs
 
   before do
     described_class.new.up

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

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!
 

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