Extend merge request tests for all commits method

2 files changed, 45 insertions, 20 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 88e46ecdc8b..ced0c13b837 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -788,8 +788,8 @@ class MergeRequest < ActiveRecord::Base
     return unless source_project
 
     @all_pipelines ||= source_project.pipelines
-        .where(sha: all_commits_sha, ref: source_branch)
-        .order(id: :desc)
+      .where(sha: all_commits_sha, ref: source_branch)
+      .order(id: :desc)
   end
 
   # Note that this could also return SHA from now dangling commits
@@ -798,7 +798,8 @@ class MergeRequest < ActiveRecord::Base
     if persisted?
       merge_request_diffs.flat_map(&:commits_sha).uniq
     else
-      compare_commits.reverse.map(&:id)
+      cached_commits = compare_commits.to_a.reverse.map(&:id)
+      cached_commits.any? ? cached_commits : [diff_head_sha]
     end
   end
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 91a423b670c..344f69a703e 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -640,32 +640,56 @@ describe MergeRequest, models: true do
   end
 
   describe '#all_commits_sha' do
-    let(:all_commits_sha) do
-      subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq
-    end
+    context 'when merge request is persisted' do
+      let(:all_commits_sha) do
+        subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq
+      end
 
-    shared_examples 'returning all SHA' do
-      it 'returns all SHA from all merge_request_diffs' do
-        expect(subject.merge_request_diffs.size).to eq(2)
-        expect(subject.all_commits_sha).to eq(all_commits_sha)
+      shared_examples 'returning all SHA' do
+        it 'returns all SHA from all merge_request_diffs' do
+          expect(subject.merge_request_diffs.size).to eq(2)
+          expect(subject.all_commits_sha).to eq(all_commits_sha)
+        end
       end
-    end
 
-    context 'with a completely different branch' do
-      before do
-        subject.update(target_branch: 'v1.0.0')
+      context 'with a completely different branch' do
+        before do
+          subject.update(target_branch: 'v1.0.0')
+        end
+
+        it_behaves_like 'returning all SHA'
       end
 
-      it_behaves_like 'returning all SHA'
+      context 'with a branch having no difference' do
+        before do
+          subject.update(target_branch: 'v1.1.0')
+          subject.reload # make sure commits were not cached
+        end
+
+        it_behaves_like 'returning all SHA'
+      end
     end
 
-    context 'with a branch having no difference' do
-      before do
-        subject.update(target_branch: 'v1.1.0')
-        subject.reload # make sure commits were not cached
+    context 'when merge request is not persisted' do
+      context 'when compare commits are set in the service' do
+        let(:commit) { spy('commit') }
+
+        subject do
+          build(:merge_request, compare_commits: [commit, commit])
+        end
+
+        it 'returns commits from compare commits temporary data' do
+          expect(subject.all_commits_sha).to eq [commit, commit]
+        end
       end
 
-      it_behaves_like 'returning all SHA'
+      context 'when compare commits are not set in the service' do
+        subject { build(:merge_request) }
+
+        it 'returns array with diff head sha element only' do
+          expect(subject.all_commits_sha).to eq [subject.diff_head_sha]
+        end
+      end
     end
   end