From 8f6208513a98b33d7edd6ecf1ae6062f266c279f Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 19 Sep 2016 12:56:25 +0530
Subject: [PATCH] Test all cycle analytics pre-calculation code.
All the code that pre-calculates metrics for use in the cycle analytics
page.
- Ci::Pipeline -> build start/finish
- Ci::Pipeline#merge_requests
- Issue -> record default metrics after save
- MergeRequest -> record default metrics after save
- Deployment -> Update "first_deployed_to_production_at" for MR metrics
- Git Push -> Update "first commit mention" for issue metrics
- Merge request create/update/refresh -> Update "merge requests closing issues"
---
app/models/ci/pipeline.rb | 2 +-
app/models/merge_request.rb | 1 +
app/services/create_deployment_service.rb | 25 ++++--
app/services/git_push_service.rb | 1 -
spec/models/ci/pipeline_spec.rb | 55 ++++++++++++++
spec/models/issue/metrics_spec.rb | 55 ++++++++++++++
spec/models/merge_request/metrics_spec.rb | 18 +++++
.../create_deployment_service_spec.rb | 76 +++++++++++++++++++
spec/services/git_push_service_spec.rb | 37 +++++++++
.../merge_requests/create_service_spec.rb | 28 +++++++
.../merge_requests/refresh_service_spec.rb | 30 ++++++++
.../merge_requests/update_service_spec.rb | 34 +++++++++
12 files changed, 352 insertions(+), 10 deletions(-)
create mode 100644 spec/models/issue/metrics_spec.rb
create mode 100644 spec/models/merge_request/metrics_spec.rb
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 1f7e6b11bb9..66f43456c06 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -283,7 +283,7 @@ module Ci
# the merge request's latest commit.
def merge_requests
project.merge_requests.where(source_branch: self.ref).
- select { |merge_request| merge_request.pipeline.id == self.id }
+ select { |merge_request| merge_request.pipeline.try(:id) == self.id }
end
private
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 14e093eed93..6548b84ea7a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -503,6 +503,7 @@ class MergeRequest < ActiveRecord::Base
# running `ReferenceExtractor` on each of them separately.
def cache_merge_request_closes_issues!(current_user = self.author)
transaction do
+ self.merge_requests_closing_issues.destroy_all
closes_issues(current_user).each do |issue|
MergeRequestsClosingIssues.create!(merge_request: self, issue: issue)
end
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index cc63eeae9c3..c126b2fad31 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -23,17 +23,26 @@ class CreateDeploymentService < BaseService
private
def update_merge_request_metrics(deployment, environment)
- # TODO: Test cases:
- # 1. Merge request with metrics available and `first_deployed_to_production_at` is nil
- # 2. Merge request with metrics available and `first_deployed_to_production_at` is set
- # 3. Merge request with metrics unavailable
- # 4. Only applies to merge requests merged AFTER the previous production deploy to this branch
if environment.name == "production"
- merge_requests_deployed_to_production_for_first_time = project.merge_requests.joins("LEFT OUTER JOIN merge_request_metrics ON merge_request_metrics.merge_request_id = merge_requests.id").
- where(target_branch: params[:ref], "merge_request_metrics.first_deployed_to_production_at" => nil).
- where("merge_request_metrics.merged_at < ?", deployment.created_at)
+ query = project.merge_requests.joins("LEFT OUTER JOIN merge_request_metrics ON merge_request_metrics.merge_request_id = merge_requests.id").
+ where(target_branch: params[:ref], "merge_request_metrics.first_deployed_to_production_at" => nil)
+
+ previous_deployment = previous_deployment_for_ref(deployment)
+ merge_requests_deployed_to_production_for_first_time = if previous_deployment
+ query.where("merge_request_metrics.merged_at < ? AND merge_request_metrics.merged_at > ?", deployment.created_at, previous_deployment.created_at)
+ else
+ query.where("merge_request_metrics.merged_at < ?", deployment.created_at)
+ end
merge_requests_deployed_to_production_for_first_time.each { |merge_request| merge_request.metrics.record_production_deploy!(deployment.created_at) }
end
end
+
+ def previous_deployment_for_ref(current_deployment)
+ @previous_deployment_for_ref ||=
+ project.deployments.joins(:environment).
+ where("environments.name": params[:environment], ref: params[:ref]).
+ where.not(id: current_deployment.id).
+ first
+ end
end
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 0f4f805155f..2aab67b4b9d 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -188,7 +188,6 @@ class GitPushService < BaseService
@branch_name ||= Gitlab::Git.ref_name(params[:ref])
end
- # TODO: What about commits created using the Web UI? Cross references are not created there.
def update_issue_metrics(commit, authors)
mentioned_issues = commit.all_references(authors[commit]).issues
mentioned_issues.each { |issue| issue.metrics.record_commit_mention!(commit.committed_date) if issue.metrics.present? }
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 598df576001..5bafcc13f61 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -169,6 +169,37 @@ describe Ci::Pipeline, models: true do
expect(pipeline.reload.finished_at).to be_nil
end
end
+
+ describe "merge request metrics" do
+ let(:project) { FactoryGirl.create :project }
+ let(:pipeline) { FactoryGirl.create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) }
+ let!(:merge_request) { create(:merge_request, source_project: project, source_branch: pipeline.ref) }
+
+ context 'when transitioning to running' do
+ it 'records the build start time' do
+ time = Time.now
+ Timecop.freeze(time) { build.run }
+
+ expect(merge_request.metrics.latest_build_started_at).to eq(time)
+ end
+
+ it 'clears the build end time' do
+ build.run
+
+ expect(merge_request.metrics.latest_build_finished_at).to be_nil
+ end
+ end
+
+ context 'when transitioning to success' do
+ it 'records the build end time' do
+ build.run
+ time = Time.now
+ Timecop.freeze(time) { build.success }
+
+ expect(merge_request.metrics.latest_build_finished_at).to eq(time)
+ end
+ end
+ end
end
describe '#branch?' do
@@ -429,4 +460,28 @@ describe Ci::Pipeline, models: true do
create(:ci_build, :created, pipeline: pipeline, name: name)
end
end
+
+ describe "#merge_requests" do
+ let(:project) { FactoryGirl.create :project }
+ let(:pipeline) { FactoryGirl.create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) }
+
+ it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do
+ merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
+
+ expect(pipeline.merge_requests).to eq([merge_request])
+ end
+
+ it "doesn't return merge requests whose source branch doesn't match the pipeline's ref" do
+ create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master')
+
+ expect(pipeline.merge_requests).to be_empty
+ end
+
+ it "doesn't return merge requests whose `diff_head_sha` doesn't match the pipeline's SHA" do
+ create(:merge_request, source_project: project, source_branch: pipeline.ref)
+ allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { '97de212e80737a608d939f648d959671fb0a0142b' }
+
+ expect(pipeline.merge_requests).to be_empty
+ end
+ end
end
diff --git a/spec/models/issue/metrics_spec.rb b/spec/models/issue/metrics_spec.rb
new file mode 100644
index 00000000000..df977486943
--- /dev/null
+++ b/spec/models/issue/metrics_spec.rb
@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+describe Issue::Metrics, models: true do
+ let(:project) { create(:project) }
+
+ subject { create(:issue, project: project) }
+
+ describe "when recording the default set of issue metrics on issue save" do
+ context "milestones" do
+ it "records the first time an issue is associated with a milestone" do
+ time = Time.now
+ Timecop.freeze(time) { subject.update(milestone: create(:milestone)) }
+ metrics = subject.metrics
+
+ expect(metrics).to be_present
+ expect(metrics.first_associated_with_milestone_at).to eq(time)
+ end
+
+ it "does not record the second time an issue is associated with a milestone" do
+ time = Time.now
+ Timecop.freeze(time) { subject.update(milestone: create(:milestone)) }
+ Timecop.freeze(time + 2.hours) { subject.update(milestone: nil) }
+ Timecop.freeze(time + 6.hours) { subject.update(milestone: create(:milestone)) }
+ metrics = subject.metrics
+
+ expect(metrics).to be_present
+ expect(metrics.first_associated_with_milestone_at).to eq(time)
+ end
+ end
+
+ context "list labels" do
+ it "records the first time an issue is associated with a list label" do
+ list_label = create(:label, lists: [create(:list)])
+ time = Time.now
+ Timecop.freeze(time) { subject.update(label_ids: [list_label.id]) }
+ metrics = subject.metrics
+
+ expect(metrics).to be_present
+ expect(metrics.first_added_to_board_at).to eq(time)
+ end
+
+ it "does not record the second time an issue is associated with a list label" do
+ time = Time.now
+ first_list_label = create(:label, lists: [create(:list)])
+ Timecop.freeze(time) { subject.update(label_ids: [first_list_label.id]) }
+ second_list_label = create(:label, lists: [create(:list)])
+ Timecop.freeze(time + 5.hours) { subject.update(label_ids: [second_list_label.id]) }
+ metrics = subject.metrics
+
+ expect(metrics).to be_present
+ expect(metrics.first_added_to_board_at).to eq(time)
+ end
+ end
+ end
+end
diff --git a/spec/models/merge_request/metrics_spec.rb b/spec/models/merge_request/metrics_spec.rb
new file mode 100644
index 00000000000..718b50642ad
--- /dev/null
+++ b/spec/models/merge_request/metrics_spec.rb
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe MergeRequest::Metrics, models: true do
+ let(:project) { create(:project) }
+
+ subject { create(:merge_request, source_project: project) }
+
+ describe "when recording the default set of metrics on merge request save" do
+ it "records the merge time" do
+ time = Time.now
+ Timecop.freeze(time) { subject.mark_as_merged }
+ metrics = subject.metrics
+
+ expect(metrics).to be_present
+ expect(metrics.merged_at).to eq(time)
+ end
+ end
+end
diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb
index 8da2a2b3c1b..5cc0f2ab974 100644
--- a/spec/services/create_deployment_service_spec.rb
+++ b/spec/services/create_deployment_service_spec.rb
@@ -132,4 +132,80 @@ describe CreateDeploymentService, services: true do
end
end
end
+
+ describe "merge request metrics" do
+ let(:params) do
+ {
+ environment: 'production',
+ ref: 'master',
+ tag: false,
+ sha: '97de212e80737a608d939f648d959671fb0a0142b',
+ }
+ end
+
+ let(:merge_request) { create(:merge_request, target_branch: 'master', source_branch: 'feature', source_project: project) }
+
+ context "while updating the 'first_deployed_to_production_at' time" do
+ before { merge_request.mark_as_merged }
+
+ context "for merge requests merged before the current deploy" do
+ it "sets the time if the deploy's environment is 'production'" do
+ time = Time.now
+ Timecop.freeze(time) { service.execute }
+
+ expect(merge_request.metrics.first_deployed_to_production_at).to eq(time)
+ end
+
+ it "doesn't set the time if the deploy's environment is not 'production'" do
+ staging_params = params.merge(environment: 'staging')
+ service = described_class.new(project, user, staging_params)
+ service.execute
+
+ expect(merge_request.metrics.first_deployed_to_production_at).to be_nil
+ end
+
+ it 'does not raise errors if the merge request does not have a metrics record' do
+ merge_request.metrics.destroy
+
+ expect(merge_request.reload.metrics).to be_nil
+ expect { service.execute }.not_to raise_error
+ end
+ end
+
+ context "for merge requests merged before the previous deploy" do
+ context "if the 'first_deployed_to_production_at' time is already set" do
+ it "does not overwrite the older 'first_deployed_to_production_at' time" do
+ # Previous deploy
+ time = Time.now
+ Timecop.freeze(time) { service.execute }
+
+ expect(merge_request.reload.metrics.first_deployed_to_production_at).to eq(time)
+
+ # Current deploy
+ service = described_class.new(project, user, params)
+ Timecop.freeze(time + 12.hours) { service.execute }
+
+ expect(merge_request.reload.metrics.first_deployed_to_production_at).to eq(time)
+ end
+ end
+
+ context "if the 'first_deployed_to_production_at' time is not already set" do
+ it "does not overwrite the older 'first_deployed_to_production_at' time" do
+ # Previous deploy
+ time = Time.now
+ Timecop.freeze(time) { service.execute }
+ merge_request.metrics.update(first_deployed_to_production_at: nil)
+
+ expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_nil
+
+ # Current deploy
+ service = described_class.new(project, user, params)
+ Timecop.freeze(time + 12.hours) { service.execute }
+
+ expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_nil
+ end
+ end
+ end
+ end
+ end
end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 6ac1fa8f182..125110e0c37 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -324,6 +324,43 @@ describe GitPushService, services: true do
end
end
+ describe "issue metrics" do
+ let(:issue) { create :issue, project: project }
+ let(:commit_author) { create :user }
+ let(:commit) { project.commit }
+ let(:commit_time) { Time.now }
+
+ before do
+ project.team << [commit_author, :developer]
+ project.team << [user, :developer]
+
+ allow(commit).to receive_messages(
+ safe_message: "this commit \n mentions #{issue.to_reference}",
+ references: [issue],
+ author_name: commit_author.name,
+ author_email: commit_author.email,
+ committed_date: commit_time
+ )
+
+ allow(project.repository).to receive(:commits_between).and_return([commit])
+ end
+
+ context "while saving the 'first_mentioned_in_commit_at' metric for an issue" do
+ it 'sets the metric for referenced issues' do
+ execute_service(project, user, @oldrev, @newrev, @ref)
+
+ expect(issue.metrics.first_mentioned_in_commit_at).to eq(commit_time)
+ end
+
+ it 'does not set the metric for non-referenced issues' do
+ non_referenced_issue = create(:issue, project: project)
+ execute_service(project, user, @oldrev, @newrev, @ref)
+
+ expect(non_referenced_issue.metrics.first_mentioned_in_commit_at).to be_nil
+ end
+ end
+ end
+
describe "closing issues from pushed commits containing a closing reference" do
let(:issue) { create :issue, project: project }
let(:other_issue) { create :issue, project: project }
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index c1e4f8bd96b..a0614abcf62 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -83,5 +83,33 @@ describe MergeRequests::CreateService, services: true do
}
end
end
+
+ context 'while saving references to issues that the created merge request closes' do
+ let(:first_issue) { create(:issue, project: project) }
+ let(:second_issue) { create(:issue, project: project) }
+
+ let(:opts) do
+ {
+ title: 'Awesome merge_request',
+ source_branch: 'feature',
+ target_branch: 'master',
+ force_remove_source_branch: '1'
+ }
+ end
+
+ before do
+ project.team << [user, :master]
+ project.team << [assignee, :developer]
+ end
+
+ it 'creates a `MergeRequestsClosingIssues` record for each issue' do
+ issue_closing_opts = opts.merge(description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}")
+ service = described_class.new(project, user, issue_closing_opts)
+ allow(service).to receive(:execute_hooks)
+ merge_request = service.execute
+
+ expect(merge_request.issues_closed).to match_array([first_issue, second_issue])
+ end
+ end
end
end
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index fff86480c6d..7ecad8d6bad 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -174,6 +174,36 @@ describe MergeRequests::RefreshService, services: true do
end
end
+ context 'push commits closing issues' do
+ let(:issue) { create :issue, project: @project }
+ let(:commit_author) { create :user }
+ let(:commit) { project.commit }
+ let!(:merge_request) { create(:merge_request, target_branch: 'master', source_branch: 'feature', source_project: @project) }
+
+ before do
+ project.team << [commit_author, :developer]
+ project.team << [user, :developer]
+
+ allow(commit).to receive_messages(
+ safe_message: "Closes #{issue.to_reference}",
+ references: [issue],
+ author_name: commit_author.name,
+ author_email: commit_author.email,
+ committed_date: Time.now
+ )
+
+ allow_any_instance_of(MergeRequest).to receive(:commits).and_return([commit])
+ end
+
+ it 'creates a `MergeRequestsClosingIssues` record for each closed issue' do
+ refresh_service = service.new(@project, @user)
+ allow(refresh_service).to receive(:execute_hooks)
+ refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature')
+
+ expect(merge_request.reload.issues_closed).to eq([issue])
+ end
+ end
+
def reload_mrs
@merge_request.reload
@fork_merge_request.reload
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 6dfeb581975..cfd506707de 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -263,5 +263,39 @@ describe MergeRequests::UpdateService, services: true do
end
end
end
+
+ context 'while saving references to issues that the updated merge request closes' do
+ let(:first_issue) { create(:issue, project: project) }
+ let(:second_issue) { create(:issue, project: project) }
+
+ it 'creates a `MergeRequestsClosingIssues` record for each issue' do
+ issue_closing_opts = { description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}" }
+ service = described_class.new(project, user, issue_closing_opts)
+ allow(service).to receive(:execute_hooks)
+ service.execute(merge_request)
+
+ expect(merge_request.reload.issues_closed).to match_array([first_issue, second_issue])
+ end
+ end
+
+ it 'removes `MergeRequestsClosingIssues` records when issues are not closed anymore' do
+ opts = {
+ title: 'Awesome merge_request',
+ description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}",
+ source_branch: 'feature',
+ target_branch: 'master',
+ force_remove_source_branch: '1'
+ }
+
+ merge_request = MergeRequests::CreateService.new(project, user, opts).execute
+
+ expect(merge_request.issues_closed).to match_array([first_issue, second_issue])
+
+ service = described_class.new(project, user, description: "not closing any issues")
+ allow(service).to receive(:execute_hooks)
+ service.execute(merge_request.reload)
+
+ expect(merge_request.reload.issues_closed).to be_empty
+ end
end
end
--
GitLab