Verified Commit 6a9f9f0d authored by Jesse Hall's avatar Jesse Hall

Code Review Changes

parent 2ef56a0c
Pipeline #129816059 canceled with stages
in 30 seconds
......@@ -34,8 +34,8 @@ module Gitlab
@blob ||= first_suggestion.diff_file.new_blob
end
def first_file_path
@first_file_path ||= _first_file_path
def file_path
@file_path ||= _file_path
end
private
......@@ -65,7 +65,7 @@ module Gitlab
end
def for_different_file?(suggestion)
first_file_path && first_file_path != suggestion_file_path(suggestion)
file_path && file_path != suggestion_file_path(suggestion)
end
def suggestion_file_path(suggestion)
......@@ -76,7 +76,7 @@ module Gitlab
suggestions.first
end
def _first_file_path
def _file_path
if suggestions.present?
suggestion_file_path(first_suggestion)
else
......
......@@ -71,7 +71,7 @@ module Gitlab
end
def first_file_path
first_file_suggestion.first_file_path
first_file_suggestion.file_path
end
def last_commit_id
......
......@@ -3,7 +3,7 @@
require 'spec_helper'
describe Gitlab::Suggestions::CommitMessage do
describe '#paths' do
describe '.paths' do
it 'returns the unique paths of the files the suggestions refer to' do
file_paths = %w(path/to/B path/to/B path/to/A)
......@@ -18,7 +18,7 @@ describe Gitlab::Suggestions::CommitMessage do
end
end
describe '#format_paths' do
describe '.format_paths' do
it 'returns alphabetically sorted paths, separated by commas and whitespace' do
paths = %w(path/to/B path/to/C path/to/A)
......
......@@ -3,97 +3,240 @@
require 'spec_helper'
describe Gitlab::Suggestions::FileSuggestion do
describe '#add_suggestion' do
def create_fake_suggestions(paths)
fake_diff_files = paths.map do |path|
double("DiffFile", file_path: path)
end
def create_suggestion(new_line, to_content)
position = Gitlab::Diff::Position.new(old_path: file_path,
new_path: file_path,
old_line: nil,
new_line: new_line,
diff_refs: merge_request.diff_refs)
diff_note = create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
create(:suggestion, note: diff_note, to_content: to_content)
end
fake_diff_files.map do |diff_file|
double("Suggestion", diff_file: diff_file)
end
end
let_it_be(:user) { create(:user) }
let_it_be(:file_path) { 'files/ruby/popen.rb'}
let(:fake_suggestions) { create_fake_suggestions(paths) }
let_it_be(:project) { create(:project, :repository) }
let(:fake_suggestion1) { fake_suggestions[0] }
let(:fake_suggestion2) { fake_suggestions[1] }
let_it_be(:merge_request) do
create(:merge_request, source_project: project, target_project: project)
end
let(:file_suggestion) { described_class.new }
let_it_be(:suggestion1) do
create_suggestion(9, " raise RuntimeError, 'Explosion'\n # explosion?\n")
end
before do
file_suggestion.add_suggestion(fake_suggestion1)
end
let_it_be(:suggestion2) do
create_suggestion(15, " *** SUGGESTION CHANGE ***\n")
end
context 'when adding a suggestion for the same file as the original' do
let(:paths) { %w(path/to/A path/to/A) }
let(:file_suggestion) { described_class.new }
it 'succeeds ' do
expect { file_suggestion.add_suggestion(fake_suggestion2) }
.not_to raise_error
end
describe '#add_suggestion' do
it 'succeeds when adding a suggestion for the same file as the original' do
file_suggestion.add_suggestion(suggestion1)
expect { file_suggestion.add_suggestion(suggestion2) }.not_to raise_error
end
context 'when adding a suggestion for a file different from the original' do
let(:paths) { %w(path/to/A path/to/B) }
it 'raises an error when adding a suggestion for a different file' do
allow(suggestion2)
.to(receive_message_chain(:diff_file, :file_path)
.and_return('path/to/different/file'))
it 'raises an error ' do
expect { file_suggestion.add_suggestion(fake_suggestion2) }.to(
raise_error(described_class::SuggestionForDifferentFileError)
)
end
file_suggestion.add_suggestion(suggestion1)
expect { file_suggestion.add_suggestion(suggestion2) }.to(
raise_error(described_class::SuggestionForDifferentFileError)
)
end
end
describe '#line_conflict' do
def stub_suggestions(index_tuples)
fake_suggestions = index_tuples.map do |tuple|
double("Suggestion", from_line_index: tuple[0], to_line_index: tuple[1])
def stub_suggestions(line_index_spans)
fake_suggestions = line_index_spans.map do |span|
double("Suggestion",
from_line_index: span[:from_line_index],
to_line_index: span[:to_line_index])
end
allow(file_suggestion).to(receive(:suggestions).and_return(fake_suggestions))
end
let(:file_suggestion) { described_class.new }
context 'when line ranges do not overlap' do
it 'return false' do
stub_suggestions([[0, 10], [11, 20]])
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 11,
to_line_index: 20
}
]
)
expect(file_suggestion.line_conflict?).to eq(false)
expect(file_suggestion.line_conflict?).to be(false)
end
end
context 'when line ranges are identical' do
it 'returns true' do
stub_suggestions([[0, 10], [0, 10]])
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 0,
to_line_index: 10
}
]
)
expect(file_suggestion.line_conflict?).to eq(true)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when one range starts, and the other ends, on the same line' do
it 'returns true' do
stub_suggestions([[0, 10], [10, 20]])
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 10,
to_line_index: 20
}
]
)
expect(file_suggestion.line_conflict?).to eq(true)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when one line range contains the other' do
it 'returns true' do
stub_suggestions([[0, 10], [5, 7]])
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 5,
to_line_index: 7
}
]
)
expect(file_suggestion.line_conflict?).to eq(true)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when line ranges overlap' do
it 'returns true' do
stub_suggestions([[0, 10], [8, 15]])
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 8,
to_line_index: 15
}
]
)
expect(file_suggestion.line_conflict?).to eq(true)
expect(file_suggestion.line_conflict?).to be(true)
end
end
end
describe '#new_content' do
it 'returns a blob with the suggestions applied to it' do
file_suggestion.add_suggestion(suggestion1)
file_suggestion.add_suggestion(suggestion2)
expected_content = <<-CONTENT.strip_heredoc
require 'fileutils'
require 'open3'
module Popen
extend self
def popen(cmd, path=nil)
unless cmd.is_a?(Array)
raise RuntimeError, 'Explosion'
# explosion?
end
path ||= Dir.pwd
vars = {
*** SUGGESTION CHANGE ***
}
options = {
chdir: path
}
unless File.directory?(path)
FileUtils.mkdir_p(path)
end
@cmd_output = ""
@cmd_status = 0
Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
@cmd_output << stdout.read
@cmd_output << stderr.read
@cmd_status = wait_thr.value.exitstatus
end
return @cmd_output, @cmd_status
end
end
CONTENT
expect(file_suggestion.new_content).to eq(expected_content)
end
end
describe '#blob' do
it 'returns a copy of the blob associated with the suggestions' do
file_suggestion.add_suggestion(suggestion1)
expected_blob_data = suggestion1
.project
.repository
.blob_at('master', 'files/ruby/popen.rb').data
expect(file_suggestion.blob.data).to eq(expected_blob_data)
end
end
describe '#file_path' do
it 'returns the path of the file associated with the suggestions' do
file_suggestion.add_suggestion(suggestion1)
expect(file_suggestion.file_path).to eq(file_path)
end
it 'returns nil if no file path exists' do
expect(file_suggestion.file_path).to be(nil)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Suggestions::SuggestionSet do
let_it_be(:suggestion) { create(:suggestion) }
let(:suggestion_set) { described_class.new([suggestion]) }
describe '#project' do
it 'returns the project associated with the suggestions' do
expected_project = suggestion.project
expect(suggestion_set.project).to be(expected_project)
end
end
describe '#branch' do
it 'returns the branch associated with the suggestions' do
expected_branch = suggestion.branch
expect(suggestion_set.branch).to be(expected_branch)
end
end
describe '#valid?' do
it 'returns true if no errors are found' do
expect(suggestion_set.valid?).to be(true)
end
it 'returns false if an error is found' do
allow(suggestion).to(receive(:diff_file).and_return(nil))
expect(suggestion_set.valid?).to be(false)
end
end
describe '#error_message' do
it 'returns an error message if an error is found' do
allow(suggestion).to(receive(:diff_file).and_return(nil))
expect(suggestion_set.error_message).to be_a(String)
end
it 'returns nil if no errors are found' do
expect(suggestion_set.error_message).to be(nil)
end
end
describe '#actions' do
it 'returns an array of hashes with proper key/value pairs' do
first_action = suggestion_set.actions.first
file_path, file_suggestion = suggestion_set
.send(:suggestions_per_file).first
last_commit_id = suggestion_set.send(:last_commit_id)
expect(first_action[:action]).to be('update')
expect(first_action[:file_path]).to eq(file_path)
expect(first_action[:content]).to eq(file_suggestion.new_content)
expect(first_action[:last_commit_id]).to be(last_commit_id)
end
end
end
......@@ -52,13 +52,11 @@ describe Suggestions::ApplyService do
it 'updates suggestion applied and commit_id columns' do
expect(suggestions.map(&:applied)).to all(be false)
expect(suggestions.map(&:commit_id)).to all(be nil)
apply(suggestions)
expect(suggestions.map(&:applied)).to all(be true)
expect(suggestions.map(&:commit_id)).to all(be_present)
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment