Commit 02bd4f66 authored by Jacob Schatz's avatar Jacob Schatz

Merge branch '3046-integrated-browser-performance-testing' into 'master'

Integrated Browser Performance Testing

Closes #3046

See merge request gitlab-org/gitlab-ee!3507
parents 977de0fa 1cd4f895
......@@ -811,6 +811,10 @@
.failed {
color: $red-500;
}
.neutral {
color: $gl-gray-light;
}
}
}
}
......
---
title: Add performance metrics to the merge request widget
merge_request: 3507
author:
type: added
......@@ -7,7 +7,7 @@ export default {
name: 'MRWidgetCodeQuality',
props: {
// security | codequality
// security | codequality | performance
type: {
type: String,
required: true,
......@@ -39,6 +39,11 @@ export default {
required: false,
default: () => [],
},
neutralIssues: {
type: Array,
required: false,
default: () => [],
},
},
components: {
......@@ -131,6 +136,14 @@ export default {
:issues="resolvedIssues"
/>
<issues-block
class="js-mr-code-non-issues"
v-if="neutralIssues.length"
:type="type"
status="neutral"
:issues="neutralIssues"
/>
<issues-block
class="js-mr-code-new-issues"
v-if="unresolvedIssues.length"
......
......@@ -8,7 +8,7 @@
type: Array,
required: true,
},
// security || codequality
// security || codequality || performance
type: {
type: String,
required: true,
......@@ -21,7 +21,7 @@
},
computed: {
icon() {
return this.isStatusFailed ? spriteIcon('cut') : spriteIcon('plus');
return this.isStatusSuccess ? spriteIcon('plus') : spriteIcon('cut');
},
isStatusFailed() {
return this.status === 'failed';
......@@ -29,9 +29,15 @@
isStatusSuccess() {
return this.status === 'success';
},
isStatusNeutral() {
return this.status === 'neutral';
},
isTypeQuality() {
return this.type === 'codequality';
},
isTypePerformance() {
return this.type === 'performance';
},
isTypeSecurity() {
return this.type === 'security';
},
......@@ -43,7 +49,8 @@
<li
:class="{
failed: isStatusFailed,
success: isStatusSuccess
success: isStatusSuccess,
neutral: isStatusNeutral
}
"v-for="issue in issues">
......@@ -55,17 +62,25 @@
<template v-if="isStatusSuccess && isTypeQuality">Fixed:</template>
<template v-if="isTypeSecurity && issue.priority">{{issue.priority}}:</template>
{{issue.name}}
{{issue.name}}<template v-if="issue.score">: <strong>{{issue.score}}</strong></template>
<template v-if="isTypePerformance && issue.delta != null">
({{issue.delta >= 0 ? '+' : ''}}{{issue.delta}})
</template>
<template v-if="issue.path">
in
<a
v-if="issue.urlPath"
:href="issue.urlPath"
target="_blank"
rel="noopener noreferrer nofollow">
{{issue.path}}<template v-if="issue.line">:{{issue.line}}</template>
</a>
<template v-else>
{{issue.path}}<template v-if="issue.line">:{{issue.line}}</template>
</template>
</template>
</li>
......
......@@ -16,8 +16,10 @@ export default {
data() {
return {
isLoadingCodequality: false,
isLoadingPerformance: false,
isLoadingSecurity: false,
loadingCodequalityFailed: false,
loadingPerformanceFailed: false,
loadingSecurityFailed: false,
};
},
......@@ -29,6 +31,10 @@ export default {
const { codeclimate } = this.mr;
return codeclimate && codeclimate.head_path && codeclimate.base_path;
},
shouldRenderPerformance() {
const { performance } = this.mr;
return performance && performance.head_path && performance.base_path;
},
shouldRenderSecurityReport() {
return this.mr.sast;
},
......@@ -65,6 +71,39 @@ export default {
return text.join('');
},
performanceText() {
const { improved, degraded } = this.mr.performanceMetrics;
const text = [];
if (!improved.length && !degraded.length) {
text.push('No changes to performance metrics');
} else if (improved.length || degraded.length) {
text.push('Performance metrics');
if (improved.length) {
text.push(n__(
' improved on %d point',
' improved on %d points',
improved.length,
));
}
if (improved.length > 0 && degraded.length > 0) {
text.push(' and');
}
if (degraded.length) {
text.push(n__(
' degraded on %d point',
' degraded on %d points',
degraded.length,
));
}
}
return text.join('');
},
securityText() {
if (this.mr.securityReport.length) {
return n__(
......@@ -86,6 +125,15 @@ export default {
return 'success';
},
performanceStatus() {
if (this.isLoadingPerformance) {
return 'loading';
} else if (this.loadingPerformanceFailed) {
return 'error';
}
return 'success';
},
securityStatus() {
if (this.isLoadingSecurity) {
return 'loading';
......@@ -115,6 +163,25 @@ export default {
});
},
fetchPerformance() {
const { head_path, base_path } = this.mr.performance;
this.isLoadingPerformance = true;
Promise.all([
this.service.fetchReport(head_path),
this.service.fetchReport(base_path),
])
.then((values) => {
this.mr.comparePerformanceMetrics(values[0], values[1]);
this.isLoadingPerformance = false;
})
.catch(() => {
this.isLoadingPerformance = false;
this.loadingPerformanceFailed = true;
});
},
fetchSecurity() {
const { path, blob_path } = this.mr.sast;
this.isLoadingSecurity = true;
......@@ -135,6 +202,10 @@ export default {
this.fetchCodeQuality();
}
if (this.shouldRenderPerformance) {
this.fetchPerformance();
}
if (this.shouldRenderSecurityReport) {
this.fetchSecurity();
}
......@@ -167,6 +238,18 @@ export default {
:unresolvedIssues="mr.codeclimateMetrics.newIssues"
:resolvedIssues="mr.codeclimateMetrics.resolvedIssues"
/>
<collapsible-section
class="js-performance-widget"
v-if="shouldRenderPerformance"
type="performance"
:status="performanceStatus"
loading-text="Loading performance report"
error-text="Failed to load performance report"
:success-text="performanceText"
:unresolvedIssues="mr.performanceMetrics.degraded"
:resolvedIssues="mr.performanceMetrics.improved"
:neutralIssues="mr.performanceMetrics.neutral"
/>
<collapsible-section
class="js-sast-widget"
v-if="shouldRenderSecurityReport"
......
......@@ -4,6 +4,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
constructor(data) {
super(data);
this.initCodeclimate(data);
this.initPerformanceReport(data);
this.initSecurityReport(data);
}
......@@ -57,6 +58,14 @@ export default class MergeRequestStore extends CEMergeRequestStore {
};
}
initPerformanceReport(data) {
this.performance = data.performance;
this.performanceMetrics = {
improved: [],
degraded: [],
};
}
initSecurityReport(data) {
this.sast = data.sast;
this.securityReport = [];
......@@ -79,6 +88,49 @@ export default class MergeRequestStore extends CEMergeRequestStore {
parsedHeadIssues,
);
}
comparePerformanceMetrics(headMetrics, baseMetrics) {
const headMetricsIndexed = MergeRequestStore.normalizePerformanceMetrics(headMetrics);
const baseMetricsIndexed = MergeRequestStore.normalizePerformanceMetrics(baseMetrics);
const improved = [];
const degraded = [];
const neutral = [];
Object.keys(headMetricsIndexed).forEach((subject) => {
const subjectMetrics = headMetricsIndexed[subject];
Object.keys(subjectMetrics).forEach((metric) => {
const headMetricData = subjectMetrics[metric];
if (baseMetricsIndexed[subject] && baseMetricsIndexed[subject][metric]) {
const baseMetricData = baseMetricsIndexed[subject][metric];
const metricData = {
name: metric,
path: subject,
score: headMetricData.value,
delta: headMetricData.value - baseMetricData.value,
};
if (headMetricData.value > baseMetricData.value) {
improved.push(metricData);
} else if (headMetricData.value < baseMetricData.value) {
degraded.push(metricData);
} else {
neutral.push(metricData);
}
} else {
neutral.push({
name: metric,
path: subject,
score: headMetricData.value,
});
}
});
});
this.performanceMetrics = { improved, degraded, neutral };
}
/**
* In order to reuse the same component we need
* to set both codequality and security issues to have the same data structure:
......@@ -136,4 +188,18 @@ export default class MergeRequestStore extends CEMergeRequestStore {
static filterByFingerprint(firstArray, secondArray) {
return firstArray.filter(item => !secondArray.find(el => el.fingerprint === item.fingerprint));
}
// normalize performance metrics by indexing on performance subject and metric name
static normalizePerformanceMetrics(performanceData) {
const indexedSubjects = {};
performanceData.forEach(({ subject, metrics }) => {
const indexedMetrics = {};
metrics.forEach(({ name, ...data }) => {
indexedMetrics[name] = data;
});
indexedSubjects[subject] = indexedMetrics;
});
return indexedSubjects;
}
}
......@@ -9,6 +9,7 @@ module EE
included do
scope :codequality, ->() { where(name: %w[codequality codeclimate]) }
scope :performance, ->() { where(name: %w[performance deploy]) }
scope :sast, ->() { where(name: 'sast') }
after_save :stick_build_if_status_changed
......@@ -30,6 +31,11 @@ module EE
artifacts_metadata?
end
def has_performance_json?
options.dig(:artifacts, :paths) == ['performance.json'] &&
artifacts_metadata?
end
def has_sast_json?
options.dig(:artifacts, :paths) == ['gl-sast-report.json'] &&
artifacts_metadata?
......
......@@ -17,6 +17,10 @@ module EE
artifacts.codequality.find(&:has_codeclimate_json?)
end
def performance_artifact
artifacts.performance.find(&:has_performance_json?)
end
def sast_artifact
artifacts.sast.find(&:has_sast_json?)
end
......
......@@ -11,6 +11,8 @@ module EE
delegate :codeclimate_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
delegate :codeclimate_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
delegate :performance_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
delegate :performance_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
delegate :sast_artifact, to: :head_pipeline, allow_nil: true
delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
......@@ -44,6 +46,11 @@ module EE
base_codeclimate_artifact&.success?)
end
def has_performance_data?
!!(head_performance_artifact&.success? &&
base_performance_artifact&.success?)
end
def has_sast_data?
sast_artifact&.success?
end
......
......@@ -45,6 +45,7 @@ class License < ActiveRecord::Base
jira_dev_panel_integration
ldap_group_sync_filter
multiple_clusters
merge_request_performance_metrics
object_storage
service_desk
variable_environment_scope
......
......@@ -25,6 +25,20 @@ module EE
end
end
expose :performance, if: -> (mr, _) { expose_performance_data?(mr) } do
expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_performance_artifact) } do |merge_request|
raw_project_build_artifacts_url(merge_request.source_project,
merge_request.head_performance_artifact,
path: 'performance.json')
end
expose :base_path, if: -> (mr, _) { can?(current_user, :read_build, mr.base_performance_artifact) } do |merge_request|
raw_project_build_artifacts_url(merge_request.target_project,
merge_request.base_performance_artifact,
path: 'performance.json')
end
end
expose :sast, if: -> (mr, _) { expose_sast_data?(mr, current_user) } do
expose :path do |merge_request|
raw_project_build_artifacts_url(merge_request.source_project,
......@@ -45,5 +59,10 @@ module EE
mr.has_sast_data? &&
can?(current_user, :read_build, mr.sast_artifact)
end
def expose_performance_data?(mr)
mr.project.feature_available?(:merge_request_performance_metrics) &&
mr.has_performance_data?
end
end
end
......@@ -162,6 +162,40 @@ describe Ci::Build do
end
end
describe '#has_performance_json?' do
context 'valid build' do
let!(:build) do
create(
:ci_build,
:artifacts,
name: 'performance',
pipeline: pipeline,
options: {
artifacts: {
paths: ['performance.json']
}
}
)
end
it { expect(build.has_performance_json?).to be_truthy }
end
context 'invalid build' do
let!(:build) do
create(
:ci_build,
:artifacts,
name: 'performance',
pipeline: pipeline,
options: {}
)
end
it { expect(build.has_performance_json?).to be_falsey }
end
end
describe '#has_sast_json?' do
context 'valid build' do
let!(:build) do
......
......@@ -43,6 +43,34 @@ describe Ci::Pipeline do
end
end
describe '#performance_artifact' do
context 'has performance job' do
let!(:build) do
create(
:ci_build,
:artifacts,
name: 'performance',
pipeline: pipeline,
options: {
artifacts: {
paths: ['performance.json']
}
}
)
end
it { expect(pipeline.performance_artifact).to eq(build) }
end
context 'no performance job' do
before do
create(:ci_build, pipeline: pipeline)
end
it { expect(pipeline.performance_artifact).to be_nil }
end
end
describe '#sast_artifact' do
context 'has sast job' do
let!(:build) do
......
......@@ -167,6 +167,72 @@ describe MergeRequest do
end
end
describe '#base_pipeline' do
let!(:pipeline) { create(:ci_empty_pipeline, project: subject.project, sha: subject.diff_base_sha) }
it { expect(subject.base_pipeline).to eq(pipeline) }
end
describe '#base_codeclimate_artifact' do
before do
allow(subject.base_pipeline).to receive(:codeclimate_artifact)
.and_return(1)
end
it 'delegates to merge request diff' do
expect(subject.base_codeclimate_artifact).to eq(1)
end
end
describe '#head_codeclimate_artifact' do
before do
allow(subject.head_pipeline).to receive(:codeclimate_artifact)
.and_return(1)
end
it 'delegates to merge request diff' do
expect(subject.head_codeclimate_artifact).to eq(1)
end
end
describe '#base_performance_artifact' do
before do
allow(subject.base_pipeline).to receive(:performance_artifact)
.and_return(1)
end
it 'delegates to merge request diff' do
expect(subject.base_performance_artifact).to eq(1)
end
end
describe '#head_performance_artifact' do
before do
allow(subject.head_pipeline).to receive(:performance_artifact)
.and_return(1)
end
it 'delegates to merge request diff' do
expect(subject.head_performance_artifact).to eq(1)
end
end
describe '#has_codeclimate_data?' do
context 'with codeclimate artifact' do
before do
artifact = double(success?: true)
allow(subject.head_pipeline).to receive(:codeclimate_artifact).and_return(artifact)
allow(subject.base_pipeline).to receive(:codeclimate_artifact).and_return(artifact)
end
it { expect(subject.has_codeclimate_data?).to be_truthy }
end
context 'without codeclimate artifact' do
it { expect(subject.has_codeclimate_data?).to be_falsey }
end
end
describe '#sast_artifact' do
it { is_expected.to delegate_method(:sast_artifact).to(:head_pipeline) }
end
......
......@@ -4,13 +4,21 @@ describe MergeRequestEntity do
let(:user) { create(:user) }
let(:project) { create :project, :repository }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let(:build) { create(:ci_build, name: 'sast') }
let(:build) { create(:ci_build, name: 'job') }
let(:request) { double('request', current_user: user) }
subject do
described_class.new(merge_request, request: request)
end
it 'has performance data' do
allow(subject).to receive(:expose_performance_data?).and_return(true)
allow(merge_request).to receive(:base_performance_artifact).and_return(build)
allow(merge_request).to receive(:head_performance_artifact).and_return(build)
expect(subject.as_json).to include(:performance)
end
it 'has sast data' do
allow(subject).to receive(:expose_sast_data?).and_return(true)
allow(merge_request).to receive(:sast_artifact).and_return(build)
......
......@@ -2,7 +2,13 @@ import Vue from 'vue';
import mrWidgetOptions from 'ee/vue_merge_request_widget/mr_widget_options';
import MRWidgetService from 'ee/vue_merge_request_widget/services/mr_widget_service';
import MRWidgetStore from 'ee/vue_merge_request_widget/stores/mr_widget_store';
import mockData, { baseIssues, headIssues, securityIssues } from './mock_data';
import mockData, {
baseIssues,
headIssues,
basePerformance,
headPerformance,
securityIssues,
} from './mock_data';
import mountComponent from '../helpers/vue_mount_component_helper';
describe('ee merge request widget options', () => {
......@@ -278,6 +284,158 @@ describe('ee merge request widget options', () => {
});
});
describe('performance', () => {
beforeEach(() => {
gl.mrWidgetData = {
...mockData,
performance: {
head_path: 'head.json',
base_path: 'base.json',
},
};
Component.mr = new MRWidgetStore(gl.mrWidgetData);
Component.service = new MRWidgetService({});
});
describe('when it is loading', () => {
it('should render loading indicator', () => {
vm = mountComponent(Component);
expect(
vm.$el.querySelector('.js-performance-widget').textContent.trim(),
).toContain('Loading performance report');
});
});
describe('with successful request', () => {
const interceptor = (request, next) => {
if (request.url === 'head.json') {
next(request.respondWith(JSON.stringify(headPerformance), {
status: 200,
}));
}
if (request.url === 'base.json') {
next(request.respondWith(JSON.stringify(basePerformance), {
status: 200,
}));
}
};
beforeEach(() => {
Vue.http.interceptors.push(interceptor);
vm = mountComponent(Component);
});
afterEach(() => {
Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor);
});
it('should render provided data', (done) => {
setTimeout(() => {
expect(
vm.$el.querySelector('.js-performance-widget .js-code-text').textContent.trim(),
).toEqual('Performance metrics improved on 1 point and degraded on 1 point');
done();
}, 0);
});
describe('text connector', () => {
it('should only render information about fixed issues', (done) => {
setTimeout(() => {
vm.mr.performanceMetrics.degraded = [];
Vue.nextTick(() => {
expect(
vm.$el.querySelector('.js-performance-widget .js-code-text').textContent.trim(),
).toEqual('Performance metrics improved on 1 point');
done();
});
}, 0);
});
it('should only render information about added issues', (done) => {
setTimeout(() => {
vm.mr.performanceMetrics.improved = [];
Vue.nextTick(() => {
expect(
vm.$el.querySelector('.js-performance-widget .js-code-text').textContent.trim(),
).toEqual('Performance metrics degraded on 1 point');
done();
});
}, 0);
});
});
});
describe('with empty successful request', () => {
const emptyInterceptor = (request, next) => {
if (request.url === 'head.json') {
next(request.respondWith(JSON.stringify([]), {
status: 200,
}));
}
if (request.url === 'base.json') {
next(request.respondWith(JSON.stringify([]), {
status: 200,
}));
}
};
beforeEach(() => {
Vue.http.interceptors.push(emptyInterceptor);
vm = mountComponent(Component);
});
afterEach(() => {
Vue.http.interceptors = _.without(Vue.http.interceptors, emptyInterceptor);
});
it('should render provided data', (done) => {
setTimeout(() => {
expect(
vm.$el.querySelector('.js-performance-widget .js-code-text').textContent.trim(),
).toEqual('No changes to performance metrics');
done();
}, 0);
});
});
describe('with failed request', () => {
const errorInterceptor = (request, next) => {
if (request.url === 'head.json') {
next(request.respondWith(JSON.stringify([]), {
status: 500,
}));
}
if (request.url === 'base.json') {
next(request.respondWith(JSON.stringify([]), {
status: 500,
}));
}
};
beforeEach(() => {
Vue.http.interceptors.push(errorInterceptor);
vm = mountComponent(Component);
});
afterEach(() => {
Vue.http.interceptors = _.without(Vue.http.interceptors, errorInterceptor);
});
it('should render error indicator', (done) => {
setTimeout(() => {
expect(vm.$el.querySelector('.js-performance-widget').textContent.trim()).toContain('Failed to load performance report');
done();
}, 0);
});
});
});
describe('computed', () => {
describe('shouldRenderApprovals', () => {
it('should return false when no approvals', () => {
......
......@@ -308,6 +308,57 @@ export const parsedBaseIssues = [
},
];
export const headPerformance = [
{
subject: '/some/path',
metrics: [
{
name: 'Sitespeed Score',
value: 85,
},
],
},
{
subject: '/some/other/path',
metrics: [
{
name: 'Sitespeed Score',
value: 79,
},
],
},
{
subject: '/yet/another/path',
metrics: [
{
name: 'Sitespeed Score',
value: 80,
},
],
},
];
export const basePerformance = [
{
subject: '/some/path',
metrics: [
{
name: 'Sitespeed Score',
value: 84,
},
],
},
{
subject: '/some/other/path',
metrics: [
{
name: 'Sitespeed Score',
value: 80,
},
],
},
];
export const codequalityParsedIssues = [
{
name: 'Insecure Dependency',
......
......@@ -2250,50 +2250,6 @@ describe MergeRequest do
end
end
describe '#base_pipeline' do
let!(:pipeline) { create(:ci_empty_pipeline, project: subject.project, sha: subject.diff_base_sha) }
it { expect(subject.base_pipeline).to eq(pipeline) }
end
describe '#base_codeclimate_artifact' do
before do
allow(subject.base_pipeline).to receive(:codeclimate_artifact)
.and_return(1)
end
it 'delegates to merge request diff' do
expect(subject.base_codeclimate_artifact).to eq(1)
end
end
describe '#head_codeclimate_artifact' do
before do
allow(subject.head_pipeline).to receive(:codeclimate_artifact)
.and_return(1)
end
it 'delegates to merge request diff' do
expect(subject.head_codeclimate_artifact).to eq(1)
end
end
describe '#has_codeclimate_data?' do
context 'with codeclimate artifact' do
before do
artifact = double(success?: true)
allow(subject.head_pipeline).to receive(:codeclimate_artifact).and_return(artifact)
allow(subject.base_pipeline).to receive(:codeclimate_artifact).and_return(artifact)
end
it { expect(subject.has_codeclimate_data?).to be_truthy }
end
context 'without codeclimate artifact' do
it { expect(subject.has_codeclimate_data?).to be_falsey }
end
end
describe '#fetch_ref!' do
it 'fetches the ref correctly' do
expect { subject.target_project.repository.delete_refs(subject.ref_path) }.not_to raise_error
......
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