Merge branch 'pb-job-trace-polling-fix' into 'master'

Job Trace polling solution

See merge request gitlab-org/gitlab!49982

app/assets/javascripts/jobs/store/actions.js

@@ -21,7 +21,8 @@ export const init = ({ dispatch }, { endpoint, logState, pagePath }) => {
     logState,
     pagePath,
   });
-  dispatch('fetchJob');
+
+  return Promise.all([dispatch('fetchJob'), dispatch('fetchTrace')]);
 };
 
 export const setJobEndpoint = ({ commit }, endpoint) => commit(types.SET_JOB_ENDPOINT, endpoint);
@@ -39,7 +40,6 @@ export const toggleSidebar = ({ dispatch, state }) => {
 };
 
 let eTagPoll;
-let isTraceReadyForRender;
 
 export const clearEtagPoll = () => {
   eTagPoll = null;
@@ -71,14 +71,7 @@ export const fetchJob = ({ state, dispatch }) => {
   });
 
   if (!Visibility.hidden()) {
-    // eslint-disable-next-line promise/catch-or-return
-    eTagPoll.makeRequest().then(() => {
-      // if a job is canceled we still need to dispatch
-      // fetchTrace to get the trace so we check for has_trace
-      if (state.job.started || state.job.has_trace) {
-        dispatch('fetchTrace');
-      }
-    });
+    eTagPoll.makeRequest();
   } else {
     axios
       .get(state.jobEndpoint)
@@ -88,15 +81,9 @@ export const fetchJob = ({ state, dispatch }) => {
 
   Visibility.change(() => {
     if (!Visibility.hidden()) {
-      // This check is needed to ensure the loading icon
-      // is not shown for a finished job during a visibility change
-      if (!isTraceReadyForRender && state.job.started) {
-        dispatch('startPollingTrace');
-      }
       dispatch('restartPolling');
     } else {
       dispatch('stopPolling');
-      dispatch('stopPollingTrace');
     }
   });
 };
@@ -177,8 +164,6 @@ export const fetchTrace = ({ dispatch, state }) =>
       params: { state: state.traceState },
     })
     .then(({ data }) => {
-      isTraceReadyForRender = data.complete;
-
       dispatch('toggleScrollisInBottom', isScrolledToBottom());
       dispatch('receiveTraceSuccess', data);
 
@@ -258,7 +243,7 @@ export const receiveJobsForStageError = ({ commit }) => {
   flash(__('An error occurred while fetching the jobs.'));
 };
 
-export const triggerManualJob = ({ state, dispatch }, variables) => {
+export const triggerManualJob = ({ state }, variables) => {
   const parsedVariables = variables.map((variable) => {
     const copyVar = { ...variable };
     delete copyVar.id;
@@ -269,6 +254,5 @@ export const triggerManualJob = ({ state, dispatch }, variables) => {
     .post(state.job.status.action.path, {
       job_variables_attributes: parsedVariables,
     })
-    .then(() => dispatch('fetchTrace'))
     .catch(() => flash(__('An error occurred while triggering the job.')));
 };

app/assets/javascripts/jobs/store/mutations.js

@@ -49,7 +49,6 @@ export default {
 
   [types.SET_TRACE_TIMEOUT](state, id) {
     state.traceTimeout = id;
-    state.isTraceComplete = false;
   },
 
   /**

app/controllers/projects/jobs_controller.rb

@@ -49,21 +49,25 @@ class Projects::JobsController < Projects::ApplicationController
   # rubocop: enable CodeReuse/ActiveRecord
 
   def trace
-    @build.trace.read do |stream|
-      respond_to do |format|
-        format.json do
-          @build.trace.being_watched!
-
-          build_trace = Ci::BuildTrace.new(
-            build: @build,
-            stream: stream,
-            state: params[:state])
-
-          render json: BuildTraceSerializer
-            .new(project: @project, current_user: @current_user)
-            .represent(build_trace)
+    @build.trace.being_watched! if @build.running?
+
+    if @build.has_trace?
+      @build.trace.read do |stream|
+        respond_to do |format|
+          format.json do
+            build_trace = Ci::BuildTrace.new(
+              build: @build,
+              stream: stream,
+              state: params[:state])
+
+            render json: BuildTraceSerializer
+              .new(project: @project, current_user: @current_user)
+              .represent(build_trace)
+          end
         end
       end
+    else
+      head :no_content
     end
   end
 

spec/controllers/projects/jobs_controller_spec.rb

@@ -700,10 +700,22 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         expect(json_response['lines']).to eq [{ 'content' => [{ 'text' => 'BUILD TRACE' }], 'offset' => 0 }]
       end
 
-      it 'sets being-watched flag for the job' do
-        expect(response).to have_gitlab_http_status(:ok)
+      context 'when job is running' do
+        let(:job) { create(:ci_build, :trace_live, :running, pipeline: pipeline) }
+
+        it 'sets being-watched flag for the job' do
+          expect(response).to have_gitlab_http_status(:ok)
+
+          expect(job.trace.being_watched?).to be(true)
+        end
+      end
 
-        expect(job.trace.being_watched?).to be(true)
+      context 'when job is not running' do
+        it 'does not set being-watched flag for the job' do
+          expect(response).to have_gitlab_http_status(:ok)
+
+          expect(job.trace.being_watched?).to be(false)
+        end
       end
     end
 
@@ -711,11 +723,7 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do
       let(:job) { create(:ci_build, pipeline: pipeline) }
 
       it 'returns no traces' do
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to match_response_schema('job/build_trace')
-        expect(json_response['id']).to eq job.id
-        expect(json_response['status']).to eq job.status
-        expect(json_response['lines']).to be_nil
+        expect(response).to have_gitlab_http_status(:no_content)
       end
     end
 

spec/features/projects/jobs_spec.rb

@@ -1212,9 +1212,11 @@ RSpec.describe 'Jobs', :clean_gitlab_redis_shared_state do
   end
 
   describe "GET /:project/jobs/:id/trace.json" do
+    let(:build) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
+
     context "Job from project" do
       before do
-        visit trace_project_job_path(project, job, format: :json)
+        visit trace_project_job_path(project, build, format: :json)
       end
 
       it { expect(page.status_code).to eq(200) }

spec/features/security/project/internal_access_spec.rb

@@ -430,7 +430,7 @@ RSpec.describe "Internal Project Access" do
 
   describe 'GET /:project_path/builds/:id/trace' do
     let(:pipeline) { create(:ci_pipeline, project: project) }
-    let(:build) { create(:ci_build, pipeline: pipeline) }
+    let(:build) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
 
     subject { trace_project_job_path(project, build.id) }
 

spec/features/security/project/private_access_spec.rb

@@ -423,7 +423,7 @@ RSpec.describe "Private Project Access" do
 
   describe 'GET /:project_path/builds/:id/trace' do
     let(:pipeline) { create(:ci_pipeline, project: project) }
-    let(:build) { create(:ci_build, pipeline: pipeline) }
+    let(:build) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
 
     subject { trace_project_job_path(project, build.id) }
 

spec/features/security/project/public_access_spec.rb

@@ -238,7 +238,7 @@ RSpec.describe "Public Project Access" do
 
   describe 'GET /:project_path/builds/:id/trace' do
     let(:pipeline) { create(:ci_pipeline, project: project) }
-    let(:build) { create(:ci_build, pipeline: pipeline) }
+    let(:build) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
 
     subject { trace_project_job_path(project, build.id) }
 

spec/frontend/jobs/store/actions_spec.js

@@ -27,7 +27,6 @@ import {
   hideSidebar,
   showSidebar,
   toggleSidebar,
-  triggerManualJob,
 } from '~/jobs/store/actions';
 import state from '~/jobs/store/state';
 import * as types from '~/jobs/store/mutation_types';
@@ -159,32 +158,6 @@ describe('Job State actions', () => {
         );
       });
     });
-
-    it('fetchTrace is called only if the job has started or has a trace', (done) => {
-      mock.onGet(`${TEST_HOST}/endpoint.json`).replyOnce(200, { id: 121212, name: 'karma' });
-
-      mockedState.job.started = true;
-
-      testAction(
-        fetchJob,
-        null,
-        mockedState,
-        [],
-        [
-          {
-            type: 'requestJob',
-          },
-          {
-            payload: { id: 121212, name: 'karma' },
-            type: 'receiveJobSuccess',
-          },
-          {
-            type: 'fetchTrace',
-          },
-        ],
-        done,
-      );
-    });
   });
 
   describe('receiveJobSuccess', () => {
@@ -536,43 +509,4 @@ describe('Job State actions', () => {
       );
     });
   });
-
-  describe('triggerManualJob', () => {
-    let mock;
-
-    beforeEach(() => {
-      mock = new MockAdapter(axios);
-    });
-
-    afterEach(() => {
-      mock.restore();
-    });
-
-    it('should dispatch fetchTrace', (done) => {
-      const playManualJobEndpoint = `${TEST_HOST}/manual-job/jobs/1000/play`;
-
-      mock.onPost(playManualJobEndpoint).reply(200);
-
-      mockedState.job = {
-        status: {
-          action: {
-            path: playManualJobEndpoint,
-          },
-        },
-      };
-
-      testAction(
-        triggerManualJob,
-        [{ id: '1', key: 'test_var', secret_value: 'test_value' }],
-        mockedState,
-        [],
-        [
-          {
-            type: 'fetchTrace',
-          },
-        ],
-        done,
-      );
-    });
-  });
 });

spec/frontend/jobs/store/mutations_spec.js

@@ -153,7 +153,6 @@ describe('Jobs Store Mutations', () => {
       mutations[types.SET_TRACE_TIMEOUT](stateCopy, id);
 
       expect(stateCopy.traceTimeout).toEqual(id);
-      expect(stateCopy.isTraceComplete).toBe(false);
     });
   });