Merge branch 'master' into feature/runner-per-group

8 files changed, 142 insertions, 82 deletions

diff --git a/spec/services/applications/create_service_spec.rb b/spec/services/applications/create_service_spec.rb
index 47a2a9d6403..9c43b56744b 100644
--- a/spec/services/applications/create_service_spec.rb
+++ b/spec/services/applications/create_service_spec.rb
@@ -1,13 +1,17 @@
-require 'spec_helper'
+require "spec_helper"
 
 describe ::Applications::CreateService do
   let(:user) { create(:user) }
   let(:params) { attributes_for(:application) }
-  let(:request) { ActionController::TestRequest.new(remote_ip: '127.0.0.1') }
+  let(:request) do
+    if Gitlab.rails5?
+      ActionController::TestRequest.new({ remote_ip: "127.0.0.1" }, ActionController::TestSession.new)
+    else
+      ActionController::TestRequest.new(remote_ip: "127.0.0.1")
+    end
+  end
 
   subject { described_class.new(user, params) }
 
-  it 'creates an application' do
-    expect { subject.execute(request) }.to change { Doorkeeper::Application.count }.by(1)
-  end
+  it { expect { subject.execute(request) }.to change { Doorkeeper::Application.count }.by(1) }
 end
diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb
index 8de0bdf92e2..5bc6031388e 100644
--- a/spec/services/ci/retry_build_service_spec.rb
+++ b/spec/services/ci/retry_build_service_spec.rb
@@ -6,7 +6,9 @@ describe Ci::RetryBuildService do
   set(:pipeline) { create(:ci_pipeline, project: project) }
 
   let(:stage) do
-    Ci::Stage.create!(project: project, pipeline: pipeline, name: 'test')
+    create(:ci_stage_entity, project: project,
+                             pipeline: pipeline,
+                             name: 'test')
   end
 
   let(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) }
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index c38ddf4612b..e8568bf8bb3 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -219,7 +219,7 @@ describe MergeRequests::MergeService do
 
         service.execute(merge_request)
 
-        expect(merge_request.merge_error).to include(error_message)
+        expect(merge_request.merge_error).to include('Something went wrong during merge')
         expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
 
@@ -231,7 +231,7 @@ describe MergeRequests::MergeService do
 
         service.execute(merge_request)
 
-        expect(merge_request.merge_error).to include(error_message)
+        expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook')
         expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
 
diff --git a/spec/services/notes/resolve_service_spec.rb b/spec/services/notes/resolve_service_spec.rb
new file mode 100644
index 00000000000..b54d40a7a5c
--- /dev/null
+++ b/spec/services/notes/resolve_service_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe Notes::ResolveService do
+  let(:merge_request) { create(:merge_request) }
+  let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.project) }
+  let(:user) { merge_request.author }
+
+  describe '#execute' do
+    it "resolves the note" do
+      described_class.new(merge_request.project, user).execute(note)
+      note.reload
+
+      expect(note.resolved?).to be true
+      expect(note.resolved_by).to eq(user)
+    end
+
+    it "sends notifications if all discussions are resolved" do
+      expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
+
+      described_class.new(merge_request.project, user).execute(note)
+    end
+  end
+end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 55bbe954491..48ef5f3c115 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -96,6 +96,37 @@ describe NotificationService, :mailer do
     it_should_behave_like 'participating by assignee notification'
   end
 
+  describe '#async' do
+    let(:async) { notification.async }
+    set(:key) { create(:personal_key) }
+
+    it 'returns an Async object with the correct parent' do
+      expect(async).to be_a(described_class::Async)
+      expect(async.parent).to eq(notification)
+    end
+
+    context 'when receiving a public method' do
+      it 'schedules a MailScheduler::NotificationServiceWorker' do
+        expect(MailScheduler::NotificationServiceWorker)
+          .to receive(:perform_async).with('new_key', key)
+
+        async.new_key(key)
+      end
+    end
+
+    context 'when receiving a private method' do
+      it 'raises NoMethodError' do
+        expect { async.notifiable?(key) }.to raise_error(NoMethodError)
+      end
+    end
+
+    context 'when recieving a non-existent method' do
+      it 'raises NoMethodError' do
+        expect { async.foo(key) }.to raise_error(NoMethodError)
+      end
+    end
+  end
+
   describe 'Keys' do
     describe '#new_key' do
       let(:key_options) { {} }
@@ -982,6 +1013,8 @@ describe NotificationService, :mailer do
     let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' }
 
     before do
+      project.add_master(merge_request.author)
+      project.add_master(merge_request.assignee)
       build_team(merge_request.target_project)
       add_users_with_subscription(merge_request.target_project, merge_request)
       update_custom_notification(:new_merge_request, @u_guest_custom, resource: project)
