Skip to content

G
gitlab-ce
Commit b922ca7d authored by Scott Hampton's avatar Scott Hampton
Browse files
Options

Remove build_report_summary feature flag 

Now that we have tested the feature in prod
we can remove this build_report_summary
feature flag. Most of what was hidden is still
behind another feature flag anyway.
parent aa6f7e85
Show whitespace changes
Inline Side-by-side
Showing with 56 additions and 393 deletions
app/assets/javascripts/pipelines/components/test_reports/test_reports.vue
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
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
View file @ b922ca7d
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
View file @ b922ca7d
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
View file @ b922ca7d
......@@ -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
pipeline.latest_builds.where(id: build_params)
......
app/controllers/projects/pipelines_controller.rb
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -24,7 +24,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
......@@ -86,8 +86,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 0 → 100644
View file @ b922ca7d
---
title: Improve performance of test report with summary and test suite endpoints
merge_request: 37629
author:
type: performance
locale/gitlab.pot
View file @ b922ca7d
......@@ -23742,9 +23742,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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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
View file @ b922ca7d
......@@ -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)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7