Resolve "Add more tests for spec/controllers/projects/builds_controller_spec.rb"

app/controllers/application_controller.rb

@@ -118,6 +118,10 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  def respond_422
+    head :unprocessable_entity
+  end
+
   def no_cache_headers
     response.headers["Cache-Control"] = "no-cache, no-store, max-age=0, must-revalidate"
     response.headers["Pragma"] = "no-cache"

app/controllers/projects/builds_controller.rb

@@ -1,6 +1,6 @@
 class Projects::BuildsController < Projects::ApplicationController
   before_action :build, except: [:index, :cancel_all]
-  before_action :authorize_read_build!, except: [:cancel, :cancel_all, :retry, :play]
+  before_action :authorize_read_build!, only: [:index, :show, :status, :raw, :trace]
   before_action :authorize_update_build!, except: [:index, :show, :status, :raw, :trace]
   layout 'project'
 
@@ -60,20 +60,22 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def retry
-    return render_404 unless @build.retryable?
+    return respond_422 unless @build.retryable?
 
     build = Ci::Build.retry(@build, current_user)
     redirect_to build_path(build)
   end
 
   def play
-    return render_404 unless @build.playable?
+    return respond_422 unless @build.playable?
 
     build = @build.play(current_user)
     redirect_to build_path(build)
   end
 
   def cancel
+    return respond_422 unless @build.cancelable?
+
     @build.cancel
     redirect_to build_path(@build)
   end
@@ -85,9 +87,12 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def erase
-    @build.erase(erased_by: current_user)
-    redirect_to namespace_project_build_path(project.namespace, project, @build),
+    if @build.erase(erased_by: current_user)
+      redirect_to namespace_project_build_path(project.namespace, project, @build),
                 notice: "Build has been successfully erased!"
+    else
+      respond_422
+    end
   end
 
   def raw

changelogs/unreleased/29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb.yml

@@ -0,0 +1,4 @@
+---
+title: Resolve "Add more tests for spec/controllers/projects/builds_controller_spec.rb"
+merge_request: 10244
+author: dosuken123

config/routes/project.rb

@@ -173,7 +173,7 @@ constraints(ProjectUrlConstrainer.new) do
           post :retry
           post :play
           post :erase
-          get :trace
+          get :trace, defaults: { format: 'json' }
           get :raw
         end
 

spec/controllers/projects/builds_controller_spec.rb

@@ -1,14 +1,69 @@
 require 'spec_helper'
 
 describe Projects::BuildsController do
-  let(:user) { create(:user) }
-  let(:project) { create(:empty_project, :public) }
+  include ApiHelpers
 
-  before do
-    sign_in(user)
-  end
+  let(:project) { create(:empty_project, :public) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:user) { create(:user) }
 
   describe 'GET index' do
+    context 'when scope is pending' do
+      before do
+        create(:ci_build, :pending, pipeline: pipeline)
+
+        get_index(scope: 'pending')
+      end
+
+      it 'has only pending builds' do
+        expect(response).to have_http_status(:ok)
+        expect(assigns(:builds).first.status).to eq('pending')
+      end
+    end
+
+    context 'when scope is running' do
+      before do
+        create(:ci_build, :running, pipeline: pipeline)
+
+        get_index(scope: 'running')
+      end
+
+      it 'has only running builds' do
+        expect(response).to have_http_status(:ok)
+        expect(assigns(:builds).first.status).to eq('running')
+      end
+    end
+
+    context 'when scope is finished' do
+      before do
+        create(:ci_build, :success, pipeline: pipeline)
+
+        get_index(scope: 'finished')
+      end
+
+      it 'has only finished builds' do
+        expect(response).to have_http_status(:ok)
+        expect(assigns(:builds).first.status).to eq('success')
+      end
+    end
+
+    context 'when page is specified' do
+      let(:last_page) { project.builds.page.total_pages }
+
+      context 'when page number is eligible' do
+        before do
+          create_list(:ci_build, 2, pipeline: pipeline)
+
+          get_index(page: last_page.to_param)
+        end
+
+        it 'redirects to the page' do
+          expect(response).to have_http_status(:ok)
+          expect(assigns(:builds).current_page).to eq(last_page)
+        end
+      end
+    end
+
     context 'number of queries' do
       before do
         Ci::Build::AVAILABLE_STATUSES.each do |status|