@@ -1093,15 +1126,18 @@ describe NotificationService, :mailer do
     end
 
     describe '#reassigned_merge_request' do
+      let(:current_user) { create(:user) }
+
       before do
         update_custom_notification(:reassign_merge_request, @u_guest_custom, resource: project)
         update_custom_notification(:reassign_merge_request, @u_custom_global)
       end
 
       it do
-        notification.reassigned_merge_request(merge_request, merge_request.author)
+        notification.reassigned_merge_request(merge_request, current_user, merge_request.author)
 
         should_email(merge_request.assignee)
+        should_email(merge_request.author)
         should_email(@u_watcher)
         should_email(@u_participant_mentioned)
         should_email(@subscriber)
@@ -1116,7 +1152,7 @@ describe NotificationService, :mailer do
       end
 
       it 'adds "assigned" reason for new assignee' do
-        notification.reassigned_merge_request(merge_request, merge_request.author)
+        notification.reassigned_merge_request(merge_request, current_user, merge_request.author)
 
         email = find_email_for(merge_request.assignee)
 
@@ -1126,7 +1162,7 @@ describe NotificationService, :mailer do
       it_behaves_like 'participating notifications' do
         let(:participant) { create(:user, username: 'user-participant') }
         let(:issuable) { merge_request }
-        let(:notification_trigger) { notification.reassigned_merge_request(merge_request, @u_disabled) }
+        let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, merge_request.author) }
       end
     end
 
diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb
index 1b6caeab15d..347ac13828c 100644
--- a/spec/services/projects/update_pages_service_spec.rb
+++ b/spec/services/projects/update_pages_service_spec.rb
@@ -29,25 +29,10 @@ describe Projects::UpdatePagesService do
     end
 
     describe 'pages artifacts' do
-      context 'with expiry date' do
-        before do
-          build.artifacts_expire_in = "2 days"
-          build.save!
-        end
-
-        it "doesn't delete artifacts" do
-          expect(execute).to eq(:success)
-
-          expect(build.reload.artifacts?).to eq(true)
-        end
-      end
-
-      context 'without expiry date' do
-        it "does delete artifacts" do
-          expect(execute).to eq(:success)
+      it "doesn't delete artifacts after deploying" do
+        expect(execute).to eq(:success)
 
-          expect(build.reload.artifacts?).to eq(false)
-        end
+        expect(build.reload.artifacts?).to eq(true)
       end
     end
 
@@ -100,25 +85,10 @@ describe Projects::UpdatePagesService do
       end
 
       describe 'pages artifacts' do
-        context 'with expiry date' do
-          before do
-            build.artifacts_expire_in = "2 days"
-            build.save!
-          end
-
-          it "doesn't delete artifacts" do
-            expect(execute).to eq(:success)
-
-            expect(build.artifacts?).to eq(true)
-          end
-        end
-
-        context 'without expiry date' do
-          it "does delete artifacts" do
-            expect(execute).to eq(:success)
+        it "doesn't delete artifacts after deploying" do
+          expect(execute).to eq(:success)
 
-            expect(build.reload.artifacts?).to eq(false)
-          end
+          expect(build.artifacts?).to eq(true)
         end
       end
 
@@ -153,11 +123,13 @@ describe Projects::UpdatePagesService do
         expect(execute).not_to eq(:success)
       end
 
-      it 'fails for empty file fails' do
-        build.job_artifacts_archive.update_attributes(file: empty_file)
+      context 'when using empty file' do
+        let(:file) { empty_file }
 
-        expect { execute }
-          .to raise_error(Projects::UpdatePagesService::FailedToExtractError)
+        it 'fails to extract' do
+          expect { execute }
+            .to raise_error(Projects::UpdatePagesService::FailedToExtractError)
+        end
       end
 
       context 'when timeout happens by DNS error' do
@@ -171,13 +143,12 @@ describe Projects::UpdatePagesService do
 
           build.reload
           expect(deploy_status).to be_failed
-          expect(build.artifacts?).to be_truthy
         end
       end
 
       context 'when failed to extract zip artifacts' do
         before do
-          allow_any_instance_of(described_class)
+          expect_any_instance_of(described_class)
             .to receive(:extract_zip_archive!)
             .and_raise(Projects::UpdatePagesService::FailedToExtractError)
         end
@@ -188,21 +159,19 @@ describe Projects::UpdatePagesService do
 
           build.reload
           expect(deploy_status).to be_failed
-          expect(build.artifacts?).to be_truthy
         end
       end
 
       context 'when missing artifacts metadata' do
         before do
-          allow(build).to receive(:artifacts_metadata?).and_return(false)
+          expect(build).to receive(:artifacts_metadata?).and_return(false)
         end
 
