Merge branch '55252-fix-fe-job-app-timeout-in-state' into 'master'

Step 1 - Fix `job_app` initialization and cleaning up of side-effects

See merge request gitlab-org/gitlab!23838

app/assets/javascripts/jobs/components/job_app.vue

@@ -8,7 +8,6 @@ import { polyfillSticky } from '~/lib/utils/sticky';
 import CiHeader from '~/vue_shared/components/header_ci_component.vue';
 import Callout from '~/vue_shared/components/callout.vue';
 import Icon from '~/vue_shared/components/icon.vue';
-import createStore from '../store';
 import EmptyState from './empty_state.vue';
 import EnvironmentsBlock from './environments_block.vue';
 import ErasedBlock from './erased_block.vue';
@@ -22,7 +21,6 @@ import { isNewJobLogActive } from '../store/utils';
 
 export default {
   name: 'JobPageApp',
-  store: createStore(),
   components: {
     CiHeader,
     Callout,
@@ -60,27 +58,15 @@ export default {
       required: false,
       default: null,
     },
-    endpoint: {
-      type: String,
-      required: true,
-    },
     terminalPath: {
       type: String,
       required: false,
       default: null,
     },
-    pagePath: {
-      type: String,
-      required: true,
-    },
     projectPath: {
       type: String,
       required: true,
     },
-    logState: {
-      type: String,
-      required: true,
-    },
     subscriptionsMoreMinutesUrl: {
       type: String,
       required: false,
@@ -161,37 +147,28 @@ export default {
   created() {
     this.throttled = _.throttle(this.toggleScrollButtons, 100);
 
-    this.setJobEndpoint(this.endpoint);
-    this.setTraceOptions({
-      logState: this.logState,
-      pagePath: this.pagePath,
-    });
-
-    this.fetchJob();
-    this.fetchTrace();
-
     window.addEventListener('resize', this.onResize);
     window.addEventListener('scroll', this.updateScroll);
   },
   mounted() {
     this.updateSidebar();
   },
-  destroyed() {
+  beforeDestroy() {
+    this.stopPollingTrace();
+    this.stopPolling();
     window.removeEventListener('resize', this.onResize);
     window.removeEventListener('scroll', this.updateScroll);
   },
   methods: {
     ...mapActions([
-      'setJobEndpoint',
-      'setTraceOptions',
-      'fetchJob',
       'fetchJobsForStage',
       'hideSidebar',
       'showSidebar',
       'toggleSidebar',
-      'fetchTrace',
       'scrollBottom',
       'scrollTop',
+      'stopPollingTrace',
+      'stopPolling',
       'toggleScrollButtons',
       'toggleScrollAnimation',
     ]),
@@ -223,7 +200,7 @@ export default {
   <div>
     <gl-loading-icon
       v-if="isLoading"
-      :size="2"
+      size="lg"
       class="js-job-loading qa-loading-animation prepend-top-20"
     />
 

app/assets/javascripts/jobs/index.js

 import Vue from 'vue';
 import JobApp from './components/job_app.vue';
+import createStore from './store';
 
 export default () => {
   const element = document.getElementById('js-job-vue-app');
 
+  const store = createStore();
+
+  // Let's start initializing the store (i.e. fetching data) right away
+  store.dispatch('init', element.dataset);
+
   return new Vue({
     el: element,
+    store,
     components: {
       JobApp,
     },

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

@@ -14,6 +14,16 @@ import {
   scrollUp,
 } from '~/lib/utils/scroll_utils';
 
+export const init = ({ dispatch }, { endpoint, logState, pagePath }) => {
+  dispatch('setJobEndpoint', endpoint);
+  dispatch('setTraceOptions', {
+    logState,
+    pagePath,
+  });
+
+  return Promise.all([dispatch('fetchJob'), dispatch('fetchTrace')]);
+};
+
 export const setJobEndpoint = ({ commit }, endpoint) => commit(types.SET_JOB_ENDPOINT, endpoint);
 export const setTraceOptions = ({ commit }, options) => commit(types.SET_TRACE_OPTIONS, options);
 
@@ -147,7 +157,6 @@ export const toggleScrollisInBottom = ({ commit }, toggle) => {
 
 export const requestTrace = ({ commit }) => commit(types.REQUEST_TRACE);
 
-let traceTimeout;
 export const fetchTrace = ({ dispatch, state }) =>
   axios
     .get(`${state.traceEndpoint}/trace.json`, {
@@ -157,24 +166,32 @@ export const fetchTrace = ({ dispatch, state }) =>
       dispatch('toggleScrollisInBottom', isScrolledToBottom());
       dispatch('receiveTraceSuccess', data);
 
-      if (!data.complete) {
-        traceTimeout = setTimeout(() => {
-          dispatch('fetchTrace');
-        }, 4000);
-      } else {
+      if (data.complete) {
         dispatch('stopPollingTrace');
+      } else if (!state.traceTimeout) {
+        dispatch('startPollingTrace');
       }
     })
     .catch(() => dispatch('receiveTraceError'));
 
-export const stopPollingTrace = ({ commit }) => {
+export const startPollingTrace = ({ dispatch, commit }) => {
+  const traceTimeout = setTimeout(() => {
+    commit(types.SET_TRACE_TIMEOUT, 0);
+    dispatch('fetchTrace');
+  }, 4000);
+
+  commit(types.SET_TRACE_TIMEOUT, traceTimeout);
+};
+
+export const stopPollingTrace = ({ state, commit }) => {
+  clearTimeout(state.traceTimeout);
+  commit(types.SET_TRACE_TIMEOUT, 0);
   commit(types.STOP_POLLING_TRACE);
-  clearTimeout(traceTimeout);
 };
+
 export const receiveTraceSuccess = ({ commit }, log) => commit(types.RECEIVE_TRACE_SUCCESS, log);
-export const receiveTraceError = ({ commit }) => {
-  commit(types.RECEIVE_TRACE_ERROR);
-  clearTimeout(traceTimeout);
+export const receiveTraceError = ({ dispatch }) => {
+  dispatch('stopPollingTrace');
   flash(__('An error occurred while fetching the job log.'));
 };
 /**

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

@@ -10,7 +10,6 @@ export const DISABLE_SCROLL_BOTTOM = 'DISABLE_SCROLL_BOTTOM';
 export const DISABLE_SCROLL_TOP = 'DISABLE_SCROLL_TOP';
 export const ENABLE_SCROLL_BOTTOM = 'ENABLE_SCROLL_BOTTOM';
 export const ENABLE_SCROLL_TOP = 'ENABLE_SCROLL_TOP';
-// TODO
 export const TOGGLE_SCROLL_ANIMATION = 'TOGGLE_SCROLL_ANIMATION';
 
 export const TOGGLE_IS_SCROLL_IN_BOTTOM_BEFORE_UPDATING_TRACE = 'TOGGLE_IS_SCROLL_IN_BOTTOM';
@@ -20,6 +19,7 @@ export const RECEIVE_JOB_SUCCESS = 'RECEIVE_JOB_SUCCESS';
 export const RECEIVE_JOB_ERROR = 'RECEIVE_JOB_ERROR';
 
 export const REQUEST_TRACE = 'REQUEST_TRACE';
+export const SET_TRACE_TIMEOUT = 'SET_TRACE_TIMEOUT';
 export const STOP_POLLING_TRACE = 'STOP_POLLING_TRACE';
 export const RECEIVE_TRACE_SUCCESS = 'RECEIVE_TRACE_SUCCESS';
 export const RECEIVE_TRACE_ERROR = 'RECEIVE_TRACE_ERROR';

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

@@ -53,17 +53,14 @@ export default {
     state.isTraceComplete = log.complete || state.isTraceComplete;
   },
 
-  /**
-   * Will remove loading animation
-   */
-  [types.STOP_POLLING_TRACE](state) {
-    state.isTraceComplete = true;
+  [types.SET_TRACE_TIMEOUT](state, id) {
+    state.traceTimeout = id;
   },
 
   /**
    * Will remove loading animation
    */
-  [types.RECEIVE_TRACE_ERROR](state) {
+  [types.STOP_POLLING_TRACE](state) {
     state.isTraceComplete = true;
   },
 

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

@@ -22,6 +22,7 @@ export default () => ({
   isTraceComplete: false,
   traceSize: 0,
   isTraceSizeVisible: false,
+  traceTimeout: 0,
 
   // used as a query parameter to fetch the trace
   traceState: null,

spec/frontend/jobs/store/mutations_spec.js

@@ -157,17 +157,21 @@ describe('Jobs Store Mutations', () => {
     });
   });
 
-  describe('STOP_POLLING_TRACE', () => {
-    it('sets isTraceComplete to true', () => {
-      mutations[types.STOP_POLLING_TRACE](stateCopy);
+  describe('SET_TRACE_TIMEOUT', () => {
+    it('sets the traceTimeout id', () => {
+      const id = 7;
 
-      expect(stateCopy.isTraceComplete).toEqual(true);
+      expect(stateCopy.traceTimeout).not.toEqual(id);
+
+      mutations[types.SET_TRACE_TIMEOUT](stateCopy, id);
+
+      expect(stateCopy.traceTimeout).toEqual(id);
     });
   });
 
-  describe('RECEIVE_TRACE_ERROR', () => {
-    it('resets trace state and sets error to true', () => {
-      mutations[types.RECEIVE_TRACE_ERROR](stateCopy);
+  describe('STOP_POLLING_TRACE', () => {
+    it('sets isTraceComplete to true', () => {
+      mutations[types.STOP_POLLING_TRACE](stateCopy);
 
       expect(stateCopy.isTraceComplete).toEqual(true);
     });

spec/javascripts/jobs/components/job_app_spec.js

@@ -6,7 +6,6 @@ import axios from '~/lib/utils/axios_utils';
 import jobApp from '~/jobs/components/job_app.vue';
 import createStore from '~/jobs/store';
 import * as types from '~/jobs/store/mutation_types';
-import { resetStore } from '../store/helpers';
 import job from '../mock_data';
 
 describe('Job App ', () => {
@@ -16,24 +15,29 @@ describe('Job App ', () => {
   let vm;
   let mock;
 
-  const props = {
+  const initSettings = {
     endpoint: `${gl.TEST_HOST}jobs/123.json`,
+    pagePath: `${gl.TEST_HOST}jobs/123`,
+    logState:
+      'eyJvZmZzZXQiOjE3NDUxLCJuX29wZW5fdGFncyI6MCwiZmdfY29sb3IiOm51bGwsImJnX2NvbG9yIjpudWxsLCJzdHlsZV9tYXNrIjowfQ%3D%3D',
+  };
+
+  const props = {
     runnerHelpUrl: 'help/runner',
     deploymentHelpUrl: 'help/deployment',
     runnerSettingsUrl: 'settings/ci-cd/runners',
     variablesSettingsUrl: 'settings/ci-cd/variables',
     terminalPath: 'jobs/123/terminal',
-    pagePath: `${gl.TEST_HOST}jobs/123`,
     projectPath: 'user-name/project-name',
     subscriptionsMoreMinutesUrl: 'https://customers.gitlab.com/buy_pipeline_minutes',
-    logState:
-      'eyJvZmZzZXQiOjE3NDUxLCJuX29wZW5fdGFncyI6MCwiZmdfY29sb3IiOm51bGwsImJnX2NvbG9yIjpudWxsLCJzdHlsZV9tYXNrIjowfQ%3D%3D',
   };
 
   const waitForJobReceived = () => waitForMutation(store, types.RECEIVE_JOB_SUCCESS);
   const setupAndMount = ({ jobData = {}, traceData = {} } = {}) => {
-    mock.onGet(props.endpoint).replyOnce(200, { ...job, ...jobData });
-    mock.onGet(`${props.pagePath}/trace.json`).reply(200, traceData);
+    mock.onGet(initSettings.endpoint).replyOnce(200, { ...job, ...jobData });
+    mock.onGet(`${initSettings.pagePath}/trace.json`).reply(200, traceData);
+
+    store.dispatch('init', initSettings);
 
     vm = mountComponentWithStore(Component, { props, store });
 
@@ -46,7 +50,6 @@ describe('Job App ', () => {
   });
 
   afterEach(() => {
-    resetStore(store);
     vm.$destroy();
     mock.restore();
   });
@@ -384,7 +387,6 @@ describe('Job App ', () => {
           })
           .then(done)
           .catch(done.fail);
-        done();
       });
 
       it('displays remaining time for a delayed job', done => {

spec/javascripts/jobs/store/actions_spec.js

@@ -15,6 +15,7 @@ import {
   scrollBottom,
   requestTrace,
   fetchTrace,
+  startPollingTrace,
   stopPollingTrace,
   receiveTraceSuccess,
   receiveTraceError,
@@ -241,6 +242,50 @@ describe('Job State actions', () => {
           done,
         );
       });
+
+      describe('when job is incomplete', () => {
+        let tracePayload;
+
+        beforeEach(() => {
+          tracePayload = {
+            html: 'I, [2018-08-17T22:57:45.707325 #1841]  INFO -- :',
+            complete: false,
+          };
+
+          mock.onGet(`${TEST_HOST}/endpoint/trace.json`).replyOnce(200, tracePayload);
+        });
+
+        it('dispatches startPollingTrace', done => {
+          testAction(
+            fetchTrace,
+            null,
+            mockedState,
+            [],
+            [
+              { type: 'toggleScrollisInBottom', payload: true },
+              { type: 'receiveTraceSuccess', payload: tracePayload },
+              { type: 'startPollingTrace' },
+            ],
+            done,
+          );
+        });
+
+        it('does not dispatch startPollingTrace when timeout is non-empty', done => {
+          mockedState.traceTimeout = 1;
+
+          testAction(
+            fetchTrace,
+            null,
+            mockedState,
+            [],
+            [
+              { type: 'toggleScrollisInBottom', payload: true },
+              { type: 'receiveTraceSuccess', payload: tracePayload },
+            ],
+            done,
+          );
+        });
+      });
     });
 
     describe('error', () => {
@@ -265,16 +310,69 @@ describe('Job State actions', () => {
     });
   });
 
+  describe('startPollingTrace', () => {
+    let dispatch;
+    let commit;
+
+    beforeEach(() => {
+      jasmine.clock().install();
+
+      dispatch = jasmine.createSpy();
+      commit = jasmine.createSpy();
+
+      startPollingTrace({ dispatch, commit });
+    });
+
+    afterEach(() => {
+      jasmine.clock().uninstall();
+    });
+
+    it('should save the timeout id but not call fetchTrace', () => {
+      expect(commit).toHaveBeenCalledWith(types.SET_TRACE_TIMEOUT, 1);
+      expect(dispatch).not.toHaveBeenCalledWith('fetchTrace');
+    });
+
+    describe('after timeout has passed', () => {
+      beforeEach(() => {
+        jasmine.clock().tick(4000);
+      });
+
+      it('should clear the timeout id and fetchTrace', () => {
+        expect(commit).toHaveBeenCalledWith(types.SET_TRACE_TIMEOUT, 0);
+        expect(dispatch).toHaveBeenCalledWith('fetchTrace');
+      });
+    });
+  });
+
   describe('stopPollingTrace', () => {
+    let origTimeout;
+
+    beforeEach(() => {
+      // Can't use spyOn(window, 'clearTimeout') because this caused unrelated specs to timeout
+      // https://gitlab.com/gitlab-org/gitlab/-/merge_requests/23838#note_280277727
+      origTimeout = window.clearTimeout;
+      window.clearTimeout = jasmine.createSpy();
+    });
+
+    afterEach(() => {
+      window.clearTimeout = origTimeout;
+    });
+
     it('should commit STOP_POLLING_TRACE mutation ', done => {
+      const traceTimeout = 7;
+
       testAction(
         stopPollingTrace,
         null,
-        mockedState,
-        [{ type: types.STOP_POLLING_TRACE }],
+        { ...mockedState, traceTimeout },
+        [{ type: types.SET_TRACE_TIMEOUT, payload: 0 }, { type: types.STOP_POLLING_TRACE }],
         [],
-        done,
-      );
+      )
+        .then(() => {
+          expect(window.clearTimeout).toHaveBeenCalledWith(traceTimeout);
+        })
+        .then(done)
+        .catch(done.fail);
     });
   });
 
@@ -292,15 +390,8 @@ describe('Job State actions', () => {
   });
 
   describe('receiveTraceError', () => {
-    it('should commit RECEIVE_TRACE_ERROR mutation ', done => {
-      testAction(
-        receiveTraceError,
-        null,
-        mockedState,
-        [{ type: types.RECEIVE_TRACE_ERROR }],
-        [],
-        done,
-      );
+    it('should commit stop polling trace', done => {
+      testAction(receiveTraceError, null, mockedState, [], [{ type: 'stopPollingTrace' }], done);
     });
   });