@@ -23,13 +78,8 @@ describe Projects::BuildsController do
         RequestStore.clear!
       end
 
-      def render
-        get :index, namespace_id: project.namespace,
-                    project_id: project
-      end
-
       it "verifies number of queries" do
-        recorded = ActiveRecord::QueryRecorder.new { render }
+        recorded = ActiveRecord::QueryRecorder.new { get_index }
         expect(recorded.count).to be_within(5).of(8)
       end
 
@@ -39,10 +89,83 @@ describe Projects::BuildsController do
           pipeline: pipeline, name: name, status: status)
       end
     end
+
+    def get_index(**extra_params)
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project
+      }
+
+      get :index, params.merge(extra_params)
+    end
+  end
+
+  describe 'GET show' do
+    context 'when build exists' do
+      let!(:build) { create(:ci_build, pipeline: pipeline) }
+
+      before do
+        get_show(id: build.id)
+      end
+
+      it 'has a build' do
+        expect(response).to have_http_status(:ok)
+        expect(assigns(:build).id).to eq(build.id)
+      end
+    end
+
+    context 'when build does not exist' do
+      before do
+        get_show(id: 1234)
+      end
+
+      it 'renders not_found' do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    def get_show(**extra_params)
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project
+      }
+
+      get :show, params.merge(extra_params)
+    end
+  end
+
+  describe 'GET trace.json' do
+    before do
+      get_trace
+    end
+
+    context 'when build has a trace' do
+      let(:build) { create(:ci_build, :trace, pipeline: pipeline) }
+
+      it 'returns a trace' do
+        expect(response).to have_http_status(:ok)
+        expect(json_response['html']).to eq('BUILD TRACE')
+      end
+    end
+
+    context 'when build has no traces' do
+      let(:build) { create(:ci_build, pipeline: pipeline) }
+
+      it 'returns no traces' do
+        expect(response).to have_http_status(:ok)
+        expect(json_response['html']).to be_nil
+      end
+    end
+
+    def get_trace
+      get :trace, namespace_id: project.namespace,
+                  project_id: project,
+                  id: build.id,
+                  format: :json
+    end
   end
 
   describe 'GET status.json' do
-    let(:pipeline) { create(:ci_pipeline, project: project) }
     let(:build) { create(:ci_build, pipeline: pipeline) }
     let(:status) { build.detailed_status(double('user')) }
 
@@ -71,6 +194,7 @@ describe Projects::BuildsController do
       before do
         project.add_developer(user)
         sign_in(user)
+
         get_trace
       end
 
@@ -84,6 +208,7 @@ describe Projects::BuildsController do
     context 'when user is logged in as non member' do
       before do
         sign_in(user)
+
         get_trace
       end
 
@@ -101,4 +226,221 @@ describe Projects::BuildsController do
                   format: :json
     end
   end
