diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-09-19 10:26:25 +0300 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-09-19 10:42:06 +0300 |
commit | 8f6208513a98b33d7edd6ecf1ae6062f266c279f (patch) | |
tree | 4bc003a3e775ab02030a095dac835a84c73b8081 | |
parent | b9bf23e07cf886825fc9b0038caae4bfd78d3c05 (diff) |
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"
-rw-r--r-- | app/models/ci/pipeline.rb | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 1 | ||||
-rw-r--r-- | app/services/create_deployment_service.rb | 25 | ||||
-rw-r--r-- | app/services/git_push_service.rb | 1 | ||||
-rw-r--r-- | spec/models/ci/pipeline_spec.rb | 55 | ||||
-rw-r--r-- | spec/models/issue/metrics_spec.rb | 55 | ||||
-rw-r--r-- | spec/models/merge_request/metrics_spec.rb | 18 | ||||
-rw-r--r-- | spec/services/create_deployment_service_spec.rb | 76 | ||||
-rw-r--r-- | spec/services/git_push_service_spec.rb | 37 | ||||
-rw-r--r-- | spec/services/merge_requests/create_service_spec.rb | 28 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 30 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 34 |
12 files changed, 352 insertions, 10 deletions
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 |