Commit 8e4cdb89 authored by GitLab Bot's avatar GitLab Bot

Automatic merge of gitlab-org/gitlab master

parents ec50ff55 0ef98e28
...@@ -377,7 +377,7 @@ db:rollback: ...@@ -377,7 +377,7 @@ db:rollback:
- bundle exec rake db:migrate SKIP_SCHEMA_VERSION_CHECK=true - bundle exec rake db:migrate SKIP_SCHEMA_VERSION_CHECK=true
db:gitlabcom-database-testing: db:gitlabcom-database-testing:
extends: .rails:rules:ee-and-foss-mr-with-migration extends: .rails:rules:db:gitlabcom-database-testing
stage: test stage: test
image: ruby:2.7-alpine image: ruby:2.7-alpine
needs: [] needs: []
......
...@@ -523,6 +523,13 @@ ...@@ -523,6 +523,13 @@
changes: *db-patterns changes: *db-patterns
- <<: *if-merge-request-title-run-all-rspec - <<: *if-merge-request-title-run-all-rspec
.rails:rules:db:gitlabcom-database-testing:
rules:
- if: '$GITLABCOM_DATABASE_TESTING_TRIGGER_TOKEN == null'
when: never
- <<: *if-merge-request
changes: *db-patterns
.rails:rules:ee-and-foss-unit: .rails:rules:ee-and-foss-unit:
rules: rules:
- changes: *backend-patterns - changes: *backend-patterns
......
c38393255d22bf3533ca8b8714f614411f10fc30 86d933c0a993eabbff4c725da3cff067f371e641
...@@ -122,10 +122,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -122,10 +122,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
end end
end end
if render_merge_ref_head_diff? return @merge_request.merge_head_diff if render_merge_ref_head_diff?
return CompareService.new(@project, @merge_request.merge_ref_head.sha)
.execute(@project, @merge_request.target_branch)
end
if @start_sha if @start_sha
@merge_request_diff.compare_with(@start_sha) @merge_request_diff.compare_with(@start_sha)
......
...@@ -50,12 +50,15 @@ class MergeRequest < ApplicationRecord ...@@ -50,12 +50,15 @@ class MergeRequest < ApplicationRecord
end end
end end
has_many :merge_request_diffs has_many :merge_request_diffs,
-> { regular }, inverse_of: :merge_request
has_many :merge_request_context_commits, inverse_of: :merge_request has_many :merge_request_context_commits, inverse_of: :merge_request
has_many :merge_request_context_commit_diff_files, through: :merge_request_context_commits, source: :diff_files has_many :merge_request_context_commit_diff_files, through: :merge_request_context_commits, source: :diff_files
has_one :merge_request_diff, has_one :merge_request_diff,
-> { order('merge_request_diffs.id DESC') }, inverse_of: :merge_request -> { regular.order('merge_request_diffs.id DESC') }, inverse_of: :merge_request
has_one :merge_head_diff,
-> { merge_head }, inverse_of: :merge_request, class_name: 'MergeRequestDiff'
has_one :cleanup_schedule, inverse_of: :merge_request has_one :cleanup_schedule, inverse_of: :merge_request
belongs_to :latest_merge_request_diff, class_name: 'MergeRequestDiff' belongs_to :latest_merge_request_diff, class_name: 'MergeRequestDiff'
...@@ -476,13 +479,17 @@ class MergeRequest < ApplicationRecord ...@@ -476,13 +479,17 @@ class MergeRequest < ApplicationRecord
# This is used after project import, to reset the IDs to the correct # This is used after project import, to reset the IDs to the correct
# values. It is not intended to be called without having already scoped the # values. It is not intended to be called without having already scoped the
# relation. # relation.
#
# Only set `regular` merge request diffs as latest so `merge_head` diff
# won't be considered as `MergeRequest#merge_request_diff`.
def self.set_latest_merge_request_diff_ids! def self.set_latest_merge_request_diff_ids!
update = ' update = "
latest_merge_request_diff_id = ( latest_merge_request_diff_id = (
SELECT MAX(id) SELECT MAX(id)
FROM merge_request_diffs FROM merge_request_diffs
WHERE merge_requests.id = merge_request_diffs.merge_request_id WHERE merge_requests.id = merge_request_diffs.merge_request_id
)'.squish AND merge_request_diffs.diff_type = #{MergeRequestDiff.diff_types[:regular]}
)".squish
self.each_batch do |batch| self.each_batch do |batch|
batch.update_all(update) batch.update_all(update)
...@@ -921,7 +928,7 @@ class MergeRequest < ApplicationRecord ...@@ -921,7 +928,7 @@ class MergeRequest < ApplicationRecord
def create_merge_request_diff def create_merge_request_diff
fetch_ref! fetch_ref!
# n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37435 # n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377
Gitlab::GitalyClient.allow_n_plus_1_calls do Gitlab::GitalyClient.allow_n_plus_1_calls do
merge_request_diffs.create! merge_request_diffs.create!
reload_merge_request_diff reload_merge_request_diff
...@@ -995,7 +1002,7 @@ class MergeRequest < ApplicationRecord ...@@ -995,7 +1002,7 @@ class MergeRequest < ApplicationRecord
# rubocop: enable CodeReuse/ServiceClass # rubocop: enable CodeReuse/ServiceClass
def diffable_merge_ref? def diffable_merge_ref?
open? && merge_ref_head.present? && (Feature.enabled?(:display_merge_conflicts_in_diff, project) || can_be_merged?) open? && merge_head_diff.present? && (Feature.enabled?(:display_merge_conflicts_in_diff, project) || can_be_merged?)
end end
# Returns boolean indicating the merge_status should be rechecked in order to # Returns boolean indicating the merge_status should be rechecked in order to
......
...@@ -32,6 +32,7 @@ class MergeRequestDiff < ApplicationRecord ...@@ -32,6 +32,7 @@ class MergeRequestDiff < ApplicationRecord
has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) } has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) }
validates :base_commit_sha, :head_commit_sha, :start_commit_sha, sha: true validates :base_commit_sha, :head_commit_sha, :start_commit_sha, sha: true
validates :merge_request_id, uniqueness: { scope: :diff_type }, if: :merge_head?
state_machine :state, initial: :empty do state_machine :state, initial: :empty do
event :clean do event :clean do
...@@ -50,6 +51,11 @@ class MergeRequestDiff < ApplicationRecord ...@@ -50,6 +51,11 @@ class MergeRequestDiff < ApplicationRecord
state :overflow_diff_lines_limit state :overflow_diff_lines_limit
end end
enum diff_type: {
regular: 1,
merge_head: 2
}
scope :with_files, -> { without_states(:without_files, :empty) } scope :with_files, -> { without_states(:without_files, :empty) }
scope :viewable, -> { without_state(:empty) } scope :viewable, -> { without_state(:empty) }
scope :by_commit_sha, ->(sha) do scope :by_commit_sha, ->(sha) do
...@@ -72,6 +78,7 @@ class MergeRequestDiff < ApplicationRecord ...@@ -72,6 +78,7 @@ class MergeRequestDiff < ApplicationRecord
join_condition = merge_requests[:id].eq(mr_diffs[:merge_request_id]) join_condition = merge_requests[:id].eq(mr_diffs[:merge_request_id])
.and(mr_diffs[:id].not_eq(merge_requests[:latest_merge_request_diff_id])) .and(mr_diffs[:id].not_eq(merge_requests[:latest_merge_request_diff_id]))
.and(mr_diffs[:diff_type].eq(diff_types[:regular]))
arel_join = mr_diffs.join(merge_requests).on(join_condition) arel_join = mr_diffs.join(merge_requests).on(join_condition)
joins(arel_join.join_sources) joins(arel_join.join_sources)
...@@ -196,6 +203,10 @@ class MergeRequestDiff < ApplicationRecord ...@@ -196,6 +203,10 @@ class MergeRequestDiff < ApplicationRecord
end end
def set_as_latest_diff def set_as_latest_diff
# Don't set merge_head diff as latest so it won't get considered as the
# MergeRequest#merge_request_diff.
return if merge_head?
MergeRequest MergeRequest
.where('id = ? AND COALESCE(latest_merge_request_diff_id, 0) < ?', self.merge_request_id, self.id) .where('id = ? AND COALESCE(latest_merge_request_diff_id, 0) < ?', self.merge_request_id, self.id)
.update_all(latest_merge_request_diff_id: self.id) .update_all(latest_merge_request_diff_id: self.id)
...@@ -203,8 +214,16 @@ class MergeRequestDiff < ApplicationRecord ...@@ -203,8 +214,16 @@ class MergeRequestDiff < ApplicationRecord
def ensure_commit_shas def ensure_commit_shas
self.start_commit_sha ||= merge_request.target_branch_sha self.start_commit_sha ||= merge_request.target_branch_sha
self.head_commit_sha ||= merge_request.source_branch_sha
self.base_commit_sha ||= find_base_sha if merge_head? && merge_request.merge_ref_head.present?
diff_refs = merge_request.merge_ref_head.diff_refs
self.head_commit_sha ||= diff_refs.head_sha
self.base_commit_sha ||= diff_refs.base_sha
else
self.head_commit_sha ||= merge_request.source_branch_sha
self.base_commit_sha ||= find_base_sha
end
end end
# Override head_commit_sha to keep compatibility with merge request diff # Override head_commit_sha to keep compatibility with merge request diff
......
...@@ -114,6 +114,7 @@ module MergeRequests ...@@ -114,6 +114,7 @@ module MergeRequests
merge_to_ref_success = merge_to_ref merge_to_ref_success = merge_to_ref
reload_merge_head_diff
update_diff_discussion_positions! if merge_to_ref_success update_diff_discussion_positions! if merge_to_ref_success
if merge_to_ref_success && can_git_merge? if merge_to_ref_success && can_git_merge?
...@@ -123,6 +124,10 @@ module MergeRequests ...@@ -123,6 +124,10 @@ module MergeRequests
end end
end end
def reload_merge_head_diff
MergeRequests::ReloadMergeHeadDiffService.new(merge_request).execute
end
def update_diff_discussion_positions! def update_diff_discussion_positions!
Discussions::CaptureDiffNotePositionsService.new(merge_request).execute Discussions::CaptureDiffNotePositionsService.new(merge_request).execute
end end
...@@ -153,6 +158,7 @@ module MergeRequests ...@@ -153,6 +158,7 @@ module MergeRequests
def merge_to_ref def merge_to_ref
params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) } params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) }
result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request) result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request)
result[:status] == :success result[:status] == :success
end end
......
# frozen_string_literal: true
module MergeRequests
class ReloadMergeHeadDiffService
include BaseServiceUtility
def initialize(merge_request)
@merge_request = merge_request
end
def execute
return error("default_merge_ref_for_diffs feature flag is disabled") unless enabled?
return error("Merge request has no merge ref head.") unless merge_request.merge_ref_head.present?
error_msg = recreate_merge_head_diff
return error(error_msg) if error_msg
success
end
private
attr_reader :merge_request
def enabled?
Feature.enabled?(:default_merge_ref_for_diffs, merge_request.project)
end
def recreate_merge_head_diff
merge_request.merge_head_diff&.destroy!
# n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377
Gitlab::GitalyClient.allow_n_plus_1_calls do
merge_request.create_merge_head_diff!
end
# Reset the merge request so it won't load the merge head diff as the
# MergeRequest#merge_request_diff.
merge_request.reset
nil
rescue StandardError => e
message = "Failed to recreate merge head diff: #{e.message}"
Gitlab::AppLogger.error(message: message, merge_request_id: merge_request.id)
message
end
end
end
---
title: Support batch loading of merge head diffs
merge_request: 51078
author:
type: performance
# frozen_string_literal: true
class AddDiffTypeToMergeRequestDiffs < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
UNIQUE_INDEX_NAME = 'index_merge_request_diffs_on_unique_merge_request_id'
def up
unless column_exists?(:merge_request_diffs, :diff_type)
with_lock_retries do
add_column :merge_request_diffs, :diff_type, :integer, null: false, limit: 2, default: 1
end
end
add_concurrent_index :merge_request_diffs, :merge_request_id, unique: true, where: 'diff_type = 2', name: UNIQUE_INDEX_NAME
end
def down
remove_concurrent_index_by_name(:merge_request_diffs, UNIQUE_INDEX_NAME)
if column_exists?(:merge_request_diffs, :diff_type)
with_lock_retries do
remove_column :merge_request_diffs, :diff_type
end
end
end
end
f76ce27a82f4773dcda324d79cc93a044f21317dbb9fdff035879502b5752da3
\ No newline at end of file
...@@ -14011,6 +14011,7 @@ CREATE TABLE merge_request_diffs ( ...@@ -14011,6 +14011,7 @@ CREATE TABLE merge_request_diffs (
stored_externally boolean, stored_externally boolean,
files_count smallint, files_count smallint,
sorted boolean DEFAULT false NOT NULL, sorted boolean DEFAULT false NOT NULL,
diff_type smallint DEFAULT 1 NOT NULL,
CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL)) CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL))
); );
...@@ -22242,6 +22243,8 @@ CREATE INDEX index_merge_request_diffs_on_external_diff_store ON merge_request_d ...@@ -22242,6 +22243,8 @@ CREATE INDEX index_merge_request_diffs_on_external_diff_store ON merge_request_d
CREATE INDEX index_merge_request_diffs_on_merge_request_id_and_id ON merge_request_diffs USING btree (merge_request_id, id); CREATE INDEX index_merge_request_diffs_on_merge_request_id_and_id ON merge_request_diffs USING btree (merge_request_id, id);
CREATE UNIQUE INDEX index_merge_request_diffs_on_unique_merge_request_id ON merge_request_diffs USING btree (merge_request_id) WHERE (diff_type = 2);
CREATE INDEX index_merge_request_metrics_on_first_deployed_to_production_at ON merge_request_metrics USING btree (first_deployed_to_production_at); CREATE INDEX index_merge_request_metrics_on_first_deployed_to_production_at ON merge_request_metrics USING btree (first_deployed_to_production_at);
CREATE INDEX index_merge_request_metrics_on_latest_closed_at ON merge_request_metrics USING btree (latest_closed_at) WHERE (latest_closed_at IS NOT NULL); CREATE INDEX index_merge_request_metrics_on_latest_closed_at ON merge_request_metrics USING btree (latest_closed_at) WHERE (latest_closed_at IS NOT NULL);
......
...@@ -928,6 +928,20 @@ For example, if there is a connection timeout: ...@@ -928,6 +928,20 @@ For example, if there is a connection timeout:
error="failed to connect to internal Pages API: Get \"https://gitlab.example.com:3000/api/v4/internal/pages/status\": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)" error="failed to connect to internal Pages API: Get \"https://gitlab.example.com:3000/api/v4/internal/pages/status\": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)"
``` ```
### Pages cannot communicate with an instance of the GitLab API
If you use the default value for `domain_config_source=auto` and run multiple instances of GitLab
Pages, you may see intermittent 502 error responses while serving Pages content. You may also see
the following warning in the Pages logs:
```plaintext
WARN[0010] Pages cannot communicate with an instance of the GitLab API. Please sync your gitlab-secrets.json file https://gitlab.com/gitlab-org/gitlab-pages/-/issues/535#workaround. error="pages endpoint unauthorized"
```
This can happen if your `gitlab-secrets.json` file is out of date between GitLab Rails and GitLab
Pages. Follow steps 8-10 of [Running GitLab Pages on a separate server](#running-gitlab-pages-on-a-separate-server),
in all of your GitLab Pages instances.
### 500 error with `securecookie: failed to generate random iv` and `Failed to save the session` ### 500 error with `securecookie: failed to generate random iv` and `Failed to save the session`
This problem most likely results from an [out-dated operating system](https://docs.gitlab.com/omnibus/package-information/deprecated_os.html). This problem most likely results from an [out-dated operating system](https://docs.gitlab.com/omnibus/package-information/deprecated_os.html).
......
...@@ -6,7 +6,8 @@ import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleto ...@@ -6,7 +6,8 @@ import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleto
import { filterToQueryObject } from '~/vue_shared/components/filtered_search_bar/filtered_search_utils'; import { filterToQueryObject } from '~/vue_shared/components/filtered_search_bar/filtered_search_utils';
import throughputChartQueryBuilder from '../graphql/throughput_chart_query_builder'; import throughputChartQueryBuilder from '../graphql/throughput_chart_query_builder';
import { THROUGHPUT_CHART_STRINGS } from '../constants'; import { THROUGHPUT_CHART_STRINGS } from '../constants';
import { formatThroughputChartData } from '../utils'; import { formatThroughputChartData, computeMttmData } from '../utils';
import ThroughputStats from './throughput_stats.vue';
export default { export default {
name: 'ThroughputChart', name: 'ThroughputChart',
...@@ -14,6 +15,7 @@ export default { ...@@ -14,6 +15,7 @@ export default {
GlAreaChart, GlAreaChart,
GlAlert, GlAlert,
ChartSkeletonLoader, ChartSkeletonLoader,
ThroughputStats,
}, },
inject: ['fullPath'], inject: ['fullPath'],
props: { props: {
...@@ -88,7 +90,7 @@ export default { ...@@ -88,7 +90,7 @@ export default {
formattedThroughputChartData() { formattedThroughputChartData() {
return formatThroughputChartData(this.throughputChartData); return formatThroughputChartData(this.throughputChartData);
}, },
chartDataLoading() { isLoading() {
return !this.hasError && this.$apollo.queries.throughputChartData.loading; return !this.hasError && this.$apollo.queries.throughputChartData.loading;
}, },
chartDataAvailable() { chartDataAvailable() {
...@@ -102,6 +104,9 @@ export default { ...@@ -102,6 +104,9 @@ export default {
: THROUGHPUT_CHART_STRINGS.NO_DATA, : THROUGHPUT_CHART_STRINGS.NO_DATA,
}; };
}, },
singleStatsValues() {
return [computeMttmData(this.throughputChartData)];
},
}, },
strings: { strings: {
chartTitle: THROUGHPUT_CHART_STRINGS.CHART_TITLE, chartTitle: THROUGHPUT_CHART_STRINGS.CHART_TITLE,
...@@ -111,11 +116,12 @@ export default { ...@@ -111,11 +116,12 @@ export default {
</script> </script>
<template> <template>
<div> <div>
<throughput-stats :stats="singleStatsValues" :is-loading="isLoading" />
<h4 data-testid="chartTitle">{{ $options.strings.chartTitle }}</h4> <h4 data-testid="chartTitle">{{ $options.strings.chartTitle }}</h4>
<div class="gl-text-gray-500" data-testid="chartDescription"> <div class="gl-text-gray-500" data-testid="chartDescription">
{{ $options.strings.chartDescription }} {{ $options.strings.chartDescription }}
</div> </div>
<chart-skeleton-loader v-if="chartDataLoading" /> <chart-skeleton-loader v-if="isLoading" />
<gl-area-chart <gl-area-chart
v-else-if="chartDataAvailable" v-else-if="chartDataAvailable"
:data="formattedThroughputChartData" :data="formattedThroughputChartData"
......
<script>
import { GlSkeletonLoader } from '@gitlab/ui';
import { GlSingleStat } from '@gitlab/ui/dist/charts';
import { STAT_LOADER_HEIGHT } from '../constants';
export default {
name: 'ThroughputStats',
components: {
GlSingleStat,
GlSkeletonLoader,
},
props: {
stats: {
type: Array,
required: true,
},
isLoading: {
type: Boolean,
required: false,
default: false,
},
},
loaderHeight: STAT_LOADER_HEIGHT,
};
</script>
<template>
<div class="gl-my-7 gl-display-flex">
<div v-for="stat in stats" :key="stat.title">
<gl-skeleton-loader v-if="isLoading" :height="$options.loaderHeight" />
<gl-single-stat v-else :value="stat.value" :title="stat.title" :unit="stat.unit" />
</div>
</div>
</template>
import { __ } from '~/locale'; import { __ } from '~/locale';
export const DEFAULT_NUMBER_OF_DAYS = 365; export const DEFAULT_NUMBER_OF_DAYS = 365;
export const STAT_LOADER_HEIGHT = 46;
export const PER_PAGE = 20; export const PER_PAGE = 20;
export const ASSIGNEES_VISIBLE = 2; export const ASSIGNEES_VISIBLE = 2;
export const AVATAR_SIZE = 24; export const AVATAR_SIZE = 24;
...@@ -14,6 +15,7 @@ export const THROUGHPUT_CHART_STRINGS = { ...@@ -14,6 +15,7 @@ export const THROUGHPUT_CHART_STRINGS = {
ERROR_FETCHING_DATA: __( ERROR_FETCHING_DATA: __(
'There was an error while fetching the chart data. Please refresh the page to try again.', 'There was an error while fetching the chart data. Please refresh the page to try again.',
), ),
MTTM: __('Mean time to merge'),
}; };
export const THROUGHPUT_TABLE_STRINGS = { export const THROUGHPUT_TABLE_STRINGS = {
...@@ -51,3 +53,7 @@ export const PIPELINE_STATUS_ICON_CLASSES = { ...@@ -51,3 +53,7 @@ export const PIPELINE_STATUS_ICON_CLASSES = {
status_pending: 'gl-text-orange-500', status_pending: 'gl-text-orange-500',
default: 'gl-text-grey-500', default: 'gl-text-grey-500',
}; };
export const UNITS = {
DAYS: __('days'),
};
...@@ -32,7 +32,7 @@ export default (startDate = null, endDate = null) => { ...@@ -32,7 +32,7 @@ export default (startDate = null, endDate = null) => {
milestoneTitle: $milestoneTitle, milestoneTitle: $milestoneTitle,
sourceBranches: $sourceBranches, sourceBranches: $sourceBranches,
targetBranches: $targetBranches targetBranches: $targetBranches
) { count } ) { count, totalTimeToMerge }
`; `;
}); });
......
...@@ -4,9 +4,10 @@ import { ...@@ -4,9 +4,10 @@ import {
dateFromParams, dateFromParams,
getDateInPast, getDateInPast,
getDayDifference, getDayDifference,
secondsToDays,
} from '~/lib/utils/datetime_utility'; } from '~/lib/utils/datetime_utility';
import { dateFormats } from '../shared/constants'; import { dateFormats } from '../shared/constants';
import { THROUGHPUT_CHART_STRINGS, DEFAULT_NUMBER_OF_DAYS } from './constants'; import { THROUGHPUT_CHART_STRINGS, DEFAULT_NUMBER_OF_DAYS, UNITS } from './constants';
/** /**
* A utility function which accepts a date range and returns * A utility function which accepts a date range and returns
...@@ -73,6 +74,42 @@ export const formatThroughputChartData = (chartData) => { ...@@ -73,6 +74,42 @@ export const formatThroughputChartData = (chartData) => {
]; ];
}; };
/**
* A utility function which accepts the raw throughput data
* and computes the mean time to merge.
*
* @param {Object} rawData the raw throughput data
*
* @return {Object} the computed MTTM data
*/
export const computeMttmData = (rawData) => {
if (!rawData) return {};
const mttmData = Object.values(rawData)
// eslint-disable-next-line @gitlab/require-i18n-strings
.filter((value) => value !== 'Project')
.reduce(
(total, monthData) => {
return {
count: total.count + monthData.count,
totalTimeToMerge: total.totalTimeToMerge + monthData.totalTimeToMerge,
};
},
{
count: 0,
totalTimeToMerge: 0,
},
);
// GlSingleStat expects a String for the 'value' prop
// https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1152
return {
title: THROUGHPUT_CHART_STRINGS.MTTM,
value: `${secondsToDays(mttmData.totalTimeToMerge / mttmData.count)}`,
unit: UNITS.DAYS,
};
};
/** /**
* A utility function which accepts start and end date params * A utility function which accepts start and end date params
* and validates that the date range does not exceed the bounds * and validates that the date range does not exceed the bounds
......
...@@ -2,12 +2,12 @@ ...@@ -2,12 +2,12 @@
- if group_packages_nav? - if group_packages_nav?
= nav_link(controller: ['groups/packages', 'groups/registry/repositories', 'groups/dependency_proxies']) do = nav_link(controller: ['groups/packages', 'groups/registry/repositories', 'groups/dependency_proxies']) do
= link_to packages_link, title: _('Packages') do = link_to packages_link, title: _('Packages'), data: { qa_selector: 'group_packages_item' } do
.nav-icon-container .nav-icon-container
= sprite_icon('package') = sprite_icon('package')
%span.nav-item-name %span.nav-item-name
= _('Packages & Registries') = _('Packages & Registries')
%ul.sidebar-sub-level-items %ul.sidebar-sub-level-items{ data: { qa_selector: 'group_packages_submenu' } }
= nav_link(controller: [:packages, :repositories], html_options: { class: "fly-out-top-item" } ) do = nav_link(controller: [:packages, :repositories], html_options: { class: "fly-out-top-item" } ) do
= link_to packages_link, title: _('Packages & Registries') do = link_to packages_link, title: _('Packages & Registries') do
%strong.fly-out-top-item-name %strong.fly-out-top-item-name
...@@ -15,7 +15,7 @@ ...@@ -15,7 +15,7 @@
%li.divider.fly-out-top-item %li.divider.fly-out-top-item
- if group_packages_list_nav? - if group_packages_list_nav?
= nav_link(controller: 'groups/packages') do = nav_link(controller: 'groups/packages') do
= link_to group_packages_path(@group), title: _('Packages') do = link_to group_packages_path(@group), title: _('Packages'), data: { qa_selector: 'group_packages_link' } do
%span= _('Package Registry') %span= _('Package Registry')
- if group_container_registry_nav? - if group_container_registry_nav?
= nav_link(controller: 'groups/registry/repositories') do = nav_link(controller: 'groups/registry/repositories') do
......
---
title: Add MTTM stat to MR Analytics
merge_request: 52316
author:
type: added
...@@ -3,6 +3,7 @@ import { GlAreaChart } from '@gitlab/ui/dist/charts'; ...@@ -3,6 +3,7 @@ import { GlAreaChart } from '@gitlab/ui/dist/charts';
import { shallowMount, createLocalVue } from '@vue/test-utils'; import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex'; import Vuex from 'vuex';
import ThroughputChart from 'ee/analytics/merge_request_analytics/components/throughput_chart.vue'; import ThroughputChart from 'ee/analytics/merge_request_analytics/components/throughput_chart.vue';
import ThroughputStats from 'ee/analytics/merge_request_analytics/components/throughput_stats.vue';
import { THROUGHPUT_CHART_STRINGS } from 'ee/analytics/merge_request_analytics/constants'; import { THROUGHPUT_CHART_STRINGS } from 'ee/analytics/merge_request_analytics/constants';
import store from 'ee/analytics/merge_request_analytics/store'; import store from 'ee/analytics/merge_request_analytics/store';
import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleton_loader.vue'; import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleton_loader.vue';
...@@ -67,6 +68,10 @@ describe('ThroughputChart', () => { ...@@ -67,6 +68,10 @@ describe('ThroughputChart', () => {
wrapper = createComponent(); wrapper = createComponent();
}); });
it('displays the throughput stats component', () => {
expect(wrapper.find(ThroughputStats).exists()).toBe(true);
});
it('displays the chart title', () => { it('displays the chart title', () => {
const chartTitle = wrapper.find('[data-testid="chartTitle"').text(); const chartTitle = wrapper.find('[data-testid="chartTitle"').text();
......
import { shallowMount } from '@vue/test-utils';
import { GlSkeletonLoader } from '@gitlab/ui';
import { GlSingleStat } from '@gitlab/ui/dist/charts';
import ThroughputStats from 'ee/analytics/merge_request_analytics/components/throughput_stats';
import { stats } from '../mock_data';
describe('ThroughputStats', () => {
let wrapper;
const createWrapper = (props) => {
return shallowMount(ThroughputStats, {
propsData: {
stats,
...props,
},
});
};
describe('default behaviour', () => {
beforeEach(() => {
wrapper = createWrapper();
});
it('displays a GlSingleStat component for each stat entry', () => {
expect(wrapper.findAll(GlSingleStat)).toHaveLength(stats.length);
});
it('passes the GlSingleStat the correct props', () => {
const component = wrapper.findAll(GlSingleStat).at(0);
const { title, unit, value } = stats[0];
expect(component.props('title')).toBe(title);
expect(component.props('unit')).toBe(unit);
expect(component.props('value')).toBe(value);
});
it('does not display any GlSkeletonLoader components', () => {
expect(wrapper.findAll(GlSkeletonLoader)).toHaveLength(0);
});
});
describe('loading', () => {
beforeEach(() => {
wrapper = createWrapper({ isLoading: true });
});
it('displays a GlSkeletonLoader component for each stat entry', () => {
expect(wrapper.findAll(GlSkeletonLoader)).toHaveLength(stats.length);
});
it('does not display any GlSingleStat components', () => {
expect(wrapper.findAll(GlSingleStat)).toHaveLength(0);
});
});
});
...@@ -8,9 +8,9 @@ export const fullPath = 'gitlab-org/gitlab'; ...@@ -8,9 +8,9 @@ export const fullPath = 'gitlab-org/gitlab';
// We should update our tests to use fixtures instead of hardcoded mock data. // We should update our tests to use fixtures instead of hardcoded mock data.
// https://gitlab.com/gitlab-org/gitlab/-/issues/270544 // https://gitlab.com/gitlab-org/gitlab/-/issues/270544
export const throughputChartData = { export const throughputChartData = {
May_2020: { count: 2, __typename: 'MergeRequestConnection' }, May_2020: { count: 2, totalTimeToMerge: 1234567, __typename: 'MergeRequestConnection' },
Jun_2020: { count: 4, __typename: 'MergeRequestConnection' }, Jun_2020: { count: 4, totalTimeToMerge: 2345678, __typename: 'MergeRequestConnection' },
Jul_2020: { count: 3, __typename: 'MergeRequestConnection' }, Jul_2020: { count: 3, totalTimeToMerge: 3456789, __typename: 'MergeRequestConnection' },
__typename: 'Project', __typename: 'Project',
}; };
...@@ -32,6 +32,12 @@ export const formattedThroughputChartData = [ ...@@ -32,6 +32,12 @@ export const formattedThroughputChartData = [
}, },
]; ];
export const formattedMttmData = {
title: 'Mean time to merge',
value: '9',
unit: 'days',
};
export const expectedMonthData = [ export const expectedMonthData = [
{ {
year: 2020, year: 2020,
...@@ -67,6 +73,7 @@ export const throughputChartQuery = `query ($fullPath: ID!, $labels: [String!], ...@@ -67,6 +73,7 @@ export const throughputChartQuery = `query ($fullPath: ID!, $labels: [String!],
targetBranches: $targetBranches targetBranches: $targetBranches
) { ) {
count count
totalTimeToMerge
} }
Jun_2020: mergeRequests( Jun_2020: mergeRequests(
first: 0 first: 0
...@@ -80,6 +87,7 @@ export const throughputChartQuery = `query ($fullPath: ID!, $labels: [String!], ...@@ -80,6 +87,7 @@ export const throughputChartQuery = `query ($fullPath: ID!, $labels: [String!],
targetBranches: $targetBranches targetBranches: $targetBranches
) { ) {
count count
totalTimeToMerge
} }
Jul_2020: mergeRequests( Jul_2020: mergeRequests(
first: 0 first: 0
...@@ -93,6 +101,7 @@ export const throughputChartQuery = `query ($fullPath: ID!, $labels: [String!], ...@@ -93,6 +101,7 @@ export const throughputChartQuery = `query ($fullPath: ID!, $labels: [String!],
targetBranches: $targetBranches targetBranches: $targetBranches
) { ) {
count count
totalTimeToMerge
} }
} }
} }
...@@ -148,3 +157,16 @@ export const throughputTableData = [ ...@@ -148,3 +157,16 @@ export const throughputTableData = [
}, },
}, },
]; ];
export const stats = [
{
title: 'Mean time to merge',
unit: 'days',
value: '10',
},
{
title: 'MRs per engineer',
unit: 'MRs per engineer (per month)',
value: '23',
},
];
import * as utils from 'ee/analytics/merge_request_analytics/utils'; import * as utils from 'ee/analytics/merge_request_analytics/utils';
import { useFakeDate } from 'helpers/fake_date'; import { useFakeDate } from 'helpers/fake_date';
import { expectedMonthData, throughputChartData, formattedThroughputChartData } from './mock_data'; import {
expectedMonthData,
throughputChartData,
formattedThroughputChartData,
formattedMttmData,
} from './mock_data';
describe('computeMonthRangeData', () => { describe('computeMonthRangeData', () => {
const start = new Date('2020-05-17T00:00:00.000Z'); const start = new Date('2020-05-17T00:00:00.000Z');
...@@ -33,6 +38,14 @@ describe('formatThroughputChartData', () => { ...@@ -33,6 +38,14 @@ describe('formatThroughputChartData', () => {
}); });
}); });
describe('computeMttmData', () => {
it('returns the data as expected', () => {
const mttmData = utils.computeMttmData(throughputChartData);
expect(mttmData).toStrictEqual(formattedMttmData);
});
});
describe('parseAndValidateDates', () => { describe('parseAndValidateDates', () => {
useFakeDate('2021-01-21'); useFakeDate('2021-01-21');
......
...@@ -17550,6 +17550,9 @@ msgstr "" ...@@ -17550,6 +17550,9 @@ msgstr ""
msgid "May" msgid "May"
msgstr "" msgstr ""
msgid "Mean time to merge"
msgstr ""
msgid "Measured in bytes of code. Excludes generated and vendored code." msgid "Measured in bytes of code. Excludes generated and vendored code."
msgstr "" msgstr ""
...@@ -33593,6 +33596,9 @@ msgid_plural "days" ...@@ -33593,6 +33596,9 @@ msgid_plural "days"
msgstr[0] "" msgstr[0] ""
msgstr[1] "" msgstr[1] ""
msgid "days"
msgstr ""
msgid "default branch" msgid "default branch"
msgstr "" msgstr ""
......
...@@ -53,6 +53,12 @@ module QA ...@@ -53,6 +53,12 @@ module QA
view 'ee/app/views/layouts/nav/sidebar/_group_iterations_link.html.haml' do view 'ee/app/views/layouts/nav/sidebar/_group_iterations_link.html.haml' do
element :group_iterations_link element :group_iterations_link
end end
view 'ee/app/views/groups/sidebar/_packages.html.haml' do
element :group_packages_item
element :group_packages_link
element :group_packages_submenu
end
end end
end end
...@@ -140,6 +146,14 @@ module QA ...@@ -140,6 +146,14 @@ module QA
end end
end end
def go_to_group_packages
hover_element(:group_packages_item) do
within_submenu(:group_packages_submenu) do
click_element(:group_packages_link)
end
end
end
def click_group_wiki_link def click_group_wiki_link
within_sidebar do within_sidebar do
click_element(:wiki_link) click_element(:wiki_link)
......
...@@ -13,6 +13,13 @@ module QA ...@@ -13,6 +13,13 @@ module QA
end end
end end
let(:another_project) do
Resource::Project.fabricate_via_api! do |project|
project.name = 'nuget-package-install-project'
project.template_name = 'dotnetcore'
end
end
let!(:runner) do let!(:runner) do
Resource::Runner.fabricate! do |runner| Resource::Runner.fabricate! do |runner|
runner.name = "qa-runner-#{Time.now.to_i}" runner.name = "qa-runner-#{Time.now.to_i}"
...@@ -22,11 +29,21 @@ module QA ...@@ -22,11 +29,21 @@ module QA
end end
end end
let!(:another_runner) do
Resource::Runner.fabricate! do |runner|
runner.name = "qa-runner-#{Time.now.to_i}"
runner.tags = ["runner-for-#{another_project.name}"]
runner.executor = :docker
runner.project = another_project
end
end
after do after do
runner.remove_via_api! runner.remove_via_api!
another_runner.remove_via_api!
end end
it 'publishes a nuget package and deletes it', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1073' do it 'publishes a nuget package at the project level, installs and deletes it at the group level', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1073' do
Flow::Login.sign_in Flow::Login.sign_in
Resource::Repository::Commit.fabricate_via_api! do |commit| Resource::Repository::Commit.fabricate_via_api! do |commit|
...@@ -37,23 +54,23 @@ module QA ...@@ -37,23 +54,23 @@ module QA
{ {
file_path: '.gitlab-ci.yml', file_path: '.gitlab-ci.yml',
content: <<~YAML content: <<~YAML
image: mcr.microsoft.com/dotnet/core/sdk:3.1 image: mcr.microsoft.com/dotnet/core/sdk:3.1
stages: stages:
- deploy - deploy
deploy: deploy:
stage: deploy stage: deploy
script: script:
- dotnet restore -p:Configuration=Release - dotnet restore -p:Configuration=Release
- dotnet build -c Release - dotnet build -c Release
- dotnet pack -c Release - dotnet pack -c Release
- dotnet nuget add source "$CI_SERVER_URL/api/v4/projects/$CI_PROJECT_ID/packages/nuget/index.json" --name gitlab --username gitlab-ci-token --password $CI_JOB_TOKEN --store-password-in-clear-text - dotnet nuget add source "$CI_SERVER_URL/api/v4/projects/$CI_PROJECT_ID/packages/nuget/index.json" --name gitlab --username gitlab-ci-token --password $CI_JOB_TOKEN --store-password-in-clear-text
- dotnet nuget push "bin/Release/*.nupkg" --source gitlab - dotnet nuget push "bin/Release/*.nupkg" --source gitlab
only: only:
- "#{project.default_branch}" - "#{project.default_branch}"
tags: tags:
- "runner-for-#{project.name}" - "runner-for-#{project.name}"
YAML YAML
} }
] ]
...@@ -71,7 +88,67 @@ module QA ...@@ -71,7 +88,67 @@ module QA
expect(job).to be_successful(timeout: 800) expect(job).to be_successful(timeout: 800)
end end
Page::Project::Menu.perform(&:click_packages_link) another_project.visit!
Resource::Repository::Commit.fabricate_via_api! do |commit|
commit.project = another_project
commit.commit_message = 'Add new csproj file'
commit.add_files(
[
{
file_path: 'otherdotnet.csproj',
content: <<~EOF
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp3.1</TargetFramework>
</PropertyGroup>
</Project>
EOF
}
]
)
commit.update_files(
[
{
file_path: '.gitlab-ci.yml',
content: <<~YAML
image: mcr.microsoft.com/dotnet/core/sdk:3.1
stages:
- install
install:
stage: install
script:
- dotnet nuget locals all --clear
- dotnet nuget add source "$CI_SERVER_URL/api/v4/groups/#{another_project.group.id}/-/packages/nuget/index.json" --name gitlab --username gitlab-ci-token --password $CI_JOB_TOKEN --store-password-in-clear-text
- "dotnet add otherdotnet.csproj package #{package_name} --version 1.0.0"
only:
- "#{another_project.default_branch}"
tags:
- "runner-for-#{another_project.name}"
YAML
}
]
)
end
Flow::Pipeline.visit_latest_pipeline
Page::Project::Pipeline::Show.perform do |pipeline|
pipeline.click_job('install')
end
Page::Project::Job::Show.perform do |job|
expect(job).to be_successful(timeout: 800)
end
project.group.sandbox.visit!
Page::Group::Menu.perform(&:go_to_group_packages)
Page::Project::Packages::Index.perform do |index| Page::Project::Packages::Index.perform do |index|
expect(index).to have_package(package_name) expect(index).to have_package(package_name)
......
...@@ -226,11 +226,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do ...@@ -226,11 +226,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
let(:diffable_merge_ref) { true } let(:diffable_merge_ref) { true }
it 'compares diffs with the head' do it 'compares diffs with the head' do
MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) create(:merge_request_diff, :merge_head, merge_request: merge_request)
expect(CompareService).to receive(:new).with(
project, merge_request.merge_ref_head.sha
).and_call_original
go(diff_head: true) go(diff_head: true)
...@@ -242,8 +238,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do ...@@ -242,8 +238,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
let(:diffable_merge_ref) { false } let(:diffable_merge_ref) { false }
it 'compares diffs with the base' do it 'compares diffs with the base' do
expect(CompareService).not_to receive(:new)
go(diff_head: true) go(diff_head: true)
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
......
...@@ -10,12 +10,18 @@ FactoryBot.define do ...@@ -10,12 +10,18 @@ FactoryBot.define do
head_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) } head_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
start_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) } start_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
diff_type { :regular }
trait :external do trait :external do
external_diff { fixture_file_upload("spec/fixtures/doc_sample.txt", "plain/txt") } external_diff { fixture_file_upload("spec/fixtures/doc_sample.txt", "plain/txt") }
stored_externally { true } stored_externally { true }
importing { true } # this avoids setting the state to 'empty' importing { true } # this avoids setting the state to 'empty'
end end
trait :merge_head do
diff_type { :merge_head }
end
factory :external_merge_request_diff, traits: [:external] factory :external_merge_request_diff, traits: [:external]
end end
end end
...@@ -146,6 +146,7 @@ merge_requests: ...@@ -146,6 +146,7 @@ merge_requests:
- merge_user - merge_user
- merge_request_diffs - merge_request_diffs
- merge_request_diff - merge_request_diff
- merge_head_diff
- merge_request_context_commits - merge_request_context_commits
- merge_request_context_commit_diff_files - merge_request_context_commit_diff_files
- events - events
......
...@@ -220,6 +220,7 @@ MergeRequestDiff: ...@@ -220,6 +220,7 @@ MergeRequestDiff:
- commits_count - commits_count
- files_count - files_count
- sorted - sorted
- diff_type
MergeRequestDiffCommit: MergeRequestDiffCommit:
- merge_request_diff_id - merge_request_diff_id
- relative_order - relative_order
......
...@@ -16,6 +16,8 @@ RSpec.describe MergeRequestDiff do ...@@ -16,6 +16,8 @@ RSpec.describe MergeRequestDiff do
describe 'validations' do describe 'validations' do
subject { diff_with_commits } subject { diff_with_commits }
it { is_expected.not_to validate_uniqueness_of(:diff_type).scoped_to(:merge_request_id) }
it 'checks sha format of base_commit_sha, head_commit_sha and start_commit_sha' do it 'checks sha format of base_commit_sha, head_commit_sha and start_commit_sha' do
subject.base_commit_sha = subject.head_commit_sha = subject.start_commit_sha = 'foobar' subject.base_commit_sha = subject.head_commit_sha = subject.start_commit_sha = 'foobar'
...@@ -23,6 +25,24 @@ RSpec.describe MergeRequestDiff do ...@@ -23,6 +25,24 @@ RSpec.describe MergeRequestDiff do
expect(subject.errors.count).to eq 3 expect(subject.errors.count).to eq 3
expect(subject.errors).to all(include('is not a valid SHA')) expect(subject.errors).to all(include('is not a valid SHA'))
end end
it 'does not validate uniqueness by default' do
expect(build(:merge_request_diff, merge_request: subject.merge_request)).to be_valid
end
context 'when merge request diff is a merge_head type' do
it 'is valid' do
expect(build(:merge_request_diff, :merge_head, merge_request: subject.merge_request)).to be_valid
end
context 'when merge_head diff exists' do
let(:existing_merge_head_diff) { create(:merge_request_diff, :merge_head) }
it 'validates uniqueness' do
expect(build(:merge_request_diff, :merge_head, merge_request: existing_merge_head_diff.merge_request)).not_to be_valid
end
end
end
end end
describe 'create new record' do describe 'create new record' do
...@@ -35,6 +55,26 @@ RSpec.describe MergeRequestDiff do ...@@ -35,6 +55,26 @@ RSpec.describe MergeRequestDiff do
it { expect(subject.head_commit_sha).to eq('b83d6e391c22777fca1ed3012fce84f633d7fed0') } it { expect(subject.head_commit_sha).to eq('b83d6e391c22777fca1ed3012fce84f633d7fed0') }
it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }
it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') } it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') }
context 'when diff_type is merge_head' do
let_it_be(:merge_request) { create(:merge_request) }
let_it_be(:merge_head) do
MergeRequests::MergeToRefService
.new(merge_request.project, merge_request.author)
.execute(merge_request)
merge_request.create_merge_head_diff
end
it { expect(merge_head).to be_valid }
it { expect(merge_head).to be_persisted }
it { expect(merge_head.commits.count).to eq(30) }
it { expect(merge_head.diffs.count).to eq(20) }
it { expect(merge_head.head_commit_sha).to eq(merge_request.merge_ref_head.diff_refs.head_sha) }
it { expect(merge_head.base_commit_sha).to eq(merge_request.merge_ref_head.diff_refs.base_sha) }
it { expect(merge_head.start_commit_sha).to eq(merge_request.target_branch_sha) }
end
end end
describe '.by_commit_sha' do describe '.by_commit_sha' do
...@@ -63,6 +103,7 @@ RSpec.describe MergeRequestDiff do ...@@ -63,6 +103,7 @@ RSpec.describe MergeRequestDiff do
let_it_be(:merge_request) { create(:merge_request) } let_it_be(:merge_request) { create(:merge_request) }
let_it_be(:outdated) { merge_request.merge_request_diff } let_it_be(:outdated) { merge_request.merge_request_diff }
let_it_be(:latest) { merge_request.create_merge_request_diff } let_it_be(:latest) { merge_request.create_merge_request_diff }
let_it_be(:merge_head) { merge_request.create_merge_head_diff }
let_it_be(:closed_mr) { create(:merge_request, :closed_last_month) } let_it_be(:closed_mr) { create(:merge_request, :closed_last_month) }
let(:closed) { closed_mr.merge_request_diff } let(:closed) { closed_mr.merge_request_diff }
...@@ -103,14 +144,14 @@ RSpec.describe MergeRequestDiff do ...@@ -103,14 +144,14 @@ RSpec.describe MergeRequestDiff do
stub_external_diffs_setting(enabled: true) stub_external_diffs_setting(enabled: true)
end end
it { is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, closed_recently.id, merged_recently.id) } it { is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, closed_recently.id, merged_recently.id, merge_head.id) }
it 'ignores diffs with 0 files' do it 'ignores diffs with 0 files' do
MergeRequestDiffFile.where(merge_request_diff_id: [closed_recently.id, merged_recently.id]).delete_all MergeRequestDiffFile.where(merge_request_diff_id: [closed_recently.id, merged_recently.id]).delete_all
closed_recently.update!(files_count: 0) closed_recently.update!(files_count: 0)
merged_recently.update!(files_count: 0) merged_recently.update!(files_count: 0)
is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id) is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, merge_head.id)
end end
end end
......
...@@ -491,6 +491,7 @@ RSpec.describe MergeRequest, factory_default: :keep do ...@@ -491,6 +491,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
create(:merge_request, params).tap do |mr| create(:merge_request, params).tap do |mr|
diffs.times { mr.merge_request_diffs.create } diffs.times { mr.merge_request_diffs.create }
mr.create_merge_head_diff
end end
end end
...@@ -4379,37 +4380,41 @@ RSpec.describe MergeRequest, factory_default: :keep do ...@@ -4379,37 +4380,41 @@ RSpec.describe MergeRequest, factory_default: :keep do
end end
describe '#diffable_merge_ref?' do describe '#diffable_merge_ref?' do
let(:merge_request) { create(:merge_request) }
context 'merge request can be merged' do context 'merge request can be merged' do
context 'merge_to_ref is not calculated' do context 'merge_head diff is not created' do
it 'returns true' do it 'returns true' do
expect(subject.diffable_merge_ref?).to eq(false) expect(merge_request.diffable_merge_ref?).to eq(false)
end end
end end
context 'merge_to_ref is calculated' do context 'merge_head diff is created' do
before do before do
MergeRequests::MergeToRefService.new(subject.project, subject.author).execute(subject) create(:merge_request_diff, :merge_head, merge_request: merge_request)
end end
it 'returns true' do it 'returns true' do
expect(subject.diffable_merge_ref?).to eq(true) expect(merge_request.diffable_merge_ref?).to eq(true)
end end
context 'merge request is merged' do context 'merge request is merged' do
subject { build_stubbed(:merge_request, :merged, project: project) } before do
merge_request.mark_as_merged!
end
it 'returns false' do it 'returns false' do
expect(subject.diffable_merge_ref?).to eq(false) expect(merge_request.diffable_merge_ref?).to eq(false)
end end
end end
context 'merge request cannot be merged' do context 'merge request cannot be merged' do
before do before do
subject.mark_as_unchecked! merge_request.mark_as_unchecked!
end end
it 'returns false' do it 'returns false' do
expect(subject.diffable_merge_ref?).to eq(true) expect(merge_request.diffable_merge_ref?).to eq(true)
end end
context 'display_merge_conflicts_in_diff is disabled' do context 'display_merge_conflicts_in_diff is disabled' do
...@@ -4418,7 +4423,7 @@ RSpec.describe MergeRequest, factory_default: :keep do ...@@ -4418,7 +4423,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
end end
it 'returns false' do it 'returns false' do
expect(subject.diffable_merge_ref?).to eq(false) expect(merge_request.diffable_merge_ref?).to eq(false)
end end
end end
end end
......
...@@ -12,6 +12,8 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s ...@@ -12,6 +12,8 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s
stub_const("#{described_class.name}::BATCH_SIZE", 2) stub_const("#{described_class.name}::BATCH_SIZE", 2)
3.times { merge_request.create_merge_request_diff } 3.times { merge_request.create_merge_request_diff }
merge_request.create_merge_head_diff
merge_request.reset
end end
it 'schedules non-latest merge request diffs removal' do it 'schedules non-latest merge request diffs removal' do
......
...@@ -33,6 +33,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar ...@@ -33,6 +33,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
expect(merge_request.merge_status).to eq('can_be_merged') expect(merge_request.merge_status).to eq('can_be_merged')
end end
it 'reloads merge head diff' do
expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service|
expect(service).to receive(:execute)
end
subject
end
it 'update diff discussion positions' do it 'update diff discussion positions' do
expect_next_instance_of(Discussions::CaptureDiffNotePositionsService) do |service| expect_next_instance_of(Discussions::CaptureDiffNotePositionsService) do |service|
expect(service).to receive(:execute) expect(service).to receive(:execute)
...@@ -142,7 +150,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar ...@@ -142,7 +150,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
end end
it 'resets one merge request upon execution' do it 'resets one merge request upon execution' do
expect_any_instance_of(MergeRequest).to receive(:reset).once expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |svc|
expect(svc).to receive(:execute).and_return(status: :success)
end
expect_any_instance_of(MergeRequest).to receive(:reset).once.and_call_original
execute_within_threads(amount: 2) execute_within_threads(amount: 2)
end end
...@@ -266,6 +278,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar ...@@ -266,6 +278,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
it_behaves_like 'unmergeable merge request' it_behaves_like 'unmergeable merge request'
it 'reloads merge head diff' do
expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service|
expect(service).to receive(:execute)
end
subject
end
it 'returns ServiceResponse.error' do it 'returns ServiceResponse.error' do
result = subject result = subject
...@@ -329,6 +349,12 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar ...@@ -329,6 +349,12 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
subject subject
end end
it 'does not reload merge head diff' do
expect(MergeRequests::ReloadMergeHeadDiffService).not_to receive(:new)
subject
end
end end
end end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe MergeRequests::ReloadMergeHeadDiffService do
let(:merge_request) { create(:merge_request) }
subject { described_class.new(merge_request).execute }
describe '#execute' do
before do
MergeRequests::MergeToRefService
.new(merge_request.project, merge_request.author)
.execute(merge_request)
end
it 'creates a merge head diff' do
expect(subject[:status]).to eq(:success)
expect(merge_request.reload.merge_head_diff).to be_present
end
context 'when merge ref head is not present' do
before do
allow(merge_request).to receive(:merge_ref_head).and_return(nil)
end
it 'returns error' do
expect(subject[:status]).to eq(:error)
end
end
context 'when failed to create merge head diff' do
before do
allow(merge_request).to receive(:create_merge_head_diff!).and_raise('fail')
end
it 'returns error' do
expect(subject[:status]).to eq(:error)
end
end
context 'when there is existing merge head diff' do
let!(:existing_merge_head_diff) { create(:merge_request_diff, :merge_head, merge_request: merge_request) }
it 'recreates merge head diff' do
expect(subject[:status]).to eq(:success)
expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff)
end
end
context 'when default_merge_ref_for_diffs feature flag is disabled' do
before do
stub_feature_flags(default_merge_ref_for_diffs: false)
end
it 'returns error' do
expect(subject[:status]).to eq(:error)
end
end
end
end
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