+
+  describe 'POST retry' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+
+      post_retry
+    end
+
+    context 'when build is retryable' do
+      let(:build) { create(:ci_build, :retryable, pipeline: pipeline) }
+
+      it 'redirects to the retried build page' do
+        expect(response).to have_http_status(:found)
+        expect(response).to redirect_to(namespace_project_build_path(id: Ci::Build.last.id))
+      end
+    end
+
+    context 'when build is not retryable' do
+      let(:build) { create(:ci_build, pipeline: pipeline) }
+
+      it 'renders unprocessable_entity' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+    end
+
+    def post_retry
+      post :retry, namespace_id: project.namespace,
+                   project_id: project,
+                   id: build.id
+    end
+  end
+
+  describe 'POST play' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+
+      post_play
+    end
+
+    context 'when build is playable' do
+      let(:build) { create(:ci_build, :playable, pipeline: pipeline) }
+
+      it 'redirects to the played build page' do
+        expect(response).to have_http_status(:found)
+        expect(response).to redirect_to(namespace_project_build_path(id: build.id))
+      end
+
+      it 'transits to pending' do
+        expect(build.reload).to be_pending
+      end
+    end
+
+    context 'when build is not playable' do
+      let(:build) { create(:ci_build, pipeline: pipeline) }
+
+      it 'renders unprocessable_entity' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+    end
+
+    def post_play
+      post :play, namespace_id: project.namespace,
+                  project_id: project,
+                  id: build.id
+    end
+  end
+
+  describe 'POST cancel' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+
+      post_cancel
+    end
+
+    context 'when build is cancelable' do
+      let(:build) { create(:ci_build, :cancelable, pipeline: pipeline) }
+
+      it 'redirects to the canceled build page' do
+        expect(response).to have_http_status(:found)
+        expect(response).to redirect_to(namespace_project_build_path(id: build.id))
+      end
+
+      it 'transits to canceled' do
+        expect(build.reload).to be_canceled
+      end
+    end
+
+    context 'when build is not cancelable' do
+      let(:build) { create(:ci_build, :canceled, pipeline: pipeline) }
+
+      it 'returns unprocessable_entity' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+    end
+
+    def post_cancel
+      post :cancel, namespace_id: project.namespace,
+                    project_id: project,
+                    id: build.id
+    end
+  end
+
+  describe 'POST cancel_all' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+    end
+
+    context 'when builds are cancelable' do
+      before do
+        create_list(:ci_build, 2, :cancelable, pipeline: pipeline)
+
+        post_cancel_all
+      end
+
+      it 'redirects to a index page' do
+        expect(response).to have_http_status(:found)
+        expect(response).to redirect_to(namespace_project_builds_path)
+      end
+
+      it 'transits to canceled' do
+        expect(Ci::Build.all).to all(be_canceled)
+      end
+    end
+
+    context 'when builds are not cancelable' do
+      before do
+        create_list(:ci_build, 2, :canceled, pipeline: pipeline)
+
+        post_cancel_all
+      end
+
+      it 'redirects to a index page' do
+        expect(response).to have_http_status(:found)
+        expect(response).to redirect_to(namespace_project_builds_path)
+      end
+    end
+
+    def post_cancel_all
+      post :cancel_all, namespace_id: project.namespace,
+                        project_id: project
+    end
+  end
+
+  describe 'POST erase' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+
+      post_erase
+    end
+
+    context 'when build is erasable' do
+      let(:build) { create(:ci_build, :erasable, :trace, pipeline: pipeline) }
+
+      it 'redirects to the erased build page' do
+        expect(response).to have_http_status(:found)
+        expect(response).to redirect_to(namespace_project_build_path(id: build.id))
+      end
+
+      it 'erases artifacts' do
+        expect(build.artifacts_file.exists?).to be_falsey
+        expect(build.artifacts_metadata.exists?).to be_falsey
+      end
+
+      it 'erases trace' do
+        expect(build.trace.exist?).to be_falsey
+      end
+    end
+
+    context 'when build is not erasable' do
+      let(:build) { create(:ci_build, :erased, pipeline: pipeline) }
+
+      it 'returns unprocessable_entity' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+    end
+
+    def post_erase
+      post :erase, namespace_id: project.namespace,
+                   project_id: project,
+                   id: build.id
+    end
+  end
+
+  describe 'GET raw' do
+    before do
+      get_raw
+    end
+
+    context 'when build has a trace file' do
+      let(:build) { create(:ci_build, :trace, pipeline: pipeline) }
+
+      it 'send a trace file' do
+        expect(response).to have_http_status(:ok)
+        expect(response.content_type).to eq 'text/plain; charset=utf-8'
+        expect(response.body).to eq 'BUILD TRACE'
+      end
+    end
+
+    context 'when build does not have a trace file' do
+      let(:build) { create(:ci_build, pipeline: pipeline) }
+
+      it 'returns not_found' do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    def get_raw
+      post :raw, namespace_id: project.namespace,
+                 project_id: project,
+                 id: build.id
+    end
+  end
 end

spec/factories/ci/builds.rb

@@ -79,6 +79,19 @@ FactoryGirl.define do
       manual
     end
 
+    trait :retryable do
+      success
+    end
+
+    trait :cancelable do
+      pending
+    end
+
+    trait :erasable do
+      success
+      artifacts
+    end
+
     trait :tags do
       tag_list [:docker, :ruby]
     end

spec/features/security/project/internal_access_spec.rb

@@ -399,6 +399,44 @@ describe "Internal Project Access", feature: true  do
     end
   end
 
