summaryrefslogtreecommitdiff
path: root/spec/services/suggestions
diff options
context:
space:
mode:
authorOswaldo Ferreira <oswaldo@gitlab.com>2019-03-15 14:35:34 -0300
committerOswaldo Ferreira <oswaldo@gitlab.com>2019-03-27 12:26:53 -0300
commit03e0604d5ded6402c7fddc4001ab23d9712c98de (patch)
tree33cf57e25d71e9a9c0ab39149cbcb285f0f05a31 /spec/services/suggestions
parent1db3926dd2a5de719859ea962d4e1360b375d87b (diff)
downloadgitlab-ce-03e0604d5ded6402c7fddc4001ab23d9712c98de.tar.gz
Prepare suggestion implementation for multi-line
Adds the groundwork needed in order to persist multi-line suggestions, while providing the parsing strategy which will be reused for the **Preview** as well.
Diffstat (limited to 'spec/services/suggestions')
-rw-r--r--spec/services/suggestions/apply_service_spec.rb93
-rw-r--r--spec/services/suggestions/create_service_spec.rb73
-rw-r--r--spec/services/suggestions/outdate_service_spec.rb102
3 files changed, 198 insertions, 70 deletions
diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb
index fe85b5c9065..80b5dcac6c7 100644
--- a/spec/services/suggestions/apply_service_spec.rb
+++ b/spec/services/suggestions/apply_service_spec.rb
@@ -5,6 +5,41 @@ require 'spec_helper'
describe Suggestions::ApplyService do
include ProjectForksHelper
+ shared_examples 'successfully creates commit and updates suggestion' do
+ def apply(suggestion)
+ result = subject.execute(suggestion)
+ expect(result[:status]).to eq(:success)
+ end
+
+ it 'updates the file with the new contents' do
+ apply(suggestion)
+
+ blob = project.repository.blob_at_branch(merge_request.source_branch,
+ position.new_path)
+
+ expect(blob.data).to eq(expected_content)
+ end
+
+ it 'updates suggestion applied and commit_id columns' do
+ expect { apply(suggestion) }
+ .to change(suggestion, :applied)
+ .from(false).to(true)
+ .and change(suggestion, :commit_id)
+ .from(nil)
+ end
+
+ it 'created commit has users email and name' do
+ apply(suggestion)
+
+ commit = project.repository.commit
+
+ expect(user.commit_email).not_to eq(user.email)
+ expect(commit.author_email).to eq(user.commit_email)
+ expect(commit.committer_email).to eq(user.commit_email)
+ expect(commit.author_name).to eq(user.name)
+ end
+ end
+
let(:project) { create(:project, :repository) }
let(:user) { create(:user, :commit_email) }
@@ -17,9 +52,8 @@ describe Suggestions::ApplyService do
end
let(:suggestion) do
- create(:suggestion, note: diff_note,
- from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n",
- to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
+ create(:suggestion, :content_from_repo, note: diff_note,
+ to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
end
subject { described_class.new(user) }
@@ -84,39 +118,7 @@ describe Suggestions::ApplyService do
project.add_maintainer(user)
end
- it 'updates the file with the new contents' do
- subject.execute(suggestion)
-
- blob = project.repository.blob_at_branch(merge_request.source_branch,
- position.new_path)
-
- expect(blob.data).to eq(expected_content)
- end
-
- it 'returns success status' do
- result = subject.execute(suggestion)
-
- expect(result[:status]).to eq(:success)
- end
-
- it 'updates suggestion applied and commit_id columns' do
- expect { subject.execute(suggestion) }
- .to change(suggestion, :applied)
- .from(false).to(true)
- .and change(suggestion, :commit_id)
- .from(nil)
- end
-
- it 'created commit has users email and name' do
- subject.execute(suggestion)
-
- commit = project.repository.commit
-
- expect(user.commit_email).not_to eq(user.email)
- expect(commit.author_email).to eq(user.commit_email)
- expect(commit.committer_email).to eq(user.commit_email)
- expect(commit.author_name).to eq(user.name)
- end
+ it_behaves_like 'successfully creates commit and updates suggestion'
context 'when it fails to apply because the file was changed' do
it 'returns error message' do
@@ -212,11 +214,13 @@ describe Suggestions::ApplyService do
end
def apply_suggestion(suggestion)
- suggestion.note.reload
+ suggestion.reload
merge_request.reload
merge_request.clear_memoized_shas
result = subject.execute(suggestion)
+ expect(result[:status]).to eq(:success)
+
refresh = MergeRequests::RefreshService.new(project, user)
refresh.execute(merge_request.diff_head_sha,
suggestion.commit_id,
@@ -241,7 +245,7 @@ describe Suggestions::ApplyService do
suggestion_2_changes = { old_line: 24,
new_line: 31,
- from_content: " @cmd_output << stderr.read\n",
+ from_content: " @cmd_output << stderr.read\n",
to_content: "# v2 change\n",
path: path }
@@ -368,7 +372,18 @@ describe Suggestions::ApplyService do
result = subject.execute(suggestion)
- expect(result).to eq(message: 'The file was not found',
+ expect(result).to eq(message: 'Suggestion is not appliable',
+ status: :error)
+ end
+ end
+
+ context 'suggestion is eligible to be outdated' do
+ it 'returns error message' do
+ expect(suggestion).to receive(:outdated?) { true }
+
+ result = subject.execute(suggestion)
+
+ expect(result).to eq(message: 'Suggestion is not appliable',
status: :error)
end
end
diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb
index 1b4b15b8eaa..ce4990a34a4 100644
--- a/spec/services/suggestions/create_service_spec.rb
+++ b/spec/services/suggestions/create_service_spec.rb
@@ -40,6 +40,14 @@ describe Suggestions::CreateService do
```thing
this is not a suggestion, it's a thing
```
+
+ ```suggestion:-3+2
+ # multi-line suggestion 1
+ ```
+
+ ```suggestion:-5
+ # multi-line suggestion 1
+ ```
MARKDOWN
end
@@ -54,7 +62,7 @@ describe Suggestions::CreateService do
end
it 'does not try to parse suggestions' do
- expect(Banzai::SuggestionsParser).not_to receive(:parse)
+ expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse)
subject.execute
end
@@ -71,7 +79,7 @@ describe Suggestions::CreateService do
it 'does not try to parse suggestions' do
allow(note).to receive(:on_text?) { false }
- expect(Banzai::SuggestionsParser).not_to receive(:parse)
+ expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse)
subject.execute
end
@@ -87,7 +95,9 @@ describe Suggestions::CreateService do
end
it 'creates no suggestion when diff file is not found' do
- expect(note).to receive(:latest_diff_file) { nil }
+ expect_next_instance_of(DiffNote) do |diff_note|
+ expect(diff_note).to receive(:latest_diff_file).twice { nil }
+ end
expect { subject.execute }.not_to change(Suggestion, :count)
end
@@ -101,43 +111,44 @@ describe Suggestions::CreateService do
note: markdown)
end
- context 'single line suggestions' do
- it 'persists suggestion records' do
- expect { subject.execute }
- .to change { note.suggestions.count }
- .from(0)
- .to(2)
- end
+ let(:expected_suggestions) do
+ Gitlab::Diff::SuggestionsParser.parse(markdown,
+ project: note.project,
+ position: note.position)
+ end
- it 'persists original from_content lines and suggested lines' do
- subject.execute
+ it 'persists suggestion records' do
+ expect { subject.execute }.to change { note.suggestions.count }
+ .from(0).to(expected_suggestions.size)
+ end
- suggestions = note.suggestions.order(:relative_order)
+ it 'persists suggestions data correctly' do
+ subject.execute
- suggestion_1 = suggestions.first
- suggestion_2 = suggestions.last
+ suggestions = note.suggestions.order(:relative_order)
- expect(suggestion_1).to have_attributes(from_content: " vars = {\n",
- to_content: " foo\n bar\n")
+ suggestions.zip(expected_suggestions) do |suggestion, expected_suggestion|
+ expected_data = expected_suggestion.to_hash
- expect(suggestion_2).to have_attributes(from_content: " vars = {\n",
- to_content: " xpto\n baz\n")
+ expect(suggestion.from_content).to eq(expected_data[:from_content])
+ expect(suggestion.to_content).to eq(expected_data[:to_content])
+ expect(suggestion.lines_above).to eq(expected_data[:lines_above])
+ expect(suggestion.lines_below).to eq(expected_data[:lines_below])
end
+ end
- context 'outdated position note' do
- let!(:outdated_diff) { merge_request.merge_request_diff }
- let!(:latest_diff) { merge_request.create_merge_request_diff }
- let(:outdated_position) { build_position(diff_refs: outdated_diff.diff_refs) }
- let(:position) { build_position(diff_refs: latest_diff.diff_refs) }
+ context 'outdated position note' do
+ let!(:outdated_diff) { merge_request.merge_request_diff }
+ let!(:latest_diff) { merge_request.create_merge_request_diff }
+ let(:outdated_position) { build_position(diff_refs: outdated_diff.diff_refs) }
+ let(:position) { build_position(diff_refs: latest_diff.diff_refs) }
- it 'uses the correct position when creating the suggestion' do
- expect(note.position)
- .to receive(:diff_file)
- .with(project_with_repo.repository)
- .and_call_original
+ it 'uses the correct position when creating the suggestion' do
+ expect(Gitlab::Diff::SuggestionsParser).to receive(:parse)
+ .with(note.note, project: note.project, position: note.position)
+ .and_call_original
- subject.execute
- end
+ subject.execute
end
end
end
diff --git a/spec/services/suggestions/outdate_service_spec.rb b/spec/services/suggestions/outdate_service_spec.rb
new file mode 100644
index 00000000000..bcc627013d8
--- /dev/null
+++ b/spec/services/suggestions/outdate_service_spec.rb
@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Suggestions::OutdateService do
+ describe '#execute' do
+ let(:merge_request) { create(:merge_request) }
+ let(:project) { merge_request.target_project }
+ let(:user) { merge_request.author }
+ let(:file_path) { 'files/ruby/popen.rb' }
+ let(:branch_name) { project.default_branch }
+ let(:diff_file) { suggestion.diff_file }
+ let(:position) { build_position(file_path, comment_line) }
+ let(:note) do
+ create(:diff_note_on_merge_request, noteable: merge_request,
+ position: position,
+ project: project)
+ end
+
+ def build_position(path, line)
+ Gitlab::Diff::Position.new(old_path: path,
+ new_path: path,
+ old_line: nil,
+ new_line: line,
+ diff_refs: merge_request.diff_refs)
+ end
+
+ def commit_changes(file_path, new_content)
+ params = {
+ file_path: file_path,
+ commit_message: "Update File",
+ file_content: new_content,
+ start_project: project,
+ start_branch: project.default_branch,
+ branch_name: branch_name
+ }
+
+ Files::UpdateService.new(project, user, params).execute
+ end
+
+ def update_file_line(diff_file, change_line, content)
+ new_lines = diff_file.new_blob.data.lines
+ new_lines[change_line..change_line] = content
+ result = commit_changes(diff_file.file_path, new_lines.join)
+ newrev = result[:result]
+
+ expect(result[:status]).to eq(:success)
+ expect(newrev).to be_present
+
+ # Ensure all memoized data is cleared in order
+ # to generate the new merge_request_diff.
+ MergeRequest.find(merge_request.id).reload_diff(user)
+
+ note.reload
+ end
+
+ before do
+ project.add_maintainer(user)
+ end
+
+ subject { described_class.new.execute(merge_request) }
+
+ context 'when there is a change within multi-line suggestion range' do
+ let(:comment_line) { 9 }
+ let(:lines_above) { 8 } # suggesting to change lines 1..9
+ let(:change_line) { 2 } # line 2 is within the range
+ let!(:suggestion) do
+ create(:suggestion, :content_from_repo, note: note, lines_above: lines_above)
+ end
+
+ it 'updates the outdatable suggestion record' do
+ update_file_line(diff_file, change_line, "# foo\nbar\n")
+
+ # Make sure note is still active
+ expect(note.active?).to be(true)
+
+ expect { subject }.to change { suggestion.reload.outdated }
+ .from(false).to(true)
+ end
+ end
+
+ context 'when there is no change within multi-line suggestion range' do
+ let(:comment_line) { 9 }
+ let(:lines_above) { 3 } # suggesting to change lines 6..9
+ let(:change_line) { 2 } # line 2 is not within the range
+ let!(:suggestion) do
+ create(:suggestion, :content_from_repo, note: note, lines_above: lines_above)
+ end
+
+ subject { described_class.new.execute(merge_request) }
+
+ it 'does not outdates suggestion record' do
+ update_file_line(diff_file, change_line, "# foo\nbar\n")
+
+ # Make sure note is still active
+ expect(note.active?).to be(true)
+
+ expect { subject }.not_to change { suggestion.reload.outdated }.from(false)
+ end
+ end
+ end
+end