diff options
author | Luke Duncalfe <lduncalfe@gitlab.com> | 2019-02-06 15:33:11 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-02-06 15:33:11 +0300 |
commit | 2b7dd017af7de4d09ef3a1cd835e8d07c8800b6a (patch) | |
tree | 47615a573f6dc932353f0f6695cd4fcd050b1201 /spec | |
parent | 5bfa8e2f5e03849645570ba8c2dbfcc5c834f1b1 (diff) |
Allow custom squash commit messages
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 17 | ||||
-rw-r--r-- | spec/factories/commits.rb | 10 | ||||
-rw-r--r-- | spec/features/merge_requests/user_squashes_merge_request_spec.rb | 2 | ||||
-rw-r--r-- | spec/fixtures/api/schemas/entities/merge_request_widget.json | 6 | ||||
-rw-r--r-- | spec/javascripts/vue_mr_widget/mock_data.js | 4 | ||||
-rw-r--r-- | spec/models/commit_collection_spec.rb | 11 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 48 | ||||
-rw-r--r-- | spec/serializers/merge_request_widget_commit_entity_spec.rb | 21 | ||||
-rw-r--r-- | spec/serializers/merge_request_widget_entity_spec.rb | 22 | ||||
-rw-r--r-- | spec/services/merge_requests/merge_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/merge_requests/squash_service_spec.rb | 71 |
11 files changed, 177 insertions, 37 deletions
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index ca5ff9b1e3b..79f97aa4170 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -387,6 +387,23 @@ describe Projects::MergeRequestsController do end end + context 'when a squash commit message is passed' do + let(:message) { 'My custom squash commit message' } + + it 'passes the same message to SquashService' do + params = { squash: '1', squash_commit_message: message } + + expect_next_instance_of(MergeRequests::SquashService, project, user, params.merge(merge_request: merge_request)) do |squash_service| + expect(squash_service).to receive(:execute).and_return({ + status: :success, + squash_sha: SecureRandom.hex(20) + }) + end + + merge_with_sha(params) + end + end + context 'when the pipeline succeeds is passed' do let!(:head_pipeline) do create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request) diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb index 818f7b046f6..2bcc4b6cf52 100644 --- a/spec/factories/commits.rb +++ b/spec/factories/commits.rb @@ -16,14 +16,24 @@ FactoryBot.define do commit end + project + skip_create # Commits cannot be persisted + initialize_with do new(git_commit, project) end after(:build) do |commit, evaluator| allow(commit).to receive(:author).and_return(evaluator.author || build_stubbed(:author)) + allow(commit).to receive(:parent_ids).and_return([]) + end + + trait :merge_commit do + after(:build) do |commit| + allow(commit).to receive(:parent_ids).and_return(Array.new(2) { SecureRandom.hex(20) }) + end end trait :without_author do diff --git a/spec/features/merge_requests/user_squashes_merge_request_spec.rb b/spec/features/merge_requests/user_squashes_merge_request_spec.rb index 47f9f10815c..bf9c55cf22c 100644 --- a/spec/features/merge_requests/user_squashes_merge_request_spec.rb +++ b/spec/features/merge_requests/user_squashes_merge_request_spec.rb @@ -14,7 +14,7 @@ describe 'User squashes a merge request', :js do latest_master_commits = project.repository.commits_between(original_head.sha, 'master').map(&:raw) squash_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/), - message: "Csv\n", + message: a_string_starting_with(project.merge_requests.first.default_squash_commit_message), author_name: user.name, committer_name: user.name) diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json index 1bd39a46830..67c209f3fc3 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -44,7 +44,7 @@ "merge_user": { "type": ["object", "null"] }, "diff_head_sha": { "type": ["string", "null"] }, "diff_head_commit_short_id": { "type": ["string", "null"] }, - "merge_commit_message": { "type": ["string", "null"] }, + "default_merge_commit_message": { "type": ["string", "null"] }, "pipeline": { "type": ["object", "null"] }, "merge_pipeline": { "type": ["object", "null"] }, "work_in_progress": { "type": "boolean" }, @@ -102,7 +102,9 @@ "new_blob_path": { "type": ["string", "null"] }, "merge_check_path": { "type": "string" }, "ci_environments_status_path": { "type": "string" }, - "merge_commit_message_with_description": { "type": "string" }, + "default_merge_commit_message_with_description": { "type": "string" }, + "default_squash_commit_message": { "type": "string" }, + "commits_without_merge_commits": { "type": "array" }, "diverged_commits_count": { "type": "integer" }, "commit_change_content_path": { "type": "string" }, "merge_commit_path": { "type": ["string", "null"] }, diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js index 072e98fc0e8..75b197fb2ba 100644 --- a/spec/javascripts/vue_mr_widget/mock_data.js +++ b/spec/javascripts/vue_mr_widget/mock_data.js @@ -58,7 +58,7 @@ export default { merge_user: null, diff_head_sha: '104096c51715e12e7ae41f9333e9fa35b73f385d', diff_head_commit_short_id: '104096c5', - merge_commit_message: + default_merge_commit_message: "Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22", pipeline: { id: 172, @@ -213,7 +213,7 @@ export default { merge_check_path: '/root/acets-app/merge_requests/22/merge_check', ci_environments_status_url: '/root/acets-app/merge_requests/22/ci_environments_status', project_archived: false, - merge_commit_message_with_description: + default_merge_commit_message_with_description: "Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22", diverged_commits_count: 0, only_allow_merge_if_pipeline_succeeds: false, diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb index 005005b236b..12e59b35428 100644 --- a/spec/models/commit_collection_spec.rb +++ b/spec/models/commit_collection_spec.rb @@ -35,6 +35,17 @@ describe CommitCollection do end end + describe '#without_merge_commits' do + it 'returns all commits except merge commits' do + collection = described_class.new(project, [ + build(:commit), + build(:commit, :merge_commit) + ]) + + expect(collection.without_merge_commits.size).to eq(1) + end + end + describe '#with_pipeline_status' do it 'sets the pipeline status for every commit so no additional queries are necessary' do create( diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b62f973ad1e..afa87b8a62d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -82,6 +82,38 @@ describe MergeRequest do end end + describe '#default_squash_commit_message' do + let(:project) { subject.project } + + def commit_collection(commit_hashes) + raw_commits = commit_hashes.map { |raw| Commit.from_hash(raw, project) } + + CommitCollection.new(project, raw_commits) + end + + it 'returns the oldest multiline commit message' do + commits = commit_collection([ + { message: 'Singleline', parent_ids: [] }, + { message: "Second multiline\nCommit message", parent_ids: [] }, + { message: "First multiline\nCommit message", parent_ids: [] } + ]) + + expect(subject).to receive(:commits).and_return(commits) + + expect(subject.default_squash_commit_message).to eq("First multiline\nCommit message") + end + + it 'returns the merge request title if there are no multiline commits' do + commits = commit_collection([ + { message: 'Singleline', parent_ids: [] } + ]) + + expect(subject).to receive(:commits).and_return(commits) + + expect(subject.default_squash_commit_message).to eq(subject.title) + end + end + describe 'modules' do subject { described_class } @@ -920,18 +952,18 @@ describe MergeRequest do end end - describe '#merge_commit_message' do + describe '#default_merge_commit_message' do it 'includes merge information as the title' do request = build(:merge_request, source_branch: 'source', target_branch: 'target') - expect(request.merge_commit_message) + expect(request.default_merge_commit_message) .to match("Merge branch 'source' into 'target'\n\n") end it 'includes its title in the body' do request = build(:merge_request, title: 'Remove all technical debt') - expect(request.merge_commit_message) + expect(request.default_merge_commit_message) .to match("Remove all technical debt\n\n") end @@ -943,34 +975,34 @@ describe MergeRequest do allow(subject.project).to receive(:default_branch).and_return(subject.target_branch) subject.cache_merge_request_closes_issues! - expect(subject.merge_commit_message) + expect(subject.default_merge_commit_message) .to match("Closes #{issue.to_reference}") end it 'includes its reference in the body' do request = build_stubbed(:merge_request) - expect(request.merge_commit_message) + expect(request.default_merge_commit_message) .to match("See merge request #{request.to_reference(full: true)}") end it 'excludes multiple linebreak runs when description is blank' do request = build(:merge_request, title: 'Title', description: nil) - expect(request.merge_commit_message).not_to match("Title\n\n\n\n") + expect(request.default_merge_commit_message).not_to match("Title\n\n\n\n") end it 'includes its description in the body' do request = build(:merge_request, description: 'By removing all code') - expect(request.merge_commit_message(include_description: true)) + expect(request.default_merge_commit_message(include_description: true)) .to match("By removing all code\n\n") end it 'does not includes its description in the body' do request = build(:merge_request, description: 'By removing all code') - expect(request.merge_commit_message) + expect(request.default_merge_commit_message) .not_to match("By removing all code\n\n") end end diff --git a/spec/serializers/merge_request_widget_commit_entity_spec.rb b/spec/serializers/merge_request_widget_commit_entity_spec.rb new file mode 100644 index 00000000000..ce83978c49a --- /dev/null +++ b/spec/serializers/merge_request_widget_commit_entity_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequestWidgetCommitEntity do + let(:project) { create(:project, :repository) } + let(:commit) { project.commit } + let(:request) { double('request') } + + let(:entity) do + described_class.new(commit, request: request) + end + + context 'as json' do + subject { entity.as_json } + + it { expect(subject[:message]).to eq(commit.safe_message) } + it { expect(subject[:short_id]).to eq(commit.short_id) } + it { expect(subject[:title]).to eq(commit.title) } + end +end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index 376698a16df..4dbd79f2fc0 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -188,9 +188,14 @@ describe MergeRequestWidgetEntity do .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.diff") end - it 'has merge_commit_message_with_description' do - expect(subject[:merge_commit_message_with_description]) - .to eq(resource.merge_commit_message(include_description: true)) + it 'has default_merge_commit_message_with_description' do + expect(subject[:default_merge_commit_message_with_description]) + .to eq(resource.default_merge_commit_message(include_description: true)) + end + + it 'has default_squash_commit_message' do + expect(subject[:default_squash_commit_message]) + .to eq(resource.default_squash_commit_message) end describe 'new_blob_path' do @@ -272,4 +277,15 @@ describe MergeRequestWidgetEntity do expect(entity[:rebase_path]).to be_nil end end + + describe 'commits_without_merge_commits' do + it 'should not include merge commits' do + # Mock all but the first 5 commits to be merge commits + resource.commits.each_with_index do |commit, i| + expect(commit).to receive(:merge_commit?).at_least(:once).and_return(i > 4) + end + + expect(subject[:commits_without_merge_commits].size).to eq(5) + end + end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 5d96b5ce27c..04a62aa454d 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -258,7 +258,7 @@ describe MergeRequests::MergeService do it 'logs and saves error if there is an error when squashing' do error_message = 'Failed to squash. Should be done manually' - allow_any_instance_of(MergeRequests::SquashService).to receive(:squash).and_return(nil) + allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil) merge_request.update(squash: true) service.execute(merge_request) diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 53bce15735c..2713652873e 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe MergeRequests::SquashService do include GitHelpers - let(:service) { described_class.new(project, user, {}) } + let(:service) { described_class.new(project, user, { merge_request: merge_request }) } let(:user) { project.owner } let(:project) { create(:project, :repository) } let(:repository) { project.repository.raw } @@ -31,32 +31,49 @@ describe MergeRequests::SquashService do shared_examples 'the squash succeeds' do it 'returns the squashed commit SHA' do - result = service.execute(merge_request) + result = service.execute expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/)) expect(result[:squash_sha]).not_to eq(merge_request.diff_head_sha) end it 'cleans up the temporary directory' do - service.execute(merge_request) + service.execute expect(File.exist?(squash_dir_path)).to be(false) end it 'does not keep the branch push event' do - expect { service.execute(merge_request) }.not_to change { Event.count } + expect { service.execute }.not_to change { Event.count } + end + + context 'when there is a single commit in the merge request' do + before do + expect(merge_request).to receive(:commits_count).at_least(:once).and_return(1) + end + + it 'will skip performing the squash, as the outcome would be the same' do + expect(merge_request.target_project.repository).not_to receive(:squash) + + service.execute + end + + it 'will still perform the squash when a custom squash commit message has been provided' do + service = described_class.new(project, user, { merge_request: merge_request, squash_commit_message: 'A custom commit message' }) + + expect(merge_request.target_project.repository).to receive(:squash).and_return('sha') + + service.execute + end end context 'the squashed commit' do - let(:squash_sha) { service.execute(merge_request)[:squash_sha] } + let(:squash_sha) { service.execute[:squash_sha] } let(:squash_commit) { project.repository.commit(squash_sha) } - it 'copies the author info and message from the merge request' do + it 'copies the author info from the merge request' do expect(squash_commit.author_name).to eq(merge_request.author.name) expect(squash_commit.author_email).to eq(merge_request.author.email) - - # Commit messages have a trailing newline, but titles don't. - expect(squash_commit.message.chomp).to eq(merge_request.title) end it 'sets the current user as the committer' do @@ -72,21 +89,37 @@ describe MergeRequests::SquashService do expect(squash_diff.patch.length).to eq(mr_diff.patch.length) expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha) end + + it 'has a default squash commit message if no message was provided' do + expect(squash_commit.message.chomp).to eq(merge_request.default_squash_commit_message.chomp) + end + + context 'if a message was provided' do + let(:service) { described_class.new(project, user, { merge_request: merge_request, squash_commit_message: message }) } + let(:message) { 'My custom message' } + let(:squash_sha) { service.execute[:squash_sha] } + + it 'has the same message as the message provided' do + expect(squash_commit.message.chomp).to eq(message) + end + end end end describe '#execute' do context 'when there is only one commit in the merge request' do + let(:merge_request) { merge_request_with_one_commit } + it 'returns that commit SHA' do - result = service.execute(merge_request_with_one_commit) + result = service.execute - expect(result).to match(status: :success, squash_sha: merge_request_with_one_commit.diff_head_sha) + expect(result).to match(status: :success, squash_sha: merge_request.diff_head_sha) end it 'does not perform any git actions' do expect(repository).not_to receive(:popen) - service.execute(merge_request_with_one_commit) + service.execute end end @@ -116,12 +149,11 @@ describe MergeRequests::SquashService do expect(service).to receive(:log_error).with(log_error) expect(service).to receive(:log_error).with(error) - service.execute(merge_request) + service.execute end it 'returns an error' do - expect(service.execute(merge_request)).to match(status: :error, - message: a_string_including('squash')) + expect(service.execute).to match(status: :error, message: a_string_including('squash')) end end end @@ -131,23 +163,22 @@ describe MergeRequests::SquashService do let(:error) { 'A test error' } before do - allow(merge_request).to receive(:commits_count).and_raise(error) + allow(merge_request.target_project.repository).to receive(:squash).and_raise(error) end it 'logs the MR reference and exception' do expect(service).to receive(:log_error).with(a_string_including("#{project.full_path}#{merge_request.to_reference}")) expect(service).to receive(:log_error).with(error) - service.execute(merge_request) + service.execute end it 'returns an error' do - expect(service.execute(merge_request)).to match(status: :error, - message: a_string_including('squash')) + expect(service.execute).to match(status: :error, message: a_string_including('squash')) end it 'cleans up the temporary directory' do - service.execute(merge_request) + service.execute expect(File.exist?(squash_dir_path)).to be(false) end |