Commit b4c07c00 authored by Kerri Miller's avatar Kerri Miller

Extract fast_ and slow_path_lookup

Make explicit the expected performance difference between these two
different methods of getting at merge request path information, and
add/improve specs
parent 6d75b935
......@@ -42,13 +42,26 @@ module Gitlab
# subject to the same limit.
#
if merge_request.diff_size == "1000+"
merge_request.project.repository.diff_stats(
merge_request.commits.last.sha,
merge_request.commits.first.sha
).paths
slow_path_lookup(merge_request, merge_request_diff)
else
merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
fast_path_lookup(merge_request, merge_request_diff)
end
end
private_class_method :paths_for_merge_request
def self.slow_path_lookup(merge_request, merge_request_diff)
merge_request_diff = merge_request_diff || merge_request.merge_request_diff
merge_request.project.repository.diff_stats(
merge_request_diff.base_commit_sha,
merge_request_diff.head_commit_sha
).paths
end
private_class_method :slow_path_lookup
def self.fast_path_lookup(merge_request, merge_request_diff)
merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
end
private_class_method :fast_path_lookup
end
end
......@@ -44,6 +44,36 @@ describe Gitlab::CodeOwners do
end
end
describe ".fast_path_lookup and .slow_path_lookup" do
let(:codeowner_lookup_ref) { merge_request.target_branch }
let(:codeowner_content) { 'files/ruby/feature.rb @owner-1' }
let(:merge_request) do
create(
:merge_request,
source_project: project,
source_branch: 'feature',
target_project: project,
target_branch: 'with-codeowners'
)
end
before do
stub_licensed_features(code_owners: true)
end
it "return equivalent results" do
fast_results = described_class.entries_for_merge_request(merge_request).first
expect(merge_request).to receive(:diff_size).and_return("1000+")
slow_results = described_class.entries_for_merge_request(merge_request).first
expect(slow_results.users).to eq(fast_results.users)
expect(slow_results.groups).to eq(fast_results.groups)
expect(slow_results.pattern).to eq(fast_results.pattern)
end
end
describe '.entries_for_merge_request' do
let(:codeowner_lookup_ref) { merge_request.target_branch }
let(:merge_request) do
......@@ -72,6 +102,7 @@ describe Gitlab::CodeOwners do
context 'when merge_request_diff is specified' do
it 'returns owners at the specified ref' do
expect(described_class).to receive(:fast_path_lookup).and_call_original
expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: merge_request.merge_request_diff).and_return(['docs/CODEOWNERS'])
entry = described_class.entries_for_merge_request(merge_request, merge_request_diff: merge_request.merge_request_diff).first
......@@ -85,9 +116,9 @@ describe Gitlab::CodeOwners do
expect(merge_request).to receive(:diff_size).and_return("1000+")
end
it 'generates paths via Repository#diff_stats' do
expect(merge_request).not_to receive(:modified_paths)
expect(merge_request.project.repository).to receive(:diff_stats).and_call_original
it 'generates paths via .slow_path_lookup' do
expect(described_class).not_to receive(:fast_path_lookup)
expect(described_class).to receive(:slow_path_lookup).and_call_original
described_class.entries_for_merge_request(merge_request)
end
......@@ -100,7 +131,7 @@ describe Gitlab::CodeOwners do
end
it 'skips reading codeowners and returns an empty array' do
expect(merge_request).not_to receive(:modified_paths)
expect(described_class).not_to receive(:loader_for_merge_request)
expect(described_class.entries_for_merge_request(merge_request)).to eq([])
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