Merge branch '228949-remove-build-report-summary-ff' into 'master'

Remove build_report_summary feature flag

See merge request gitlab-org/gitlab!37629

app/assets/javascripts/pipelines/components/test_reports/test_reports.vue

@@ -14,7 +14,7 @@ export default {
     TestSummaryTable,
   },
   computed: {
-    ...mapState(['hasFullReport', 'isLoading', 'selectedSuiteIndex', 'testReports']),
+    ...mapState(['isLoading', 'selectedSuiteIndex', 'testReports']),
     ...mapGetters(['getSelectedSuite']),
     showSuite() {
       return this.selectedSuiteIndex !== null;

app/assets/javascripts/pipelines/pipeline_details_bundle.js

@@ -92,49 +92,19 @@ const createPipelineHeaderApp = mediator => {
   });
 };
 
-const createPipelinesTabs = testReportsStore => {
-  const tabsElement = document.querySelector('.pipelines-tabs');
-
-  if (tabsElement) {
-    const fetchReportsAction = 'fetchFullReport';
-    const isTestTabActive = Boolean(
-      document.querySelector('.pipelines-tabs > li > a.test-tab.active'),
-    );
-
-    if (isTestTabActive) {
-      testReportsStore.dispatch(fetchReportsAction);
-    } else {
-      const tabClickHandler = e => {
-        if (e.target.className === 'test-tab') {
-          testReportsStore.dispatch(fetchReportsAction);
-          tabsElement.removeEventListener('click', tabClickHandler);
-        }
-      };
-
-      tabsElement.addEventListener('click', tabClickHandler);
-    }
-  }
-};
-
 const createTestDetails = () => {
   if (!window.gon?.features?.junitPipelineView) {
     return;
   }
 
   const el = document.querySelector('#js-pipeline-tests-detail');
-  const { fullReportEndpoint, summaryEndpoint, suiteEndpoint, countEndpoint } = el?.dataset || {};
+  const { summaryEndpoint, suiteEndpoint } = el?.dataset || {};
 
   const testReportsStore = createTestReportsStore({
-    fullReportEndpoint,
-    summaryEndpoint: summaryEndpoint || countEndpoint,
+    summaryEndpoint,
     suiteEndpoint,
-    useBuildSummaryReport: window.gon?.features?.buildReportSummary,
   });
 
-  if (!window.gon?.features?.buildReportSummary) {
-    createPipelinesTabs(testReportsStore);
-  }
-
   // eslint-disable-next-line no-new
   new Vue({
     el,

app/assets/javascripts/pipelines/stores/test_reports/actions.js

@@ -4,39 +4,25 @@ import createFlash from '~/flash';
 import { s__ } from '~/locale';
 
 export const fetchSummary = ({ state, commit, dispatch }) => {
-  // If we do this without the build_report_summary feature flag enabled
-  // it causes a race condition for toggleLoading and ruins the loading
-  // state in the application
-  if (state.useBuildSummaryReport) {
   dispatch('toggleLoading');
-  }
 
   return axios
     .get(state.summaryEndpoint)
     .then(({ data }) => {
       commit(types.SET_SUMMARY, data);
-
-      if (!state.useBuildSummaryReport) {
-        // Set the tab counter badge to total_count
-        // This is temporary until we can server-side render that count number
-        // (see https://gitlab.com/gitlab-org/gitlab/-/issues/223134)
-        document.querySelector('.js-test-report-badge-counter').innerHTML = data.total_count || 0;
-      }
     })
     .catch(() => {
       createFlash(s__('TestReports|There was an error fetching the summary.'));
     })
     .finally(() => {
-      if (state.useBuildSummaryReport) {
       dispatch('toggleLoading');
-      }
     });
 };
 
 export const fetchTestSuite = ({ state, commit, dispatch }, index) => {
   const { hasFullSuite } = state.testReports?.test_suites?.[index] || {};
   // We don't need to fetch the suite if we have the information already
-  if (state.hasFullReport || hasFullSuite) {
+  if (hasFullSuite) {
     return Promise.resolve();
   }
 
@@ -61,20 +47,6 @@ export const fetchTestSuite = ({ state, commit, dispatch }, index) => {
     });
 };
 
-export const fetchFullReport = ({ state, commit, dispatch }) => {
-  dispatch('toggleLoading');
-
-  return axios
-    .get(state.fullReportEndpoint)
-    .then(({ data }) => commit(types.SET_REPORTS, data))
-    .catch(() => {
-      createFlash(s__('TestReports|There was an error fetching the test reports.'));
-    })
-    .finally(() => {
-      dispatch('toggleLoading');
-    });
-};
-
 export const setSelectedSuiteIndex = ({ commit }, data) =>
   commit(types.SET_SELECTED_SUITE_INDEX, data);
 export const removeSelectedSuiteIndex = ({ commit }) =>

app/assets/javascripts/pipelines/stores/test_reports/mutation_types.js

-export const SET_REPORTS = 'SET_REPORTS';
 export const SET_SELECTED_SUITE_INDEX = 'SET_SELECTED_SUITE_INDEX';
 export const SET_SUMMARY = 'SET_SUMMARY';
 export const SET_SUITE = 'SET_SUITE';

app/assets/javascripts/pipelines/stores/test_reports/mutations.js

 import * as types from './mutation_types';
 
 export default {
-  [types.SET_REPORTS](state, testReports) {
-    Object.assign(state, { testReports, hasFullReport: true });
-  },
-
   [types.SET_SUITE](state, { suite = {}, index = null }) {
     state.testReports.test_suites[index] = { ...suite, hasFullSuite: true };
   },
@@ -13,8 +9,8 @@ export default {
     Object.assign(state, { selectedSuiteIndex });
   },
 
-  [types.SET_SUMMARY](state, summary) {
-    Object.assign(state, { testReports: { ...state.testReports, ...summary } });
+  [types.SET_SUMMARY](state, testReports) {
+    Object.assign(state, { testReports });
   },
 
   [types.TOGGLE_LOADING](state) {

app/assets/javascripts/pipelines/stores/test_reports/state.js

-export default ({
-  fullReportEndpoint = '',
-  summaryEndpoint = '',
-  suiteEndpoint = '',
-  useBuildSummaryReport = false,
-}) => ({
+export default ({ summaryEndpoint = '', suiteEndpoint = '' }) => ({
   summaryEndpoint,
   suiteEndpoint,
-  fullReportEndpoint,
   testReports: {},
   selectedSuiteIndex: null,
-  hasFullReport: false,
   isLoading: false,
-  useBuildSummaryReport,
 });

app/controllers/projects/pipelines/tests_controller.rb

@@ -3,7 +3,6 @@
 module Projects
   module Pipelines
     class TestsController < Projects::Pipelines::ApplicationController
-      before_action :validate_feature_flag!
       before_action :authorize_read_build!
       before_action :builds, only: [:show]
 
@@ -29,10 +28,6 @@ module Projects
 
       private
 
-      def validate_feature_flag!
-        render_404 unless Feature.enabled?(:build_report_summary, project)
-      end
-
       # rubocop: disable CodeReuse/ActiveRecord
       def builds
         @builds ||= pipeline.latest_builds.for_ids(build_ids).presence || render_404

app/controllers/projects/pipelines_controller.rb

@@ -13,7 +13,6 @@ class Projects::PipelinesController < Projects::ApplicationController
   before_action :authorize_update_pipeline!, only: [:retry, :cancel]
   before_action do
     push_frontend_feature_flag(:junit_pipeline_view, project)
-    push_frontend_feature_flag(:build_report_summary, project)
     push_frontend_feature_flag(:filter_pipelines_search, project, default_enabled: true)
     push_frontend_feature_flag(:dag_pipeline_tab, project, default_enabled: true)
     push_frontend_feature_flag(:pipelines_security_report_summary, project)

app/serializers/pipeline_entity.rb

@@ -85,7 +85,7 @@ class PipelineEntity < Grape::Entity
     pipeline.failed_builds
   end
 
-  expose :tests_total_count, if: -> (pipeline, _) { Feature.enabled?(:build_report_summary, pipeline.project) } do |pipeline|
+  expose :tests_total_count do |pipeline|
     pipeline.test_report_summary.total_count
   end
 

app/services/ci/build_report_result_service.rb

@@ -3,7 +3,6 @@
 module Ci
   class BuildReportResultService
     def execute(build)
-      return unless Feature.enabled?(:build_report_summary, build.project)
       return unless build.has_test_reports?
 
       build.report_results.create!(

app/views/projects/pipelines/_with_tabs.html.haml

@@ -23,7 +23,7 @@
       %li.js-tests-tab-link
         = link_to test_report_project_pipeline_path(@project, @pipeline), data: { target: '#js-tab-tests', action: 'test_report', toggle: 'tab' }, class: 'test-tab' do
           = s_('TestReports|Tests')
-          %span.badge.badge-pill.js-test-report-badge-counter= Feature.enabled?(:build_report_summary, @project) ? @pipeline.test_report_summary.total_count : ''
+          %span.badge.badge-pill.js-test-report-badge-counter= @pipeline.test_report_summary.total_count
     = render_if_exists "projects/pipelines/tabs_holder", pipeline: @pipeline, project: @project
 
 .tab-content
@@ -85,8 +85,6 @@
       #js-pipeline-dag-vue{ data: { pipeline_data_path: dag_project_pipeline_path(@project, @pipeline), empty_svg_path: image_path('illustrations/empty-state/empty-dag-md.svg'), dag_doc_path: help_page_path('ci/yaml/README.md', anchor: 'needs')} }
 
   #js-tab-tests.tab-pane
-    #js-pipeline-tests-detail{ data: { full_report_endpoint: test_report_project_pipeline_path(@project, @pipeline, format: :json),
-      summary_endpoint: Feature.enabled?(:build_report_summary, @project) ? summary_project_pipeline_tests_path(@project, @pipeline, format: :json) : '',
-      suite_endpoint: Feature.enabled?(:build_report_summary, @project) ? project_pipeline_test_path(@project, @pipeline, suite_name: ':suite_name', format: :json) : '',
-      count_endpoint: test_reports_count_project_pipeline_path(@project, @pipeline, format: :json) } }
+    #js-pipeline-tests-detail{ data: { summary_endpoint: summary_project_pipeline_tests_path(@project, @pipeline, format: :json),
+      suite_endpoint: project_pipeline_test_path(@project, @pipeline, suite_name: ':suite_name', format: :json) } }
   = render_if_exists "projects/pipelines/tabs_content", pipeline: @pipeline, project: @project

changelogs/unreleased/228949-remove-build-report-summary-ff.yml

+---
+title: Improve performance of test report with summary and test suite endpoints
+merge_request: 37629
+author:
+type: performance

locale/gitlab.pot

@@ -23881,9 +23881,6 @@ msgstr ""
 msgid "TestReports|There was an error fetching the summary."
 msgstr ""
 
-msgid "TestReports|There was an error fetching the test reports."
-msgstr ""
-
 msgid "TestReports|There was an error fetching the test suite."
 msgstr ""
 

spec/controllers/projects/pipelines/tests_controller_spec.rb

@@ -31,19 +31,6 @@ RSpec.describe Projects::Pipelines::TestsController do
         expect(json_response['total_count']).to eq(0)
       end
     end
-
-    context 'when feature is disabled' do
-      before do
-        stub_feature_flags(build_report_summary: false)
-      end
-
-      it 'returns 404' do
-        get_tests_summary_json
-
-        expect(response).to have_gitlab_http_status(:not_found)
-        expect(response.body).to be_empty
-      end
-    end
   end
 
   describe 'GET #show.json' do
@@ -71,21 +58,6 @@ RSpec.describe Projects::Pipelines::TestsController do
         expect(response.body).to be_empty
       end
     end
-
-    context 'when feature is disabled' do
-      let(:suite_name) { 'test' }
-
-      before do
-        stub_feature_flags(build_report_summary: false)
-      end
-
-      it 'returns 404' do
-        get_tests_show_json([])
-
-        expect(response).to have_gitlab_http_status(:not_found)
-        expect(response.body).to be_empty
-      end
-    end
   end
 
   def get_tests_summary_json

spec/controllers/projects/pipelines_controller_spec.rb

@@ -57,27 +57,6 @@ RSpec.describe Projects::PipelinesController do
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['pipelines'].count).to eq 12
       end
-
-      context 'with build_report_summary turned off' do
-        before do
-          stub_feature_flags(build_report_summary: false)
-        end
-
-        it 'does not execute N+1 queries' do
-          get_pipelines_index_json
-
-          control_count = ActiveRecord::QueryRecorder.new do
-            get_pipelines_index_json
-          end.count
-
-          create_all_pipeline_types
-
-          # There appears to be one extra query for Pipelines#has_warnings? for some reason
-          expect { get_pipelines_index_json }.not_to exceed_query_limit(control_count + 1)
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['pipelines'].count).to eq 12
-        end
-      end
     end
 
     it 'does not include coverage data for the pipelines' do

spec/features/projects/pipelines/pipeline_spec.rb

@@ -363,42 +363,6 @@ RSpec.describe 'Pipeline', :js do
     describe 'test tabs' do
       let(:pipeline) { create(:ci_pipeline, :with_test_reports, :with_report_results, project: project) }
 
-      context 'with build_report_summary feature flag disabled' do
-        before do
-          stub_feature_flags(build_report_summary: false)
-          visit_pipeline
-          wait_for_requests
-        end
-
-        context 'with test reports' do
-          it 'shows badge counter in Tests tab' do
-            expect(pipeline.test_reports.total_count).to eq(4)
-            expect(page.find('.js-test-report-badge-counter').text).to eq(pipeline.test_reports.total_count.to_s)
-          end
-
-          it 'does not call test_report.json endpoint by default', :js do
-            expect(page).to have_selector('.js-no-tests-to-show', visible: :all)
-          end
-
-          it 'does call test_report.json endpoint when tab is selected', :js do
-            find('.js-tests-tab-link').click
-            wait_for_requests
-
-            expect(page).to have_content('Jobs')
-            expect(page).to have_selector('.js-tests-detail', visible: :all)
-          end
-        end
-
-        context 'without test reports' do
-          let(:pipeline) { create(:ci_pipeline, project: project) }
-
-          it 'shows zero' do
-            expect(page.find('.js-test-report-badge-counter', visible: :all).text).to eq("0")
-          end
-        end
-      end
-
-      context 'with build_report_summary feature flag enabled' do
       before do
         visit_pipeline
         wait_for_requests
@@ -425,7 +389,6 @@ RSpec.describe 'Pipeline', :js do
         end
       end
     end
-    end
 
     context 'retrying jobs' do
       before do

spec/frontend/pipelines/test_reports/stores/actions_spec.js

@@ -16,16 +16,13 @@ describe('Actions TestReports Store', () => {
   const testReports = getJSONFixture('pipelines/test_report.json');
   const summary = { total_count: 1 };
 
-  const fullReportEndpoint = `${TEST_HOST}/test_reports.json`;
   const suiteEndpoint = `${TEST_HOST}/tests/:suite_name.json`;
   const summaryEndpoint = `${TEST_HOST}/test_reports/summary.json`;
   const defaultState = {
-    fullReportEndpoint,
     suiteEndpoint,
     summaryEndpoint,
     testReports: {},
     selectedSuite: null,
-    useBuildSummaryReport: false,
   };
 
   beforeEach(() => {
@@ -42,12 +39,11 @@ describe('Actions TestReports Store', () => {
       mock.onGet(summaryEndpoint).replyOnce(200, summary, {});
     });
 
-    describe('when useBuildSummaryReport in state is true', () => {
     it('sets testReports and shows tests', done => {
       testAction(
         actions.fetchSummary,
         null,
-          { ...state, useBuildSummaryReport: true },
+        state,
         [{ type: types.SET_SUMMARY, payload: summary }],
         [{ type: 'toggleLoading' }, { type: 'toggleLoading' }],
         done,
@@ -58,10 +54,7 @@ describe('Actions TestReports Store', () => {
       testAction(
         actions.fetchSummary,
         null,
-          {
-            summaryEndpoint: null,
-            useBuildSummaryReport: true,
-          },
+        { summaryEndpoint: null },
         [],
         [{ type: 'toggleLoading' }, { type: 'toggleLoading' }],
         () => {
@@ -72,36 +65,6 @@ describe('Actions TestReports Store', () => {
     });
   });
 
-    describe('when useBuildSummaryReport in state is false', () => {
-      it('sets testReports and shows tests', done => {
-        testAction(
-          actions.fetchSummary,
-          null,
-          state,
-          [{ type: types.SET_SUMMARY, payload: summary }],
-          [],
-          done,
-        );
-      });
-
-      it('should create flash on API error', done => {
-        testAction(
-          actions.fetchSummary,
-          null,
-          {
-            summaryEndpoint: null,
-          },
-          [],
-          [],
-          () => {
-            expect(createFlash).toHaveBeenCalled();
-            done();
-          },
-        );
-      });
-    });
-  });
-
   describe('fetch test suite', () => {
     beforeEach(() => {
       const buildIds = [1];
@@ -150,77 +113,6 @@ describe('Actions TestReports Store', () => {
         testAction(actions.fetchTestSuite, index, { ...state, testReports }, [], [], done);
       });
     });
-
-    describe('when we already have the full report data', () => {
-      it('should not fetch suite', done => {
-        const index = 0;
-        testReports.hasFullReport = true;
-
-        testAction(actions.fetchTestSuite, index, { ...state, testReports }, [], [], done);
-      });
-    });
-
-    describe('when the suite name has a `/` in it', () => {
-      it('sets test suite, shows tests, and encodes the suite name', done => {
-        const index = 0;
-        const suite = testReports.test_suites[0];
-        const { name } = suite;
-        const slashName = `${name}/8`;
-        testReports.test_suites[0].name = slashName;
-        const buildIds = [1];
-        testReports.test_suites[0].hasFullSuite = false;
-        testReports.test_suites[0].build_ids = buildIds;
-        const endpoint = suiteEndpoint.replace(':suite_name', encodeURIComponent(slashName));
-        mock
-          .onGet(endpoint, { params: { build_ids: buildIds } })
-          .replyOnce(200, testReports.test_suites[0], {});
-
-        testAction(
-          actions.fetchTestSuite,
-          index,
-          { ...state, testReports },
-          [{ type: types.SET_SUITE, payload: { suite, index } }],
-          [{ type: 'toggleLoading' }, { type: 'toggleLoading' }],
-          () => {
-            expect(mock.history.get.map(x => x.url)).toEqual([endpoint]);
-            done();
-          },
-        );
-      });
-    });
-  });
-
-  describe('fetch full report', () => {
-    beforeEach(() => {
-      mock.onGet(fullReportEndpoint).replyOnce(200, testReports, {});
-    });
-
-    it('sets testReports and shows tests', done => {
-      testAction(
-        actions.fetchFullReport,
-        null,
-        state,
-        [{ type: types.SET_REPORTS, payload: testReports }],
-        [{ type: 'toggleLoading' }, { type: 'toggleLoading' }],
-        done,
-      );
-    });
-
-    it('should create flash on API error', done => {
-      testAction(
-        actions.fetchFullReport,
-        null,
-        {
-          fullReportEndpoint: null,
-        },
-        [],
-        [{ type: 'toggleLoading' }, { type: 'toggleLoading' }],
-        () => {
-          expect(createFlash).toHaveBeenCalled();
-          done();
-        },
-      );
-    });
   });
 
   describe('set selected suite index', () => {

spec/frontend/pipelines/test_reports/stores/mutations_spec.js

@@ -12,23 +12,12 @@ describe('Mutations TestReports Store', () => {
     testReports: {},
     selectedSuite: null,
     isLoading: false,
-    hasFullReport: false,
   };
 
   beforeEach(() => {
     mockState = { ...defaultState };
   });
 
-  describe('set reports', () => {
-    it('should set testReports', () => {
-      const expectedState = { ...mockState, testReports };
-      mutations[types.SET_REPORTS](mockState, testReports);
-
-      expect(mockState.testReports).toEqual(expectedState.testReports);
-      expect(mockState.hasFullReport).toBe(true);
-    });
-  });
-
   describe('set suite', () => {
     it('should set the suite at the given index', () => {
       mockState.testReports = testReports;

spec/serializers/pipeline_entity_spec.rb

@@ -264,25 +264,9 @@ RSpec.describe PipelineEntity do
     context 'when pipeline has build report results' do
       let(:pipeline) { create(:ci_pipeline, :with_report_results, project: project, user: user) }
 
-      context 'when feature is enabled' do
-        before do
-          stub_feature_flags(build_report_summary: true)
-        end
-
       it 'exposes tests total count' do
         expect(subject[:tests_total_count]).to eq(2)
       end
     end
-
-      context 'when feature is disabled' do
-        before do
-          stub_feature_flags(build_report_summary: false)
-        end
-
-        it 'do not expose tests total count' do
-          expect(subject).not_to include(:tests_total_count)
-        end
-      end
-    end
   end
 end

spec/serializers/pipeline_serializer_spec.rb

@@ -160,20 +160,6 @@ RSpec.describe PipelineSerializer do
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)
         end
-
-        context 'with the :build_report_summary flag turned off' do
-          before do
-            stub_feature_flags(build_report_summary: false)
-          end
-
-          it 'verifies number of queries', :request_store do
-            recorded = ActiveRecord::QueryRecorder.new { subject }
-            expected_queries = Gitlab.ee? ? 43 : 40
-
-            expect(recorded.count).to be_within(2).of(expected_queries)
-            expect(recorded.cached_count).to eq(0)
-          end
-        end
       end
 
       context 'with different refs' do
@@ -195,20 +181,6 @@ RSpec.describe PipelineSerializer do
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)
         end
-
-        context 'with the :build_report_summary flag turned off' do
-          before do
-            stub_feature_flags(build_report_summary: false)
-          end
-
-          it 'verifies number of queries', :request_store do
-            recorded = ActiveRecord::QueryRecorder.new { subject }
-            expected_queries = Gitlab.ee? ? 46 : 43
-
-            expect(recorded.count).to be_within(2).of(expected_queries)
-            expect(recorded.cached_count).to eq(0)
-          end
-        end
       end
 
       context 'with triggered pipelines' do

spec/services/ci/build_report_result_service_spec.rb

@@ -19,16 +19,6 @@ RSpec.describe Ci::BuildReportResultService do
         expect(Ci::BuildReportResult.count).to eq(1)
       end
 
-      context 'when feature is disable' do
-        it 'does not persist the data' do
-          stub_feature_flags(build_report_summary: false)
-
-          subject
-
-          expect(Ci::BuildReportResult.count).to eq(0)
-        end
-      end
-
       context 'when data has already been persisted' do
         it 'raises an error and do not persist the same data twice' do
           expect { 2.times { described_class.new.execute(build) } }.to raise_error(ActiveRecord::RecordNotUnique)