Merge branch 'ee-40744-idempotent-ids' into 'master'

[EE] Use the DatabaseCleaner 'deletion' strategy instead of 'truncation'

See merge request gitlab-org/gitlab-ee!4125

features/support/db_cleaner.rb

 require 'database_cleaner'
 
-DatabaseCleaner[:active_record].strategy = :truncation, { except: ['licenses'] }
+DatabaseCleaner[:active_record].strategy = :deletion, { except: ['licenses'] }
 
 Spinach.hooks.before_scenario do
   DatabaseCleaner.start

spec/ee/spec/lib/gitlab/geo/file_transfer_spec.rb

@@ -13,7 +13,7 @@ describe Gitlab::Geo::FileTransfer do
         expect(subject.file_id).to eq(upload.id)
         expect(subject.filename).to eq(AvatarUploader.absolute_path(upload))
         expect(Pathname.new(subject.filename).absolute?).to be_truthy
-        expect(subject.request_data).to eq({ id: upload.id,
+        expect(subject.request_data).to eq({ id: upload.model_id,
                                              type: 'User',
                                              checksum: upload.checksum })
       end

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -108,7 +108,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           it 'shows resolved discussion when toggled' do
             find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] .discussion-toggle-button").click
 
-            expect(page.find(".timeline-content #note_#{note.noteable_id}")).to be_visible
+            expect(page.find(".timeline-content #note_#{note.id}")).to be_visible
           end
         end
 

spec/javascripts/commit/pipelines/pipelines_spec.js

@@ -10,9 +10,10 @@ describe('Pipelines table in Commits and Merge requests', () => {
   preloadFixtures(jsonFixtureName);
 
   beforeEach(() => {
-    PipelinesTable = Vue.extend(pipelinesTable);
     const pipelines = getJSONFixture(jsonFixtureName).pipelines;
-    pipeline = pipelines.find(p => p.id === 1);
+
+    PipelinesTable = Vue.extend(pipelinesTable);
+    pipeline = pipelines.find(p => p.user !== null && p.commit !== null);
   });
 
   describe('successful request', () => {

spec/javascripts/pipelines/pipelines_table_row_spec.js

@@ -24,9 +24,10 @@ describe('Pipelines Table Row', () => {
 
   beforeEach(() => {
     const pipelines = getJSONFixture(jsonFixtureName).pipelines;
-    pipeline = pipelines.find(p => p.id === 1);
-    pipelineWithoutAuthor = pipelines.find(p => p.id === 2);
-    pipelineWithoutCommit = pipelines.find(p => p.id === 3);
+
+    pipeline = pipelines.find(p => p.user !== null && p.commit !== null);
+    pipelineWithoutAuthor = pipelines.find(p => p.user == null && p.commit !== null);
+    pipelineWithoutCommit = pipelines.find(p => p.user == null && p.commit == null);
   });
 
   afterEach(() => {

spec/javascripts/pipelines/pipelines_table_spec.js

@@ -11,9 +11,10 @@ describe('Pipelines Table', () => {
   preloadFixtures(jsonFixtureName);
 
   beforeEach(() => {
-    PipelinesTableComponent = Vue.extend(pipelinesTableComp);
     const pipelines = getJSONFixture(jsonFixtureName).pipelines;
-    pipeline = pipelines.find(p => p.id === 1);
+
+    PipelinesTableComponent = Vue.extend(pipelinesTableComp);
+    pipeline = pipelines.find(p => p.user !== null && p.commit !== null);
   });
 
   describe('table', () => {

spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb

 require 'spec_helper'
 
-describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits, :truncate, :migration, schema: 20171114162227 do
+describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits, :migration, schema: 20171114162227 do
   let(:merge_request_diffs) { table(:merge_request_diffs) }
   let(:merge_requests) { table(:merge_requests) }
 

spec/lib/gitlab/background_migration/migrate_system_uploads_to_new_folder_spec.rb

 require 'spec_helper'
 
-describe Gitlab::BackgroundMigration::MigrateSystemUploadsToNewFolder do
+describe Gitlab::BackgroundMigration::MigrateSystemUploadsToNewFolder, :delete do
   let(:migration) { described_class.new }
 
   before do
@@ -8,7 +8,7 @@ describe Gitlab::BackgroundMigration::MigrateSystemUploadsToNewFolder do
   end
 
   describe '#perform' do
-    it 'renames the path of system-uploads', :truncate do
+    it 'renames the path of system-uploads' do
       upload = create(:upload, model: create(:project), path: 'uploads/system/project/avatar.jpg')
 
       migration.perform('uploads/system/', 'uploads/-/system/')

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :truncate do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :truncate do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :delete do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
   let(:namespace) { create(:group, name: 'the-path') }

spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :truncate do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :delete do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
   let(:project) do

spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb

@@ -13,7 +13,7 @@ shared_examples 'renames child namespaces' do |type|
   end
 end
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1, :truncate do
+describe Gitlab::Database::RenameReservedPathsMigration::V1, :delete do
   let(:subject) { FakeRenameReservedPathMigrationV1.new }
 
   before do

spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb

 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170508170547_add_head_pipeline_for_each_merge_request.rb')
 
-describe AddHeadPipelineForEachMergeRequest, :truncate do
+describe AddHeadPipelineForEachMergeRequest, :delete do
   include ProjectForksHelper
 
   let(:migration) { described_class.new }

spec/migrations/calculate_conv_dev_index_percentages_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170803090603_calculate_conv_dev_index_percentages.rb')
 
-describe CalculateConvDevIndexPercentages, truncate: true do
+describe CalculateConvDevIndexPercentages, :delete do
   let(:migration) { described_class.new }
   let!(:conv_dev_index) do
     create(:conversational_development_index_metric,

spec/migrations/fix_wrongly_renamed_routes_spec.rb

 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170518231126_fix_wrongly_renamed_routes.rb')
 
-describe FixWronglyRenamedRoutes, :truncate, :migration do
+describe FixWronglyRenamedRoutes, :migration do
   let(:subject) { described_class.new }
   let(:namespaces_table) { table(:namespaces) }
   let(:projects_table) { table(:projects) }

spec/migrations/migrate_issues_to_ghost_user_spec.rb

@@ -8,10 +8,10 @@ describe MigrateIssuesToGhostUser, :migration do
     let(:users) { table(:users) }
 
     before do
-      projects.create!(name: 'gitlab')
+      project = projects.create!(name: 'gitlab')
       user = users.create(email: 'test@example.com')
-      issues.create(title: 'Issue 1', author_id: nil, project_id: 1)
-      issues.create(title: 'Issue 2', author_id: user.id, project_id: 1)
+      issues.create(title: 'Issue 1', author_id: nil, project_id: project.id)
+      issues.create(title: 'Issue 2', author_id: user.id, project_id: project.id)
     end
 
     context 'when ghost user exists' do

spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170324160416_migrate_user_activities_to_users_last_activity_on.rb')
 
-describe MigrateUserActivitiesToUsersLastActivityOn, :clean_gitlab_redis_shared_state, :truncate do
+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) }

spec/migrations/migrate_user_project_view_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170406142253_migrate_user_project_view.rb')
 
-describe MigrateUserProjectView, :truncate do
+describe MigrateUserProjectView, :delete do
   let(:migration) { described_class.new }
   let!(:user) { create(:user, project_view: 'readme') }
 

spec/migrations/remove_duplicate_mr_events_spec.rb

 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170815060945_remove_duplicate_mr_events.rb')
 
-describe RemoveDuplicateMrEvents, truncate: true do
+describe RemoveDuplicateMrEvents, :delete do
   let(:migration) { described_class.new }
 
   describe '#up' do

spec/migrations/rename_more_reserved_project_names_spec.rb

@@ -5,8 +5,8 @@ require Rails.root.join('db', 'post_migrate', '20170313133418_rename_more_reserv
 
 # This migration uses multiple threads, and thus different transactions. This
 # means data created in this spec may not be visible to some threads. To work
-# around this we use the TRUNCATE cleaning strategy.
-describe RenameMoreReservedProjectNames, truncate: true do
+# around this we use the DELETE cleaning strategy.
+describe RenameMoreReservedProjectNames, :delete do
   let(:migration) { described_class.new }
   let!(:project) { create(:project) }
 

spec/migrations/rename_reserved_project_names_spec.rb

@@ -5,8 +5,8 @@ require Rails.root.join('db', 'post_migrate', '20161221153951_rename_reserved_pr
 
 # This migration uses multiple threads, and thus different transactions. This
 # means data created in this spec may not be visible to some threads. To work
-# around this we use the TRUNCATE cleaning strategy.
-describe RenameReservedProjectNames, truncate: true do
+# around this we use the DELETE cleaning strategy.
+describe RenameReservedProjectNames, :delete do
   let(:migration) { described_class.new }
   let!(:project) { create(:project) }
 

spec/migrations/rename_users_with_renamed_namespace_spec.rb

 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170518200835_rename_users_with_renamed_namespace.rb')
 
-describe RenameUsersWithRenamedNamespace, truncate: true do
+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')

spec/migrations/update_retried_for_ci_build_spec.rb

 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170503004427_update_retried_for_ci_build.rb')
 
-describe UpdateRetriedForCiBuild, truncate: true do
+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') }

spec/models/concerns/avatarable_spec.rb

 require 'spec_helper'
 
 describe Avatarable do
-  subject { create(:project, avatar: fixture_file_upload(File.join(Rails.root, 'spec/fixtures/dk.png'))) }
+  set(:project) { create(:project, avatar: fixture_file_upload(File.join(Rails.root, 'spec/fixtures/dk.png'))) }
 
   let(:gitlab_host) { "https://gitlab.example.com" }
   let(:relative_url_root) { "/gitlab" }
-  let(:asset_host) { "https://gitlab-assets.example.com" }
+  let(:asset_host) { 'https://gitlab-assets.example.com' }
 
   before do
     stub_config_setting(base_url: gitlab_host)
@@ -15,29 +15,32 @@ describe Avatarable do
   describe '#avatar_path' do
     using RSpec::Parameterized::TableSyntax
 
-    where(:has_asset_host, :visibility_level, :only_path, :avatar_path) do
-      true  | Project::PRIVATE  | true  | [gitlab_host, relative_url_root, subject.avatar.url]
-      true  | Project::PRIVATE  | false | [gitlab_host, relative_url_root, subject.avatar.url]
-      true  | Project::INTERNAL | true  | [gitlab_host, relative_url_root, subject.avatar.url]
-      true  | Project::INTERNAL | false | [gitlab_host, relative_url_root, subject.avatar.url]
-      true  | Project::PUBLIC   | true  | [subject.avatar.url]
-      true  | Project::PUBLIC   | false | [asset_host, subject.avatar.url]
-      false | Project::PRIVATE  | true  | [relative_url_root, subject.avatar.url]
-      false | Project::PRIVATE  | false | [gitlab_host, relative_url_root, subject.avatar.url]
-      false | Project::INTERNAL | true  | [relative_url_root, subject.avatar.url]
-      false | Project::INTERNAL | false | [gitlab_host, relative_url_root, subject.avatar.url]
-      false | Project::PUBLIC   | true  | [relative_url_root, subject.avatar.url]
-      false | Project::PUBLIC   | false | [gitlab_host, relative_url_root, subject.avatar.url]
+    where(:has_asset_host, :visibility_level, :only_path, :avatar_path_prefix) do
+      true  | Project::PRIVATE  | true  | [gitlab_host, relative_url_root]
+      true  | Project::PRIVATE  | false | [gitlab_host, relative_url_root]
+      true  | Project::INTERNAL | true  | [gitlab_host, relative_url_root]
+      true  | Project::INTERNAL | false | [gitlab_host, relative_url_root]
+      true  | Project::PUBLIC   | true  | []
+      true  | Project::PUBLIC   | false | [asset_host]
+      false | Project::PRIVATE  | true  | [relative_url_root]
+      false | Project::PRIVATE  | false | [gitlab_host, relative_url_root]
+      false | Project::INTERNAL | true  | [relative_url_root]
+      false | Project::INTERNAL | false | [gitlab_host, relative_url_root]
+      false | Project::PUBLIC   | true  | [relative_url_root]
+      false | Project::PUBLIC   | false | [gitlab_host, relative_url_root]
     end
 
     with_them do
       before do
-        allow(ActionController::Base).to receive(:asset_host).and_return(has_asset_host ? asset_host : nil)
-        subject.visibility_level = visibility_level
+        allow(ActionController::Base).to receive(:asset_host) { has_asset_host && asset_host }
+
+        project.visibility_level = visibility_level
       end
 
+      let(:avatar_path) { (avatar_path_prefix + [project.avatar.url]).join }
+
       it 'returns the expected avatar path' do
-        expect(subject.avatar_path(only_path: only_path)).to eq(avatar_path.join)
+        expect(project.avatar_path(only_path: only_path)).to eq(avatar_path)
       end
     end
   end

spec/models/member_spec.rb

@@ -488,7 +488,7 @@ describe Member do
       member.accept_invite!(user)
     end
 
-    it "refreshes user's authorized projects", :truncate do
+    it "refreshes user's authorized projects", :delete do
       project = member.source
 
       expect(user.authorized_projects).not_to include(project)
@@ -523,7 +523,7 @@ describe Member do
     end
   end
 
-  describe "destroying a record", :truncate do
+  describe "destroying a record", :delete do
     it "refreshes user's authorized projects" do
       project = create(:project, :private)
       user    = create(:user)

spec/models/project_group_link_spec.rb

@@ -30,7 +30,7 @@ describe ProjectGroupLink do
     end
   end
 
-  describe "destroying a record", :truncate do
+  describe "destroying a record", :delete do
     it "refreshes group users' authorized projects" do
       project     = create(:project, :private)
       group       = create(:group)

spec/models/user_spec.rb

@@ -1618,7 +1618,7 @@ describe User do
     it { is_expected.to eq([private_group]) }
   end
 
-  describe '#authorized_projects', :truncate do
+  describe '#authorized_projects', :delete do
     context 'with a minimum access level' do
       it 'includes projects for which the user is an owner' do
         user = create(:user)

spec/support/db_cleaner.rb

+require 'database_cleaner/active_record/deletion'
+
+module FakeInformationSchema
+  # Work around a bug in DatabaseCleaner when using the deletion strategy:
+  # https://github.com/DatabaseCleaner/database_cleaner/issues/347
+  #
+  # On MySQL, if the information schema is said to exist, we use an inaccurate
+  # row count leading to some tables not being cleaned when they should
+  def information_schema_exists?(_connection)
+    false
+  end
+end
+
+DatabaseCleaner::ActiveRecord::Deletion.prepend(FakeInformationSchema)
+
 RSpec.configure do |config|
+  # Ensure all sequences are reset at the start of the suite run
   config.before(:suite) do
     setup_database_cleaner
     DatabaseCleaner.clean_with(:truncation)
   end
 
   config.append_after(:context) do
-    DatabaseCleaner.clean_with(:truncation, cache_tables: false)
+    DatabaseCleaner.clean_with(:deletion, cache_tables: false)
   end
 
   config.before(:each) do
@@ -14,11 +30,7 @@ RSpec.configure do |config|
   end
 
   config.before(:each, :js) do
-    DatabaseCleaner.strategy = :truncation, { except: %w[licenses] }
-  end
-
-  config.before(:each, :truncate) do
-    DatabaseCleaner.strategy = :truncation, { except: %w[licenses] }
+    DatabaseCleaner.strategy = :deletion, { except: %w[licenses] }
   end
 
   config.before(:each, :delete) do
@@ -26,7 +38,7 @@ RSpec.configure do |config|
   end
 
   config.before(:each, :migration) do
-    DatabaseCleaner.strategy = :truncation, { cache_tables: false }
+    DatabaseCleaner.strategy = :deletion, { cache_tables: false }
   end
 
   config.before(:each) do

spec/support/features/discussion_comments_shared_example.rb

@@ -143,15 +143,17 @@ shared_examples 'discussion comments' do |resource_name|
         end
 
         if resource_name == 'merge request'
+          let(:note_id) { find("#{comments_selector} .note", match: :first)['data-note-id'] }
+
           it 'shows resolved discussion when toggled' do
             click_button "Resolve discussion"
 
-            expect(page).to have_selector('.note-row-1', visible: true)
+            expect(page).to have_selector(".note-row-#{note_id}", visible: true)
 
             refresh
             click_button "Toggle discussion"
 
-            expect(page).to have_selector('.note-row-1', visible: true)
+            expect(page).to have_selector(".note-row-#{note_id}", visible: true)
           end
         end
       end

spec/uploaders/job_artifact_uploader_spec.rb

@@ -9,7 +9,7 @@ describe JobArtifactUploader do
   describe '#store_dir' do
     subject { uploader.store_dir }
 
-    let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.project_id}/#{job_artifact.id}" }
+    let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" }
 
     context 'when using local storage' do
       it { is_expected.to start_with(local_path) }
@@ -57,7 +57,7 @@ describe JobArtifactUploader do
 
     it { is_expected.to start_with(local_path) }
     it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") }
-    it { is_expected.to include("/#{job_artifact.project_id}/") }
+    it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }
   end
 end