Merge branch 'ci/improve-ci-build-api' into 'master'

Improve CI builds API specs

This modifies a CI Runners Builds API, to improve performance, and add few missing examples.
Extracted from !2560 (cherry-picked + extended).

cc @ayufan 

See merge request !2698

2 files changed, 55 insertions, 93 deletions

diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index d2db77f6286..c1b6ecd329a 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -1,30 +1,3 @@
-# == Schema Information
-#
-# Table name: builds
-#
-#  id                 :integer          not null, primary key
-#  project_id         :integer
-#  status             :string(255)
-#  finished_at        :datetime
-#  trace              :text
-#  created_at         :datetime
-#  updated_at         :datetime
-#  started_at         :datetime
-#  runner_id          :integer
-#  commit_id          :integer
-#  coverage           :float
-#  commands           :text
-#  job_id             :integer
-#  name               :string(255)
-#  deploy             :boolean          default(FALSE)
-#  options            :text
-#  allow_failure      :boolean          default(FALSE), not null
-#  stage              :string(255)
-#  trigger_request_id :integer
-#
-
-# Read about factories at https://github.com/thoughtbot/factory_girl
-
 FactoryGirl.define do
   factory :ci_build, class: Ci::Build do
     name 'test'
@@ -65,5 +38,20 @@ FactoryGirl.define do
         build.trace = 'BUILD TRACE'
       end
     end
+
+    trait :artifacts do
+      after(:create) do |build, _|
+        build.artifacts_file =
+          fixture_file_upload(Rails.root +
+                              'spec/fixtures/ci_build_artifacts.zip',
+                              'application/zip')
+
+        build.artifacts_metadata =
+          fixture_file_upload(Rails.root +
+                              'spec/fixtures/ci_build_artifacts_metadata.gz',
+                              'application/x-gzip')
+        build.save!
+      end
+    end
   end
 end
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 244947762dd..01b369720ca 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -151,8 +151,8 @@ describe Ci::API::API do
     context "Artifacts" do
       let(:file_upload) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
       let(:file_upload2) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif') }
-      let(:commit) { FactoryGirl.create(:ci_commit, project: project) }
-      let(:build) { FactoryGirl.create(:ci_build, commit: commit, runner_id: runner.id) }
+      let(:commit) { create(:ci_commit, project: project) }
+      let(:build) { create(:ci_build, commit: commit, runner_id: runner.id) }
       let(:authorize_url) { ci_api("/builds/#{build.id}/artifacts/authorize") }
       let(:post_url) { ci_api("/builds/#{build.id}/artifacts") }
       let(:delete_url) { ci_api("/builds/#{build.id}/artifacts") }
@@ -160,12 +160,10 @@ describe Ci::API::API do
       let(:headers) { { "GitLab-Workhorse" => "1.0" } }
       let(:headers_with_token) { headers.merge(Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token) }
 
+      before { build.run! }
+
       describe "POST /builds/:id/artifacts/authorize" do
         context "should authorize posting artifact to running build" do
-          before do
-            build.run!
-          end
-
           it "using token as parameter" do
             post authorize_url, { token: build.token }, headers
             expect(response.status).to eq(200)
@@ -180,10 +178,6 @@ describe Ci::API::API do
         end
 
         context "should fail to post too large artifact" do
-          before do
-            build.run!
-          end
-
           it "using token as parameter" do
             stub_application_setting(max_artifacts_size: 0)
             post authorize_url, { token: build.token, filesize: 100 }, headers
@@ -197,8 +191,8 @@ describe Ci::API::API do
           end
         end
 
-        context "should get denied" do
-          it do
+        context 'token is invalid' do
+          it 'should respond with forbidden'do
             post authorize_url, { token: 'invalid', filesize: 100 }
             expect(response.status).to eq(403)
           end
@@ -206,17 +200,13 @@ describe Ci::API::API do
       end
 
       describe "POST /builds/:id/artifacts" do
-        context "Disable sanitizer" do
+        context "disable sanitizer" do
           before do
             # by configuring this path we allow to pass temp file from any path
             allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
           end
 
           context "should post artifact to running build" do
-            before do
-              build.run!
-            end
-
             it "uses regual file post" do
               upload_artifacts(file_upload, headers_with_token, false)
               expect(response.status).to eq(201)
