diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-04-06 20:22:43 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-04-06 20:22:43 +0300 |
commit | 08e5876403edd65b08712d3167c053daf4fdccb9 (patch) | |
tree | 44e32021fdf8c1cb3661665ee65218d2fe25e1a8 | |
parent | f3fa98e93a0017af182ca18b7138a5f530432842 (diff) | |
parent | 3f598ad7cab303d5fd7865962900dce92ea8c036 (diff) |
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
28 files changed, 206 insertions, 85 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/migrations/add_foreign_keys_to_todos_spec.rb b/spec/migrations/add_foreign_keys_to_todos_spec.rb index 4a22bd6f342..bf2fa5c0f56 100644 --- a/spec/migrations/add_foreign_keys_to_todos_spec.rb +++ b/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) } diff --git a/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb b/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb index 63defcb39bf..d8dd7a2fb83 100644 --- a/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb +++ b/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 diff --git a/spec/migrations/calculate_conv_dev_index_percentages_spec.rb b/spec/migrations/calculate_conv_dev_index_percentages_spec.rb index f3a46025376..19f06810e54 100644 --- a/spec/migrations/calculate_conv_dev_index_percentages_spec.rb +++ b/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, diff --git a/spec/migrations/cleanup_namespaceless_pending_delete_projects_spec.rb b/spec/migrations/cleanup_namespaceless_pending_delete_projects_spec.rb index 033d0e7584d..b5980cb9ddb 100644 --- a/spec/migrations/cleanup_namespaceless_pending_delete_projects_spec.rb +++ b/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) diff --git a/spec/migrations/cleanup_nonexisting_namespace_pending_delete_projects_spec.rb b/spec/migrations/cleanup_nonexisting_namespace_pending_delete_projects_spec.rb index 7879105a334..8f40ac3e38b 100644 --- a/spec/migrations/cleanup_nonexisting_namespace_pending_delete_projects_spec.rb +++ b/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) diff --git a/spec/migrations/issues_moved_to_id_foreign_key_spec.rb b/spec/migrations/issues_moved_to_id_foreign_key_spec.rb index d2eef81f396..dd2b08099f2 100644 --- a/spec/migrations/issues_moved_to_id_foreign_key_spec.rb +++ b/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 } diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb index 638b2853374..4187ab149a5 100644 --- a/spec/migrations/migrate_old_artifacts_spec.rb +++ b/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(:pipeline1) { create(:ci_empty_pipeline, project: project1) } - set(:pipeline2) { create(:ci_empty_pipeline, project: project2) } - set(:pipeline3) { create(:ci_empty_pipeline, project: project3) } - - 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) } + 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) } # 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) } # 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) diff --git a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb b/spec/migrations/migrate_process_commit_worker_jobs_spec.rb index 657113812bd..4ee1d255fbd 100644 --- a/spec/migrations/migrate_process_commit_worker_jobs_spec.rb +++ b/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 diff --git a/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb b/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb index a17c9c72bde..99173708190 100644 --- a/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb +++ b/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| diff --git a/spec/migrations/migrate_user_project_view_spec.rb b/spec/migrations/migrate_user_project_view_spec.rb index 31d16e17d7b..80468b9d01e 100644 --- a/spec/migrations/migrate_user_project_view_spec.rb +++ b/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 diff --git a/spec/migrations/move_personal_snippets_files_spec.rb b/spec/migrations/move_personal_snippets_files_spec.rb index 1a319eccc0d..1f39ad98fb8 100644 --- a/spec/migrations/move_personal_snippets_files_spec.rb +++ b/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) diff --git a/spec/migrations/remove_dot_git_from_usernames_spec.rb b/spec/migrations/remove_dot_git_from_usernames_spec.rb index 3a88a66a476..f11880a83e9 100644 --- a/spec/migrations/remove_dot_git_from_usernames_spec.rb +++ b/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') diff --git a/spec/migrations/remove_duplicate_mr_events_spec.rb b/spec/migrations/remove_duplicate_mr_events_spec.rb index e51872239ad..2509ac6afd6 100644 --- a/spec/migrations/remove_duplicate_mr_events_spec.rb +++ b/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 diff --git a/spec/migrations/remove_project_labels_group_id_spec.rb b/spec/migrations/remove_project_labels_group_id_spec.rb index d80d61af20b..01b09e71d83 100644 --- a/spec/migrations/remove_project_labels_group_id_spec.rb +++ b/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 diff --git a/spec/migrations/remove_soft_removed_objects_spec.rb b/spec/migrations/remove_soft_removed_objects_spec.rb index ec089f9106d..fb70c284f5e 100644 --- a/spec/migrations/remove_soft_removed_objects_spec.rb +++ b/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) diff --git a/spec/migrations/rename_more_reserved_project_names_spec.rb b/spec/migrations/rename_more_reserved_project_names_spec.rb index 75310075cc5..034e8a6a4e5 100644 --- a/spec/migrations/rename_more_reserved_project_names_spec.rb +++ b/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' diff --git a/spec/migrations/rename_reserved_project_names_spec.rb b/spec/migrations/rename_reserved_project_names_spec.rb index 34336d705b1..592ac2b5fb9 100644 --- a/spec/migrations/rename_reserved_project_names_spec.rb +++ b/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' diff --git a/spec/migrations/rename_users_with_renamed_namespace_spec.rb b/spec/migrations/rename_users_with_renamed_namespace_spec.rb index e2994103ed7..b8a4dc2b2c0 100644 --- a/spec/migrations/rename_users_with_renamed_namespace_spec.rb +++ b/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 diff --git a/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb b/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb index 65ec07da31c..ed306fb3d62 100644 --- a/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb +++ b/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 diff --git a/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb index 7494624066a..578440cba20 100644 --- a/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb +++ b/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) diff --git a/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb b/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb index 528dc54781d..560409f08de 100644 --- a/spec/migrations/turn_nested_groups_into_regular_groups_for_mysql_spec.rb +++ b/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 diff --git a/spec/migrations/update_retried_for_ci_build_spec.rb b/spec/migrations/update_retried_for_ci_build_spec.rb index ccb77766b84..637dcbb8e01 100644 --- a/spec/migrations/update_retried_for_ci_build_spec.rb +++ b/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 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 |