-        it 'does not raise an error and remove artifacts as failed job' do
+        it 'does not raise an error as failed job' do
           execute
 
           build.reload
           expect(deploy_status).to be_failed
-          expect(build.artifacts?).to be_falsey
         end
       end
     end
diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb
index 2d7fa3f80f7..ab1c638fc39 100644
--- a/spec/services/repository_archive_clean_up_service_spec.rb
+++ b/spec/services/repository_archive_clean_up_service_spec.rb
@@ -1,15 +1,47 @@
 require 'spec_helper'
 
 describe RepositoryArchiveCleanUpService do
-  describe '#execute' do
-    subject(:service) { described_class.new }
+  subject(:service) { described_class.new }
 
+  describe '#execute (new archive locations)' do
+    let(:sha) { "0" * 40 }
+
+    it 'removes outdated archives and directories in a new-style path' do
+      in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 3.hours) do |dirname, files|
+        service.execute
+
+        files.each { |filename| expect(File.exist?(filename)).to be_falsy }
+        expect(File.directory?(dirname)).to be_falsy
+        expect(File.directory?(File.dirname(dirname))).to be_falsy
+      end
+    end
+
+    it 'does not remove directories when they contain outdated non-archives' do
+      in_directory_with_files("project-999/#{sha}", %w[tar conf rb], 2.hours) do |dirname, files|
+        service.execute
+
+        expect(File.directory?(dirname)).to be_truthy
+      end
+    end
+
+    it 'does not remove in-date archives in a new-style path' do
+      in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 1.hour) do |dirname, files|
+        service.execute
+
+        files.each { |filename| expect(File.exist?(filename)).to be_truthy }
+      end
+    end
+  end
+
+  describe '#execute (legacy archive locations)' do
     context 'when the downloads directory does not exist' do
       it 'does not remove any archives' do
         path = '/invalid/path/'
         stub_repository_downloads_path(path)
 
+        allow(File).to receive(:directory?).and_call_original
         expect(File).to receive(:directory?).with(path).and_return(false)
+
         expect(service).not_to receive(:clean_up_old_archives)
         expect(service).not_to receive(:clean_up_empty_directories)
 
@@ -19,7 +51,7 @@ describe RepositoryArchiveCleanUpService do
 
     context 'when the downloads directory exists' do
       shared_examples 'invalid archive files' do |dirname, extensions, mtime|
-        it 'does not remove files and directoy' do
+        it 'does not remove files and directory' do
           in_directory_with_files(dirname, extensions, mtime) do |dir, files|
             service.execute
 
@@ -43,7 +75,7 @@ describe RepositoryArchiveCleanUpService do
       end
 
       context 'with files older than 2 hours inside invalid directories' do
-        it_behaves_like 'invalid archive files', 'john_doe/sample.git', %w[conf rb tar tar.gz], 2.hours
+        it_behaves_like 'invalid archive files', 'john/doe/sample.git', %w[conf rb tar tar.gz], 2.hours
       end
 
       context 'with files newer than 2 hours that matches valid archive extensions' do
@@ -58,24 +90,24 @@ describe RepositoryArchiveCleanUpService do
         it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb tar tar.gz], 1.hour
       end
     end
+  end
 
-    def in_directory_with_files(dirname, extensions, mtime)
-      Dir.mktmpdir do |tmpdir|
-        stub_repository_downloads_path(tmpdir)
-        dir = File.join(tmpdir, dirname)
-        files = create_temporary_files(dir, extensions, mtime)
+  def in_directory_with_files(dirname, extensions, mtime)
+    Dir.mktmpdir do |tmpdir|
+      stub_repository_downloads_path(tmpdir)
+      dir = File.join(tmpdir, dirname)
+      files = create_temporary_files(dir, extensions, mtime)
 
-        yield(dir, files)
-      end
+      yield(dir, files)
     end
+  end
 
-    def stub_repository_downloads_path(path)
-      allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path)
-    end
+  def stub_repository_downloads_path(path)
+    allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path)
+  end
 
-    def create_temporary_files(dir, extensions, mtime)
-      FileUtils.mkdir_p(dir)
-      FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime)
-    end
+  def create_temporary_files(dir, extensions, mtime)
+    FileUtils.mkdir_p(dir)
+    FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime)
   end
 end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 893804f1470..e28b0ea5cf2 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -909,13 +909,7 @@ describe SystemNoteService do
       it 'sets the note text' do
         noteable.update_attribute(:time_estimate, 277200)
 
-        expect(subject.note).to eq "changed time estimate to 1w 4d 5h,"
-      end
-
-      it 'appends a comma to separate the note from the update_at time' do
-        noteable.update_attribute(:time_estimate, 277200)
-
-        expect(subject.note).to end_with(',')
+        expect(subject.note).to eq "changed time estimate to 1w 4d 5h"
       end
     end