@@ -244,10 +234,7 @@ describe Ci::API::API do
             let(:stored_artifacts_file) { build.reload.artifacts_file.file }
             let(:stored_metadata_file) { build.reload.artifacts_metadata.file }
 
-            before do
-              build.run!
-              post(post_url, post_data, headers_with_token)
-            end
+            before { post(post_url, post_data, headers_with_token) }
 
             context 'post data accelerated by workhorse is correct' do
               let(:post_data) do
@@ -257,11 +244,8 @@ describe Ci::API::API do
                   'metadata.name' => metadata.original_filename }
               end
 
-              it 'responds with valid status' do
-                expect(response.status).to eq(201)
-              end
-
               it 'stores artifacts and artifacts metadata' do
+                expect(response.status).to eq(201)
                 expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename)
                 expect(stored_metadata_file.original_filename).to eq(metadata.original_filename)
               end
@@ -282,56 +266,42 @@ describe Ci::API::API do
             end
           end
 
-
-          context "should fail to post too large artifact" do
-            before do
-              build.run!
-            end
-
-            it do
+          context "artifacts file is too large" do
+            it "should fail to post too large artifact" do
               stub_application_setting(max_artifacts_size: 0)
               upload_artifacts(file_upload, headers_with_token)
               expect(response.status).to eq(413)
             end
           end
 
-          context "should fail to post artifacts without file" do
-            before do
-              build.run!
-            end
-
-            it do
+          context "artifacts post request does not contain file" do
+            it "should fail to post artifacts without file" do
               post post_url, {}, headers_with_token
               expect(response.status).to eq(400)
             end
           end
 
-          context "should fail to post artifacts without GitLab-Workhorse" do
-            before do
-              build.run!
-            end
-
-            it do
+          context 'GitLab Workhorse is not configured' do
+            it "should fail to post artifacts without GitLab-Workhorse" do
               post post_url, { token: build.token }, {}
               expect(response.status).to eq(403)
             end
           end
         end
 
-        context "should fail to post artifacts for outside of tmp path" do
+        context "artifacts are being stored outside of tmp path" do
           before do
             # by configuring this path we allow to pass file from @tmpdir only
             # but all temporary files are stored in system tmp directory
             @tmpdir = Dir.mktmpdir
             allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
-            build.run!
           end
 
           after do
             FileUtils.remove_entry @tmpdir
           end
 
-          it do
+          it "should fail to post artifacts for outside of tmp path" do
             upload_artifacts(file_upload, headers_with_token)
             expect(response.status).to eq(400)
           end
@@ -349,33 +319,37 @@ describe Ci::API::API do
         end
       end
 
-      describe "DELETE /builds/:id/artifacts" do
-        before do
-          build.run!
-          post delete_url, token: build.token, file: file_upload
-        end
+      describe 'DELETE /builds/:id/artifacts' do
+        let(:build) { create(:ci_build, :artifacts) }
+        before { delete delete_url, token: build.token }
 
-        it "should delete artifact build" do
-          build.success
-          delete delete_url, token: build.token
+        it 'should remove build artifacts' do
           expect(response.status).to eq(200)
+          expect(build.artifacts_file.exists?).to be_falsy
+          expect(build.artifacts_metadata.exists?).to be_falsy
         end
       end
 
-      describe "GET /builds/:id/artifacts" do
-        before do
-          build.run!
-        end
+      describe 'GET /builds/:id/artifacts' do
+        before { get get_url, token: build.token }
 
-        it "should download artifact" do
-          build.update_attributes(artifacts_file: file_upload)
-          get get_url, token: build.token
-          expect(response.status).to eq(200)
+        context 'build has artifacts' do
+          let(:build) { create(:ci_build, :artifacts) }
+          let(:download_headers) do
+            { 'Content-Transfer-Encoding'=>'binary',
+              'Content-Disposition'=>'attachment; filename=ci_build_artifacts.zip' }
+          end
+
+          it 'should download artifact' do
+            expect(response.status).to eq(200)
+            expect(response.headers).to include download_headers
+          end
         end
 
-        it "should fail to download if no artifact uploaded" do
-          get get_url, token: build.token
-          expect(response.status).to eq(404)
+        context 'build does not has artifacts' do
+          it 'should respond with not found' do
+            expect(response.status).to eq(404)
+          end
         end
       end
     end