From 1e19737ca20fe53327094196bbc9758e5c9e03fa Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 11:52:58 +0100 Subject: [PATCH 1/7] Refactor feature tests for project builds page --- spec/features/projects/builds_spec.rb | 159 ++++++++++++++------------ 1 file changed, 84 insertions(+), 75 deletions(-) diff --git a/spec/features/projects/builds_spec.rb b/spec/features/projects/builds_spec.rb index a8022a5361f..eec1d337224 100644 --- a/spec/features/projects/builds_spec.rb +++ b/spec/features/projects/builds_spec.rb @@ -2,51 +2,56 @@ require 'spec_helper' require 'tempfile' describe "Builds" do - let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } + let(:artifacts_file) do + fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') + end + + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:pipeline) { create(:ci_pipeline, project: project) } + + let!(:build) { create(:ci_build, :trace, pipeline: pipeline) } + let!(:build2) { create(:ci_build) } before do - login_as(:user) - @commit = FactoryGirl.create :ci_pipeline - @build = FactoryGirl.create :ci_build, :trace, pipeline: @commit - @build2 = FactoryGirl.create :ci_build - @project = @commit.project - @project.team << [@user, :developer] + project.team << [user, :developer] + login_as(user) end describe "GET /:project/builds" do context "Pending scope" do before do - visit namespace_project_builds_path(@project.namespace, @project, scope: :pending) + visit namespace_project_builds_path(project.namespace, project, scope: :pending) end it "shows Pending tab builds" do expect(page).to have_link 'Cancel running' expect(page).to have_selector('.nav-links li.active', text: 'Pending') - expect(page).to have_content @build.short_sha - expect(page).to have_content @build.ref - expect(page).to have_content @build.name + expect(page).to have_content build.short_sha + expect(page).to have_content build.ref + expect(page).to have_content build.name end end context "Running scope" do before do - @build.run! - visit namespace_project_builds_path(@project.namespace, @project, scope: :running) + build.run! + visit namespace_project_builds_path(project.namespace, project, scope: :running) end it "shows Running tab builds" do expect(page).to have_selector('.nav-links li.active', text: 'Running') expect(page).to have_link 'Cancel running' - expect(page).to have_content @build.short_sha - expect(page).to have_content @build.ref - expect(page).to have_content @build.name + expect(page).to have_content build.short_sha + expect(page).to have_content build.ref + expect(page).to have_content build.name end end context "Finished scope" do before do - @build.run! - visit namespace_project_builds_path(@project.namespace, @project, scope: :finished) + build.run! + visit namespace_project_builds_path(project.namespace, project, scope: :finished) end it "shows Finished tab builds" do @@ -58,15 +63,15 @@ describe "Builds" do context "All builds" do before do - @project.builds.running_or_pending.each(&:success) - visit namespace_project_builds_path(@project.namespace, @project) + project.builds.running_or_pending.each(&:success) + visit namespace_project_builds_path(project.namespace, project) end it "shows All tab builds" do expect(page).to have_selector('.nav-links li.active', text: 'All') - expect(page).to have_content @build.short_sha - expect(page).to have_content @build.ref - expect(page).to have_content @build.name + expect(page).to have_content build.short_sha + expect(page).to have_content build.ref + expect(page).to have_content build.name expect(page).not_to have_link 'Cancel running' end end @@ -74,17 +79,17 @@ describe "Builds" do describe "POST /:project/builds/:id/cancel_all" do before do - @build.run! - visit namespace_project_builds_path(@project.namespace, @project) + build.run! + visit namespace_project_builds_path(project.namespace, project) click_link "Cancel running" end it 'shows all necessary content' do expect(page).to have_selector('.nav-links li.active', text: 'All') expect(page).to have_content 'canceled' - expect(page).to have_content @build.short_sha - expect(page).to have_content @build.ref - expect(page).to have_content @build.name + expect(page).to have_content build.short_sha + expect(page).to have_content build.ref + expect(page).to have_content build.name expect(page).not_to have_link 'Cancel running' end end @@ -92,20 +97,20 @@ describe "Builds" do describe "GET /:project/builds/:id" do context "Build from project" do before do - visit namespace_project_build_path(@project.namespace, @project, @build) + visit namespace_project_build_path(project.namespace, project, build) end it 'shows commit`s data' do expect(page.status_code).to eq(200) - expect(page).to have_content @commit.sha[0..7] - expect(page).to have_content @commit.git_commit_message - expect(page).to have_content @commit.git_author_name + expect(page).to have_content pipeline.sha[0..7] + expect(page).to have_content pipeline.git_commit_message + expect(page).to have_content pipeline.git_author_name end end context "Build from other project" do before do - visit namespace_project_build_path(@project.namespace, @project, @build2) + visit namespace_project_build_path(project.namespace, project, build2) end it { expect(page.status_code).to eq(404) } @@ -113,8 +118,8 @@ describe "Builds" do context "Download artifacts" do before do - @build.update_attributes(artifacts_file: artifacts_file) - visit namespace_project_build_path(@project.namespace, @project, @build) + build.update_attributes(artifacts_file: artifacts_file) + visit namespace_project_build_path(project.namespace, project, build) end it 'has button to download artifacts' do @@ -124,8 +129,8 @@ describe "Builds" do context 'Artifacts expire date' do before do - @build.update_attributes(artifacts_file: artifacts_file, artifacts_expire_at: expire_at) - visit namespace_project_build_path(@project.namespace, @project, @build) + build.update_attributes(artifacts_file: artifacts_file, artifacts_expire_at: expire_at) + visit namespace_project_build_path(project.namespace, project, build) end context 'no expire date defined' do @@ -160,8 +165,8 @@ describe "Builds" do context 'Build raw trace' do before do - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) end it do @@ -170,10 +175,14 @@ describe "Builds" do end describe 'Variables' do + let(:trigger_request) { create(:ci_trigger_request_with_variables) } + + let(:build) do + create :ci_build, pipeline: pipeline, trigger_request: trigger_request + end + before do - @trigger_request = create :ci_trigger_request_with_variables - @build = create :ci_build, pipeline: @commit, trigger_request: @trigger_request - visit namespace_project_build_path(@project.namespace, @project, @build) + visit namespace_project_build_path(project.namespace, project, build) end it 'shows variable key and value after click', js: true do @@ -193,8 +202,8 @@ describe "Builds" do describe "POST /:project/builds/:id/cancel" do context "Build from project" do before do - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) click_link "Cancel" end @@ -207,9 +216,9 @@ describe "Builds" do context "Build from other project" do before do - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) - page.driver.post(cancel_namespace_project_build_path(@project.namespace, @project, @build2)) + build.run! + visit namespace_project_build_path(project.namespace, project, build) + page.driver.post(cancel_namespace_project_build_path(project.namespace, project, build2)) end it { expect(page.status_code).to eq(404) } @@ -219,8 +228,8 @@ describe "Builds" do describe "POST /:project/builds/:id/retry" do context "Build from project" do before do - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) click_link 'Cancel' page.within('.build-header') do click_link 'Retry build' @@ -238,10 +247,10 @@ describe "Builds" do context "Build from other project" do before do - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) click_link 'Cancel' - page.driver.post(retry_namespace_project_build_path(@project.namespace, @project, @build2)) + page.driver.post(retry_namespace_project_build_path(project.namespace, project, build2)) end it { expect(page).to have_http_status(404) } @@ -249,13 +258,13 @@ describe "Builds" do context "Build that current user is not allowed to retry" do before do - @build.run! - @build.cancel! - @project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + build.run! + build.cancel! + project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) logout_direct login_with(create(:user)) - visit namespace_project_build_path(@project.namespace, @project, @build) + visit namespace_project_build_path(project.namespace, project, build) end it 'does not show the Retry button' do @@ -268,15 +277,15 @@ describe "Builds" do describe "GET /:project/builds/:id/download" do before do - @build.update_attributes(artifacts_file: artifacts_file) - visit namespace_project_build_path(@project.namespace, @project, @build) + build.update_attributes(artifacts_file: artifacts_file) + visit namespace_project_build_path(project.namespace, project, build) click_link 'Download' end context "Build from other project" do before do - @build2.update_attributes(artifacts_file: artifacts_file) - visit download_namespace_project_build_artifacts_path(@project.namespace, @project, @build2) + build2.update_attributes(artifacts_file: artifacts_file) + visit download_namespace_project_build_artifacts_path(project.namespace, project, build2) end it { expect(page.status_code).to eq(404) } @@ -288,23 +297,23 @@ describe "Builds" do context 'build from project' do before do Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile') - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) page.within('.js-build-sidebar') { click_link 'Raw' } end it 'sends the right headers' do expect(page.status_code).to eq(200) expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8') - expect(page.response_headers['X-Sendfile']).to eq(@build.path_to_trace) + expect(page.response_headers['X-Sendfile']).to eq(build.path_to_trace) end end context 'build from other project' do before do Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile') - @build2.run! - visit raw_namespace_project_build_path(@project.namespace, @project, @build2) + build2.run! + visit raw_namespace_project_build_path(project.namespace, project, build2) end it 'sends the right headers' do @@ -325,8 +334,8 @@ describe "Builds" do context 'when build has trace in file' do before do Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile') - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) allow_any_instance_of(Project).to receive(:ci_id).and_return(nil) allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(existing_file) @@ -345,8 +354,8 @@ describe "Builds" do context 'when build has trace in old file' do before do Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile') - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) allow_any_instance_of(Project).to receive(:ci_id).and_return(999) allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(non_existing_file) @@ -365,8 +374,8 @@ describe "Builds" do context 'when build has trace in DB' do before do Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile') - @build.run! - visit namespace_project_build_path(@project.namespace, @project, @build) + build.run! + visit namespace_project_build_path(project.namespace, project, build) allow_any_instance_of(Project).to receive(:ci_id).and_return(nil) allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(non_existing_file) @@ -385,7 +394,7 @@ describe "Builds" do describe "GET /:project/builds/:id/trace.json" do context "Build from project" do before do - visit trace_namespace_project_build_path(@project.namespace, @project, @build, format: :json) + visit trace_namespace_project_build_path(project.namespace, project, build, format: :json) end it { expect(page.status_code).to eq(200) } @@ -393,7 +402,7 @@ describe "Builds" do context "Build from other project" do before do - visit trace_namespace_project_build_path(@project.namespace, @project, @build2, format: :json) + visit trace_namespace_project_build_path(project.namespace, project, build2, format: :json) end it { expect(page.status_code).to eq(404) } @@ -403,7 +412,7 @@ describe "Builds" do describe "GET /:project/builds/:id/status" do context "Build from project" do before do - visit status_namespace_project_build_path(@project.namespace, @project, @build) + visit status_namespace_project_build_path(project.namespace, project, build) end it { expect(page.status_code).to eq(200) } @@ -411,7 +420,7 @@ describe "Builds" do context "Build from other project" do before do - visit status_namespace_project_build_path(@project.namespace, @project, @build2) + visit status_namespace_project_build_path(project.namespace, project, build2) end it { expect(page.status_code).to eq(404) } From e249a35161401e9b2449346132f1130c6cd1b824 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 12:19:46 +0100 Subject: [PATCH 2/7] Add missing specs for loading build HTML trace --- spec/features/projects/builds_spec.rb | 30 ++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/spec/features/projects/builds_spec.rb b/spec/features/projects/builds_spec.rb index eec1d337224..f0ad936ca69 100644 --- a/spec/features/projects/builds_spec.rb +++ b/spec/features/projects/builds_spec.rb @@ -1,11 +1,7 @@ require 'spec_helper' require 'tempfile' -describe "Builds" do - let(:artifacts_file) do - fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') - end - +feature 'Builds', :feature do let(:user) { create(:user) } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } @@ -13,6 +9,10 @@ describe "Builds" do let!(:build) { create(:ci_build, :trace, pipeline: pipeline) } let!(:build2) { create(:ci_build) } + let(:artifacts_file) do + fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') + end + before do project.team << [user, :developer] login_as(user) @@ -163,7 +163,7 @@ describe "Builds" do end end - context 'Build raw trace' do + feature 'Raw trace' do before do build.run! visit namespace_project_build_path(project.namespace, project, build) @@ -174,7 +174,23 @@ describe "Builds" do end end - describe 'Variables' do + feature 'HTML trace', :js do + before do + build.run! + + visit namespace_project_build_path(project.namespace, project, build) + end + + it 'loads build trace' do + expect(page).to have_content 'BUILD TRACE' + + build.append_trace(' and more trace', 11) + + expect(page).to have_content 'BUILD TRACE and more trace' + end + end + + feature 'Variables' do let(:trigger_request) { create(:ci_trigger_request_with_variables) } let(:build) do From 89e367f2e5c9e52e3b8f3e966b5e541d4e1cde72 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 13:15:10 +0100 Subject: [PATCH 3/7] Add missing feature tests for loading build trace See #24638 --- spec/features/projects/builds_spec.rb | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/spec/features/projects/builds_spec.rb b/spec/features/projects/builds_spec.rb index f0ad936ca69..eb50d397b29 100644 --- a/spec/features/projects/builds_spec.rb +++ b/spec/features/projects/builds_spec.rb @@ -181,12 +181,24 @@ feature 'Builds', :feature do visit namespace_project_build_path(project.namespace, project, build) end - it 'loads build trace' do - expect(page).to have_content 'BUILD TRACE' + context 'when build has an initial trace' do + it 'loads build trace' do + expect(page).to have_content 'BUILD TRACE' - build.append_trace(' and more trace', 11) + build.append_trace(' and more trace', 11) - expect(page).to have_content 'BUILD TRACE and more trace' + expect(page).to have_content 'BUILD TRACE and more trace' + end + end + + context 'when build does not have an initial trace' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'loads new trace' do + build.append_trace('build trace', 0) + + expect(page).to have_content 'build trace' + end end end From 07ce4c28b3c854a912a1b98d0715d0ed3f8c937d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 13:35:18 +0100 Subject: [PATCH 4/7] Improve reproducibility of build trace test example --- spec/features/projects/builds_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/features/projects/builds_spec.rb b/spec/features/projects/builds_spec.rb index eb50d397b29..a0ccc472d11 100644 --- a/spec/features/projects/builds_spec.rb +++ b/spec/features/projects/builds_spec.rb @@ -6,8 +6,8 @@ feature 'Builds', :feature do let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } - let!(:build) { create(:ci_build, :trace, pipeline: pipeline) } - let!(:build2) { create(:ci_build) } + let(:build) { create(:ci_build, :trace, pipeline: pipeline) } + let(:build2) { create(:ci_build) } let(:artifacts_file) do fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') @@ -19,6 +19,8 @@ feature 'Builds', :feature do end describe "GET /:project/builds" do + let!(:build) { create(:ci_build, pipeline: pipeline) } + context "Pending scope" do before do visit namespace_project_builds_path(project.namespace, project, scope: :pending) From 6a18a78184293863eafc9b9b1a9e7fd5687a507c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 13:37:23 +0100 Subject: [PATCH 5/7] Include build trace status in data attribute always --- app/helpers/builds_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/builds_helper.rb b/app/helpers/builds_helper.rb index fde297c588e..9662ac7e341 100644 --- a/app/helpers/builds_helper.rb +++ b/app/helpers/builds_helper.rb @@ -12,7 +12,7 @@ module BuildsHelper build_url: namespace_project_build_url(@project.namespace, @project, @build, :json), build_status: @build.status, build_stage: @build.stage, - state1: @build.trace_with_state[:state] + state1: @build.trace_with_state[:state].to_s } end end From 0914efbfceb32c0d303b5113bc13f86cc1b120cd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 13:40:43 +0100 Subject: [PATCH 6/7] Add Changelog entry for build trace exceptions fix --- changelogs/unreleased/fix-build-without-trace-exceptions.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/fix-build-without-trace-exceptions.yml diff --git a/changelogs/unreleased/fix-build-without-trace-exceptions.yml b/changelogs/unreleased/fix-build-without-trace-exceptions.yml new file mode 100644 index 00000000000..3b95e96e212 --- /dev/null +++ b/changelogs/unreleased/fix-build-without-trace-exceptions.yml @@ -0,0 +1,4 @@ +--- +title: Fix exceptions when loading build trace +merge_request: 7658 +author: From de24902852454a7805c30912eaca32903f491aa7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 22 Nov 2016 14:48:14 +0100 Subject: [PATCH 7/7] Improve name of build log state data attribute --- app/assets/javascripts/build.js | 2 +- app/helpers/builds_helper.rb | 2 +- spec/javascripts/fixtures/build.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/build.js b/app/assets/javascripts/build.js index e198306e67a..116a47b0907 100644 --- a/app/assets/javascripts/build.js +++ b/app/assets/javascripts/build.js @@ -12,7 +12,7 @@ this.pageUrl = options.pageUrl; this.buildUrl = options.buildUrl; this.buildStatus = options.buildStatus; - this.state = options.state1; + this.state = options.logState; this.buildStage = options.buildStage; this.updateDropdown = bind(this.updateDropdown, this); this.$document = $(document); diff --git a/app/helpers/builds_helper.rb b/app/helpers/builds_helper.rb index 9662ac7e341..9fc69e12266 100644 --- a/app/helpers/builds_helper.rb +++ b/app/helpers/builds_helper.rb @@ -12,7 +12,7 @@ module BuildsHelper build_url: namespace_project_build_url(@project.namespace, @project, @build, :json), build_status: @build.status, build_stage: @build.stage, - state1: @build.trace_with_state[:state].to_s + log_state: @build.trace_with_state[:state].to_s } end end diff --git a/spec/javascripts/fixtures/build.html.haml b/spec/javascripts/fixtures/build.html.haml index 27136beb14c..06b49516e5c 100644 --- a/spec/javascripts/fixtures/build.html.haml +++ b/spec/javascripts/fixtures/build.html.haml @@ -54,7 +54,7 @@ build_url: 'http://example.com/root/test-build/builds/2.json', build_status: 'passed', build_stage: 'test', - state1: 'buildstate' }} + log_state: 'buildstate' }} %p.build-detail-row The artifacts will be removed in