+  describe 'GET /:project_path/builds/:id/trace' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:build) { create(:ci_build, pipeline: pipeline) }
+    subject { trace_namespace_project_build_path(project.namespace, project, build.id) }
+
+    context 'when allowed for public and internal' do
+      before do
+        project.update(public_builds: true)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_allowed_for(:guest).of(project) }
+      it { is_expected.to be_allowed_for(:user) }
+      it { is_expected.to be_denied_for(:external) }
+      it { is_expected.to be_denied_for(:visitor) }
+    end
+
+    context 'when disallowed for public and internal' do
+      before do
+        project.update(public_builds: false)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_denied_for(:guest).of(project) }
+      it { is_expected.to be_denied_for(:user) }
+      it { is_expected.to be_denied_for(:external) }
+      it { is_expected.to be_denied_for(:visitor) }
+    end
+  end
+
   describe "GET /:project_path/environments" do
     subject { namespace_project_environments_path(project.namespace, project) }
 

spec/features/security/project/private_access_spec.rb

@@ -388,6 +388,38 @@ describe "Private Project Access", feature: true  do
     end
   end
 
+  describe 'GET /:project_path/builds/:id/trace' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:build) { create(:ci_build, pipeline: pipeline) }
+    subject { trace_namespace_project_build_path(project.namespace, project, build.id) }
+
+    it { is_expected.to be_allowed_for(:admin) }
+    it { is_expected.to be_allowed_for(:owner).of(project) }
+    it { is_expected.to be_allowed_for(:master).of(project) }
+    it { is_expected.to be_allowed_for(:developer).of(project) }
+    it { is_expected.to be_allowed_for(:reporter).of(project) }
+    it { is_expected.to be_denied_for(:guest).of(project) }
+    it { is_expected.to be_denied_for(:user) }
+    it { is_expected.to be_denied_for(:external) }
+    it { is_expected.to be_denied_for(:visitor) }
+
+    context 'when public builds is enabled' do
+      before do
+        project.update(public_builds: true)
+      end
+
+      it { is_expected.to be_allowed_for(:guest).of(project) }
+    end
+
+    context 'when public builds is disabled' do
+      before do
+        project.update(public_builds: false)
+      end
+
+      it { is_expected.to be_denied_for(:guest).of(project) }
+    end
+  end
+
   describe "GET /:project_path/environments" do
     subject { namespace_project_environments_path(project.namespace, project) }
 

spec/features/security/project/public_access_spec.rb

@@ -219,6 +219,44 @@ describe "Public Project Access", feature: true  do
     end
   end
 
+  describe 'GET /:project_path/builds/:id/trace' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:build) { create(:ci_build, pipeline: pipeline) }
+    subject { trace_namespace_project_build_path(project.namespace, project, build.id) }
+
+    context 'when allowed for public' do
+      before do
+        project.update(public_builds: true)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_allowed_for(:guest).of(project) }
+      it { is_expected.to be_allowed_for(:user) }
+      it { is_expected.to be_allowed_for(:external) }
+      it { is_expected.to be_allowed_for(:visitor) }
+    end
+
+    context 'when disallowed for public' do
+      before do
+        project.update(public_builds: false)
+      end
+
+      it { is_expected.to be_allowed_for(:admin) }
+      it { is_expected.to be_allowed_for(:owner).of(project) }
+      it { is_expected.to be_allowed_for(:master).of(project) }
+      it { is_expected.to be_allowed_for(:developer).of(project) }
+      it { is_expected.to be_allowed_for(:reporter).of(project) }
+      it { is_expected.to be_denied_for(:guest).of(project) }
+      it { is_expected.to be_denied_for(:user) }
+      it { is_expected.to be_denied_for(:external) }
+      it { is_expected.to be_denied_for(:visitor) }
+    end
+  end
+
   describe "GET /:project_path/environments" do
     subject { namespace_project_environments_path(project.namespace, project) }
 

spec/support/matchers/access_matchers.rb

@@ -51,7 +51,7 @@ module AccessMatchers
       emulate_user(user, @membership)
       visit(url)
 
-      status_code != 404 && current_path != new_user_session_path
+      status_code == 200 && current_path != new_user_session_path
     end
 
     chain :of do |membership|
@@ -66,7 +66,7 @@ module AccessMatchers
       emulate_user(user, @membership)
       visit(url)
 
-      status_code == 404 || current_path == new_user_session_path
+      [401, 404].include?(status_code) || current_path == new_user_session_path
     end
 
     chain :of do |membership|