diff options
Diffstat (limited to 'spec/lib/gitlab')
33 files changed, 1184 insertions, 78 deletions
diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb index 8f2434acd26..ff002acbd35 100644 --- a/spec/lib/gitlab/asciidoc_spec.rb +++ b/spec/lib/gitlab/asciidoc_spec.rb @@ -45,28 +45,117 @@ module Gitlab end context "XSS" do - links = { - 'links' => { + items = { + 'link with extra attribute' => { input: 'link:mylink"onmouseover="alert(1)[Click Here]', output: "<div>\n<p><a href=\"mylink\">Click Here</a></p>\n</div>" }, - 'images' => { + 'link with unsafe scheme' => { + input: 'link:data://danger[Click Here]', + output: "<div>\n<p><a>Click Here</a></p>\n</div>" + }, + 'image with onerror' => { input: 'image:https://localhost.com/image.png[Alt text" onerror="alert(7)]', output: "<div>\n<p><span><img src=\"https://localhost.com/image.png\" alt='Alt text\" onerror=\"alert(7)'></span></p>\n</div>" }, - 'pre' => { + 'fenced code with inline script' => { input: '```mypre"><script>alert(3)</script>', output: "<div>\n<div>\n<pre class=\"code highlight js-syntax-highlight plaintext\" lang=\"plaintext\" v-pre=\"true\"><code><span id=\"LC1\" class=\"line\" lang=\"plaintext\">\"></span></code></pre>\n</div>\n</div>" } } - links.each do |name, data| + items.each do |name, data| it "does not convert dangerous #{name} into HTML" do expect(render(data[:input], context)).to include(data[:output]) end end end + context 'with admonition' do + it 'preserves classes' do + input = <<~ADOC + NOTE: An admonition paragraph, like this note, grabs the reader’s attention. + ADOC + + output = <<~HTML + <div class="admonitionblock"> + <table> + <tr> + <td class="icon"> + <i class="fa icon-note" title="Note"></i> + </td> + <td> + An admonition paragraph, like this note, grabs the reader’s attention. + </td> + </tr> + </table> + </div> + HTML + + expect(render(input, context)).to include(output.strip) + end + end + + context 'with checklist' do + it 'preserves classes' do + input = <<~ADOC + * [x] checked + * [ ] not checked + ADOC + + output = <<~HTML + <div> + <ul class="checklist"> + <li> + <p><i class="fa fa-check-square-o"></i> checked</p> + </li> + <li> + <p><i class="fa fa-square-o"></i> not checked</p> + </li> + </ul> + </div> + HTML + + expect(render(input, context)).to include(output.strip) + end + end + + context 'with marks' do + it 'preserves classes' do + input = <<~ADOC + Werewolves are allergic to #cassia cinnamon#. + + Did the werewolves read the [.small]#small print#? + + Where did all the [.underline.small]#cores# run off to? + + We need [.line-through]#ten# make that twenty VMs. + + [.big]##O##nce upon an infinite loop. + ADOC + + output = <<~HTML + <div> + <p>Werewolves are allergic to <mark>cassia cinnamon</mark>.</p> + </div> + <div> + <p>Did the werewolves read the <span class="small">small print</span>?</p> + </div> + <div> + <p>Where did all the <span class="underline small">cores</span> run off to?</p> + </div> + <div> + <p>We need <span class="line-through">ten</span> make that twenty VMs.</p> + </div> + <div> + <p><span class="big">O</span>nce upon an infinite loop.</p> + </div> + HTML + + expect(render(input, context)).to include(output.strip) + end + end + context 'with fenced block' do it 'highlights syntax' do input = <<~ADOC diff --git a/spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb b/spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb new file mode 100644 index 00000000000..5938ecca459 --- /dev/null +++ b/spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::FixUserNamespaceNames, :migration, schema: 20190620112608 do + let(:namespaces) { table(:namespaces) } + let(:users) { table(:users) } + let(:user) { users.create(name: "The user's full name", projects_limit: 10, username: 'not-null', email: '1') } + + context 'updating the namespace names' do + it 'updates a user namespace within range' do + user2 = users.create(name: "Other user's full name", projects_limit: 10, username: 'also-not-null', email: '2') + user_namespace1 = namespaces.create( + id: 2, + owner_id: user.id, + name: "Should be the user's name", + path: user.username + ) + user_namespace2 = namespaces.create( + id: 3, + owner_id: user2.id, + name: "Should also be the user's name", + path: user.username + ) + + described_class.new.perform(1, 5) + + expect(user_namespace1.reload.name).to eq("The user's full name") + expect(user_namespace2.reload.name).to eq("Other user's full name") + end + + it 'does not update namespaces out of range' do + user_namespace = namespaces.create( + id: 6, + owner_id: user.id, + name: "Should be the user's name", + path: user.username + ) + + expect { described_class.new.perform(1, 5) } + .not_to change { user_namespace.reload.name } + end + + it 'does not update groups owned by the users' do + user_group = namespaces.create( + id: 2, + owner_id: user.id, + name: 'A group name', + path: 'the-path', + type: 'Group' + ) + + expect { described_class.new.perform(1, 5) } + .not_to change { user_group.reload.name } + end + end + + context 'namespace route names' do + let(:routes) { table(:routes) } + let(:namespace) do + namespaces.create( + id: 2, + owner_id: user.id, + name: "Will be updated to the user's name", + path: user.username + ) + end + + it "updates the route name if it didn't match the namespace" do + route = routes.create(path: namespace.path, name: 'Incorrect name', source_type: 'Namespace', source_id: namespace.id) + + described_class.new.perform(1, 5) + + expect(route.reload.name).to eq("The user's full name") + end + + it 'updates the route name if it was nil match the namespace' do + route = routes.create(path: namespace.path, name: nil, source_type: 'Namespace', source_id: namespace.id) + + described_class.new.perform(1, 5) + + expect(route.reload.name).to eq("The user's full name") + end + + it "doesn't update group routes" do + route = routes.create(path: 'group-path', name: 'Group name', source_type: 'Group', source_id: namespace.id) + + expect { described_class.new.perform(1, 5) } + .not_to change { route.reload.name } + end + + it "doesn't touch routes for namespaces out of range" do + user_namespace = namespaces.create( + id: 6, + owner_id: user.id, + name: "Should be the user's name", + path: user.username + ) + + expect { described_class.new.perform(1, 5) } + .not_to change { user_namespace.reload.name } + end + end +end diff --git a/spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb b/spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb new file mode 100644 index 00000000000..d1d6d8411d1 --- /dev/null +++ b/spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::FixUserProjectRouteNames, :migration, schema: 20190620112608 do + let(:namespaces) { table(:namespaces) } + let(:users) { table(:users) } + let(:routes) { table(:routes) } + let(:projects) { table(:projects) } + + let(:user) { users.create(name: "The user's full name", projects_limit: 10, username: 'not-null', email: '1') } + + let(:namespace) do + namespaces.create( + owner_id: user.id, + name: "Should eventually be the user's name", + path: user.username + ) + end + + let(:project) do + projects.create(namespace_id: namespace.id, name: 'Project Name') + end + + it "updates the route for a project if it did not match the user's name" do + route = routes.create( + id: 1, + path: "#{user.username}/#{project.path}", + source_id: project.id, + source_type: 'Project', + name: 'Completely wrong' + ) + + described_class.new.perform(1, 5) + + expect(route.reload.name).to eq("The user's full name / Project Name") + end + + it 'updates the route for a project if the name was nil' do + route = routes.create( + id: 1, + path: "#{user.username}/#{project.path}", + source_id: project.id, + source_type: 'Project', + name: nil + ) + + described_class.new.perform(1, 5) + + expect(route.reload.name).to eq("The user's full name / Project Name") + end + + it 'does not update routes that were are out of the range' do + route = routes.create( + id: 6, + path: "#{user.username}/#{project.path}", + source_id: project.id, + source_type: 'Project', + name: 'Completely wrong' + ) + + expect { described_class.new.perform(1, 5) } + .not_to change { route.reload.name } + end + + it 'does not update routes for projects in groups owned by the user' do + group = namespaces.create( + owner_id: user.id, + name: 'A group', + path: 'a-path', + type: '' + ) + project = projects.create(namespace_id: group.id, name: 'Project Name') + route = routes.create( + id: 1, + path: "#{group.path}/#{project.path}", + source_id: project.id, + source_type: 'Project', + name: 'Completely wrong' + ) + + expect { described_class.new.perform(1, 5) } + .not_to change { route.reload.name } + end + + it 'does not update routes for namespaces' do + route = routes.create( + id: 1, + path: namespace.path, + source_id: namespace.id, + source_type: 'Namespace', + name: 'Completely wrong' + ) + + expect { described_class.new.perform(1, 5) } + .not_to change { route.reload.name } + end +end diff --git a/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb b/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb index 4a81a37d341..ad4fa4fe03a 100644 --- a/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb @@ -27,14 +27,19 @@ describe Gitlab::BackgroundMigration::PopulateMergeRequestAssigneesTable, :migra merge_requests.create(params) end + before do + create_merge_request(2, assignee_id: user.id) + create_merge_request(3, assignee_id: user_2.id) + create_merge_request(4, assignee_id: user_3.id) + + # Test filtering MRs without assignees + create_merge_request(5, assignee_id: nil) + # Test filtering already migrated row + merge_request_assignees.create!(merge_request_id: 2, user_id: user_3.id) + end + describe '#perform' do it 'creates merge_request_assignees rows according to merge_requests' do - create_merge_request(2, assignee_id: user.id) - create_merge_request(3, assignee_id: user_2.id) - create_merge_request(4, assignee_id: user_3.id) - # Test filtering already migrated row - merge_request_assignees.create!(merge_request_id: 2, user_id: user_3.id) - subject.perform(1, 4) rows = merge_request_assignees.order(:id).map { |row| row.attributes.slice('merge_request_id', 'user_id') } @@ -53,4 +58,13 @@ describe Gitlab::BackgroundMigration::PopulateMergeRequestAssigneesTable, :migra end end end + + describe '#perform_all_sync' do + it 'executes peform for all merge requests in batches' do + expect(subject).to receive(:perform).with(2, 4).ordered + expect(subject).to receive(:perform).with(5, 5).ordered + + subject.perform_all_sync(batch_size: 3) + end + end end diff --git a/spec/lib/gitlab/batch_pop_queueing_spec.rb b/spec/lib/gitlab/batch_pop_queueing_spec.rb new file mode 100644 index 00000000000..28984d52024 --- /dev/null +++ b/spec/lib/gitlab/batch_pop_queueing_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BatchPopQueueing do + include ExclusiveLeaseHelpers + using RSpec::Parameterized::TableSyntax + + describe '#initialize' do + where(:namespace, :queue_id, :expect_error, :error_type) do + 'feature' | '1' | false | nil + :feature | '1' | false | nil + nil | '1' | true | NoMethodError + 'feature' | nil | true | NoMethodError + '' | '1' | true | ArgumentError + 'feature' | '' | true | ArgumentError + 'feature' | 1 | true | NoMethodError + end + + with_them do + it do + if expect_error + expect { described_class.new(namespace, queue_id) }.to raise_error(error_type) + else + expect { described_class.new(namespace, queue_id) }.not_to raise_error + end + end + end + end + + describe '#safe_execute', :clean_gitlab_redis_queues do + subject { queue.safe_execute(new_items, lock_timeout: lock_timeout) } + + let(:queue) { described_class.new(namespace, queue_id) } + let(:namespace) { 'feature' } + let(:queue_id) { '1' } + let(:lock_timeout) { 10.minutes } + let(:new_items) { %w[A B] } + let(:lock_key) { queue.send(:lock_key) } + let(:queue_key) { queue.send(:queue_key) } + + it 'enqueues new items always' do + Gitlab::Redis::Queues.with do |redis| + expect(redis).to receive(:sadd).with(queue_key, new_items) + expect(redis).to receive(:expire).with(queue_key, (lock_timeout + described_class::EXTRA_QUEUE_EXPIRE_WINDOW).to_i) + end + + subject + end + + it 'yields the new items with exclusive lease' do + uuid = 'test' + expect_to_obtain_exclusive_lease(lock_key, uuid, timeout: lock_timeout) + expect_to_cancel_exclusive_lease(lock_key, uuid) + + expect { |b| queue.safe_execute(new_items, lock_timeout: lock_timeout, &b) } + .to yield_with_args(match_array(new_items)) + end + + it 'returns the result and no items in the queue' do + expect(subject[:status]).to eq(:finished) + expect(subject[:new_items]).to be_empty + + Gitlab::Redis::Queues.with do |redis| + expect(redis.llen(queue_key)).to be(0) + end + end + + context 'when new items are enqueued during the process' do + it 'returns the result with newly added items' do + result = queue.safe_execute(new_items) do + queue.safe_execute(['C']) + end + + expect(result[:status]).to eq(:finished) + expect(result[:new_items]).to eq(['C']) + + Gitlab::Redis::Queues.with do |redis| + expect(redis.scard(queue_key)).to be(1) + end + end + end + + context 'when interger items are enqueued' do + let(:new_items) { [1, 2, 3] } + + it 'yields as String values' do + expect { |b| queue.safe_execute(new_items, lock_timeout: lock_timeout, &b) } + .to yield_with_args(%w[1 2 3]) + end + end + + context 'when the queue key does not exist in Redis' do + before do + allow(queue).to receive(:enqueue) { } + end + + it 'yields empty array' do + expect { |b| queue.safe_execute(new_items, lock_timeout: lock_timeout, &b) } + .to yield_with_args([]) + end + end + + context 'when the other process has already been working on the queue' do + before do + stub_exclusive_lease_taken(lock_key, timeout: lock_timeout) + end + + it 'does not yield the block' do + expect { |b| queue.safe_execute(new_items, lock_timeout: lock_timeout, &b) } + .not_to yield_control + end + + it 'returns the result' do + expect(subject[:status]).to eq(:enqueued) + end + end + + context 'when a duplicate item is enqueued' do + it 'returns the poped items to the queue and raise an error' do + expect { |b| queue.safe_execute(%w[1 1 2 2], &b) } + .to yield_with_args(match_array(%w[1 2])) + end + end + + context 'when there are two queues' do + it 'enqueues items to each queue' do + queue_1 = described_class.new(namespace, '1') + queue_2 = described_class.new(namespace, '2') + + result_2 = nil + + result_1 = queue_1.safe_execute(['A']) do |_| + result_2 = queue_2.safe_execute(['B']) do |_| + queue_1.safe_execute(['C']) + queue_2.safe_execute(['D']) + end + end + + expect(result_1[:status]).to eq(:finished) + expect(result_1[:new_items]).to eq(['C']) + expect(result_2[:status]).to eq(:finished) + expect(result_2[:new_items]).to eq(['D']) + end + end + end +end diff --git a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb index 483c5ea9cff..972dd7e0d2b 100644 --- a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb +++ b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb @@ -26,6 +26,7 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do end it 'loads 10 projects without hitting Gitaly call limit', :request_store do + allow(Gitlab::GitalyClient).to receive(:can_access_disk?).and_return(false) projects = Gitlab::GitalyClient.allow_n_plus_1_calls do (1..10).map { create(:project, :repository) } end diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index 909dbffa38f..db31e5280a3 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -80,7 +80,7 @@ describe Gitlab::CurrentSettings do # during the initialization phase of the test suite, so instead let's mock the internals of it expect(ActiveRecord::Base.connection).not_to receive(:active?) expect(ActiveRecord::Base.connection).not_to receive(:cached_table_exists?) - expect(ActiveRecord::Migrator).not_to receive(:needs_migration?) + expect_any_instance_of(ActiveRecord::MigrationContext).not_to receive(:needs_migration?) expect(ActiveRecord::QueryRecorder.new { described_class.current_application_settings }.count).to eq(0) end end @@ -109,7 +109,7 @@ describe Gitlab::CurrentSettings do context 'with pending migrations' do before do - expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true) + expect_any_instance_of(ActiveRecord::MigrationContext).to receive(:needs_migration?).and_return(true) end shared_examples 'a non-persisted ApplicationSetting object' do diff --git a/spec/lib/gitlab/cycle_analytics/events_spec.rb b/spec/lib/gitlab/cycle_analytics/events_spec.rb index f8b103c0fab..5ee02650e49 100644 --- a/spec/lib/gitlab/cycle_analytics/events_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/events_spec.rb @@ -7,7 +7,7 @@ describe 'cycle analytics events' do let!(:context) { create(:issue, project: project, created_at: 2.days.ago) } let(:events) do - CycleAnalytics.new(project, { from: from_date, current_user: user })[stage].events + CycleAnalytics::ProjectLevel.new(project, options: { from: from_date, current_user: user })[stage].events end before do diff --git a/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb index eacde22cd56..8633a63849f 100644 --- a/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb @@ -3,6 +3,40 @@ require 'lib/gitlab/cycle_analytics/shared_stage_spec' describe Gitlab::CycleAnalytics::TestStage do let(:stage_name) { :test } + let(:project) { create(:project) } + let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } it_behaves_like 'base stage' + + describe '#median' do + before do + issue_1 = create(:issue, project: project, created_at: 90.minutes.ago) + issue_2 = create(:issue, project: project, created_at: 60.minutes.ago) + issue_3 = create(:issue, project: project, created_at: 60.minutes.ago) + mr_1 = create(:merge_request, :closed, source_project: project, created_at: 60.minutes.ago) + mr_2 = create(:merge_request, :closed, source_project: project, created_at: 40.minutes.ago, source_branch: 'A') + mr_3 = create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') + mr_4 = create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'C') + mr_5 = create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'D') + mr_1.metrics.update!(latest_build_started_at: 32.minutes.ago, latest_build_finished_at: 2.minutes.ago) + mr_2.metrics.update!(latest_build_started_at: 62.minutes.ago, latest_build_finished_at: 32.minutes.ago) + mr_3.metrics.update!(latest_build_started_at: nil, latest_build_finished_at: nil) + mr_4.metrics.update!(latest_build_started_at: nil, latest_build_finished_at: nil) + mr_5.metrics.update!(latest_build_started_at: nil, latest_build_finished_at: nil) + + create(:merge_requests_closing_issues, merge_request: mr_1, issue: issue_1) + create(:merge_requests_closing_issues, merge_request: mr_2, issue: issue_2) + create(:merge_requests_closing_issues, merge_request: mr_3, issue: issue_3) + create(:merge_requests_closing_issues, merge_request: mr_4, issue: issue_3) + create(:merge_requests_closing_issues, merge_request: mr_5, issue: issue_3) + end + + around do |example| + Timecop.freeze { example.run } + end + + it 'counts median from issues with metrics' do + expect(stage.median).to eq(ISSUES_MEDIAN) + end + end end diff --git a/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb b/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb index a785b17f682..8122e85a981 100644 --- a/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb @@ -34,7 +34,7 @@ describe Gitlab::CycleAnalytics::UsageData do expect(result).to have_key(:avg_cycle_analytics) - CycleAnalytics::STAGES.each do |stage| + CycleAnalytics::Base::STAGES.each do |stage| expect(result[:avg_cycle_analytics]).to have_key(stage) stage_values = result[:avg_cycle_analytics][stage] diff --git a/spec/lib/gitlab/database_importers/common_metrics/importer_spec.rb b/spec/lib/gitlab/database_importers/common_metrics/importer_spec.rb new file mode 100644 index 00000000000..57c8bafd488 --- /dev/null +++ b/spec/lib/gitlab/database_importers/common_metrics/importer_spec.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Gitlab::DatabaseImporters::CommonMetrics::Importer do + subject { described_class.new } + + context "does import common_metrics.yml" do + let(:groups) { subject.content['panel_groups'] } + let(:panels) { groups.map { |group| group['panels'] }.flatten } + let(:metrics) { panels.map { |group| group['metrics'] }.flatten } + let(:metric_ids) { metrics.map { |metric| metric['id'] } } + + before do + subject.execute + end + + it "has the same amount of groups" do + expect(PrometheusMetric.common.group(:group).count.count).to eq(groups.count) + end + + it "has the same amount of panels" do + expect(PrometheusMetric.common.group(:group, :title).count.count).to eq(panels.count) + end + + it "has the same amount of metrics" do + expect(PrometheusMetric.common.count).to eq(metrics.count) + end + + it "does not have duplicate IDs" do + expect(metric_ids).to eq(metric_ids.uniq) + end + + it "imports all IDs" do + expect(PrometheusMetric.common.pluck(:identifier)).to contain_exactly(*metric_ids) + end + end + + context "does import common_metrics.yml" do + it "when executed from outside of the Rails.root" do + Dir.chdir(Dir.tmpdir) do + expect { subject.execute }.not_to raise_error + end + + expect(PrometheusMetric.common).not_to be_empty + end + end + + context 'does import properly all fields' do + let(:query_identifier) { 'response-metric' } + let(:dashboard) do + { + panel_groups: [{ + group: 'Response metrics (NGINX Ingress)', + panels: [{ + title: "Throughput", + y_label: "Requests / Sec", + metrics: [{ + id: query_identifier, + query_range: 'my-query', + unit: 'my-unit', + label: 'status code' + }] + }] + }] + } + end + + before do + expect(subject).to receive(:content) { dashboard.deep_stringify_keys } + end + + shared_examples 'stores metric' do + let(:metric) { PrometheusMetric.find_by(identifier: query_identifier) } + + it 'with all data' do + expect(metric.group).to eq('nginx_ingress') + expect(metric.title).to eq('Throughput') + expect(metric.y_label).to eq('Requests / Sec') + expect(metric.unit).to eq('my-unit') + expect(metric.legend).to eq('status code') + expect(metric.query).to eq('my-query') + end + end + + context 'if ID is missing' do + let(:query_identifier) { } + + it 'raises exception' do + expect { subject.execute }.to raise_error(Gitlab::DatabaseImporters::CommonMetrics::Importer::MissingQueryId) + end + end + + context 'for existing common metric with different ID' do + let!(:existing_metric) { create(:prometheus_metric, :common, identifier: 'my-existing-metric') } + + before do + subject.execute + end + + it_behaves_like 'stores metric' do + it 'and existing metric is not changed' do + expect(metric).not_to eq(existing_metric) + end + end + end + + context 'when metric with ID exists ' do + let!(:existing_metric) { create(:prometheus_metric, :common, identifier: 'response-metric') } + + before do + subject.execute + end + + it_behaves_like 'stores metric' do + it 'and existing metric is changed' do + expect(metric).to eq(existing_metric) + end + end + end + end +end diff --git a/spec/lib/gitlab/database_importers/common_metrics/prometheus_metric_spec.rb b/spec/lib/gitlab/database_importers/common_metrics/prometheus_metric_spec.rb new file mode 100644 index 00000000000..94f544e59b3 --- /dev/null +++ b/spec/lib/gitlab/database_importers/common_metrics/prometheus_metric_spec.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Gitlab::DatabaseImporters::CommonMetrics::PrometheusMetric do + it 'group enum equals ::PrometheusMetric' do + expect(described_class.groups).to eq(::PrometheusMetric.groups) + end + + it '.group_titles equals ::PrometheusMetric' do + existing_group_titles = ::PrometheusMetricEnums.group_details.each_with_object({}) do |(key, value), memo| + memo[key] = value[:group_title] + end + expect(Gitlab::DatabaseImporters::CommonMetrics::PrometheusMetricEnums.group_titles).to eq(existing_group_titles) + end +end diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index 48139c2f9dc..7833b9f387d 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -105,6 +105,7 @@ describe Gitlab::Email::Handler::CreateIssueHandler do context "when the issue could not be saved" do before do allow_any_instance_of(Issue).to receive(:persisted?).and_return(false) + allow_any_instance_of(Issue).to receive(:ensure_metrics).and_return(nil) end it "raises an InvalidIssueError" do diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 25052a79916..3f0e6b34291 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -408,7 +408,7 @@ describe Gitlab::Git::Commit, :seed_helper do context 'when oids is empty' do it 'makes no Gitaly request' do - expect(Gitlab::GitalyClient).not_to receive(:call) + expect(Gitlab::GitalyClient).not_to receive(:call).with(repository.storage, :commit_service, :list_commits_by_oid) described_class.batch_by_oid(repository, []) end diff --git a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb new file mode 100644 index 00000000000..e7ef9d08f80 --- /dev/null +++ b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'json' +require 'tempfile' + +describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:feature_flag_name) { 'feature-flag-name' } + let(:feature_flag) { Feature.get(feature_flag_name) } + let(:temp_gitaly_metadata_file) { create_temporary_gitaly_metadata_file } + + before(:all) do + create_gitaly_metadata_file + end + + subject(:wrapper) do + klazz = Class.new { include Gitlab::Git::RuggedImpl::UseRugged } + klazz.new + end + + before do + Gitlab::GitalyClient.instance_variable_set(:@can_use_disk, {}) + end + + context 'when feature flag is not persisted' do + before do + allow(Feature).to receive(:persisted?).with(feature_flag).and_return(false) + end + + it 'returns true when gitaly matches disk' do + pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338') + expect(subject.use_rugged?(repository, feature_flag_name)).to be true + end + + it 'returns false when disk access fails' do + allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist") + + expect(subject.use_rugged?(repository, feature_flag_name)).to be false + end + + it "returns false when gitaly doesn't match disk" do + allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file) + + expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey + + File.delete(temp_gitaly_metadata_file) + end + + it "doesn't lead to a second rpc call because gitaly client should use the cached value" do + pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338') + expect(subject.use_rugged?(repository, feature_flag_name)).to be true + + expect(Gitlab::GitalyClient).not_to receive(:filesystem_id) + + subject.use_rugged?(repository, feature_flag_name) + end + end + + context 'when feature flag is persisted' do + before do + allow(Feature).to receive(:persisted?).with(feature_flag).and_return(true) + end + + it 'returns false when the feature flag is off' do + allow(feature_flag).to receive(:enabled?).and_return(false) + + expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey + end + + it "returns true when feature flag is on" do + allow(feature_flag).to receive(:enabled?).and_return(true) + allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(false) + + expect(subject.use_rugged?(repository, feature_flag_name)).to be true + end + end + + def create_temporary_gitaly_metadata_file + tmp = Tempfile.new('.gitaly-metadata') + gitaly_metadata = { + "gitaly_filesystem_id" => "some-value" + } + tmp.write(gitaly_metadata.to_json) + tmp.flush + tmp.close + tmp.path + end + + def create_gitaly_metadata_file + File.open(File.join(SEED_STORAGE_PATH, '.gitaly-metadata'), 'w+') do |f| + gitaly_metadata = { + "gitaly_filesystem_id" => SecureRandom.uuid + } + f.write(gitaly_metadata.to_json) + end + end +end diff --git a/spec/lib/gitlab/global_id_spec.rb b/spec/lib/gitlab/global_id_spec.rb new file mode 100644 index 00000000000..d35b5da0b75 --- /dev/null +++ b/spec/lib/gitlab/global_id_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::GlobalId do + describe '.build' do + set(:object) { create(:issue) } + + it 'returns a standard GlobalId if only object is passed' do + expect(described_class.build(object).to_s).to eq(object.to_global_id.to_s) + end + + it 'returns a GlobalId from params' do + expect(described_class.build(model_name: 'MyModel', id: 'myid').to_s).to eq( + 'gid://gitlab/MyModel/myid' + ) + end + + it 'returns a GlobalId from object and `id` param' do + expect(described_class.build(object, id: 'myid').to_s).to eq( + 'gid://gitlab/Issue/myid' + ) + end + + it 'returns a GlobalId from object and `model_name` param' do + expect(described_class.build(object, model_name: 'MyModel').to_s).to eq( + "gid://gitlab/MyModel/#{object.id}" + ) + end + + it 'returns an error if model_name and id are not able to be determined' do + expect { described_class.build(id: 'myid') }.to raise_error(URI::InvalidComponentError) + expect { described_class.build(model_name: 'MyModel') }.to raise_error(URI::InvalidComponentError) + expect { described_class.build }.to raise_error(URI::InvalidComponentError) + end + end +end diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb index 978e64c4407..97ebb5f1554 100644 --- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb +++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb @@ -176,6 +176,9 @@ describe Gitlab::Kubernetes::KubeClient do let(:rbac_client) { client.rbac_client } [ + :create_role, + :get_role, + :update_role, :create_cluster_role_binding, :get_cluster_role_binding, :update_cluster_role_binding diff --git a/spec/lib/gitlab/kubernetes/role_binding_spec.rb b/spec/lib/gitlab/kubernetes/role_binding_spec.rb index 50acee254cb..4c200eb545f 100644 --- a/spec/lib/gitlab/kubernetes/role_binding_spec.rb +++ b/spec/lib/gitlab/kubernetes/role_binding_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe Gitlab::Kubernetes::RoleBinding, '#generate' do let(:role_name) { 'edit' } + let(:role_kind) { 'ClusterRole' } let(:namespace) { 'my-namespace' } let(:service_account_name) { 'my-service-account' } @@ -20,7 +21,7 @@ describe Gitlab::Kubernetes::RoleBinding, '#generate' do let(:role_ref) do { apiGroup: 'rbac.authorization.k8s.io', - kind: 'ClusterRole', + kind: role_kind, name: role_name } end @@ -37,6 +38,7 @@ describe Gitlab::Kubernetes::RoleBinding, '#generate' do described_class.new( name: "gitlab-#{namespace}", role_name: role_name, + role_kind: role_kind, namespace: namespace, service_account_name: service_account_name ).generate diff --git a/spec/lib/gitlab/kubernetes/role_spec.rb b/spec/lib/gitlab/kubernetes/role_spec.rb new file mode 100644 index 00000000000..3a5cd3b6704 --- /dev/null +++ b/spec/lib/gitlab/kubernetes/role_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Kubernetes::Role do + let(:role) { described_class.new(name: name, namespace: namespace, rules: rules) } + let(:name) { 'example-name' } + let(:namespace) { 'example-namespace' } + + let(:rules) do + [{ + apiGroups: %w(hello.world), + resources: %w(oil diamonds coffee), + verbs: %w(say do walk run) + }] + end + + describe '#generate' do + subject { role.generate } + + let(:resource) do + ::Kubeclient::Resource.new( + metadata: { name: name, namespace: namespace }, + rules: rules + ) + end + + it { is_expected.to eq(resource) } + end +end diff --git a/spec/lib/gitlab/lets_encrypt_spec.rb b/spec/lib/gitlab/lets_encrypt_spec.rb index 674b114e9d3..65aea0937f1 100644 --- a/spec/lib/gitlab/lets_encrypt_spec.rb +++ b/spec/lib/gitlab/lets_encrypt_spec.rb @@ -10,21 +10,10 @@ describe ::Gitlab::LetsEncrypt do end describe '.enabled?' do - let(:project) { create(:project) } - let(:pages_domain) { create(:pages_domain, project: project) } - - subject { described_class.enabled?(pages_domain) } + subject { described_class.enabled? } context 'when terms of service are accepted' do it { is_expected.to eq(true) } - - context 'when feature flag is disabled' do - before do - stub_feature_flags(pages_auto_ssl: false) - end - - it { is_expected.to eq(false) } - end end context 'when terms of service are not accepted' do @@ -34,23 +23,5 @@ describe ::Gitlab::LetsEncrypt do it { is_expected.to eq(false) } end - - context 'when feature flag for project is disabled' do - before do - stub_feature_flags(pages_auto_ssl_for_project: false) - end - - it 'returns false' do - is_expected.to eq(false) - end - end - - context 'when domain has not project' do - let(:pages_domain) { create(:pages_domain) } - - it 'returns false' do - is_expected.to eq(false) - end - end end end diff --git a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb index 18052b1991c..c5fc74afea5 100644 --- a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb +++ b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb @@ -9,12 +9,13 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do cache_markdown_field :title, whitelisted: true cache_markdown_field :description, pipeline: :single_line - attr_accessor :author, :project + attribute :author + attribute :project end end let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.create(title: markdown, title_html: html, cached_markdown_version: cache_version) } let(:markdown) { '`Foo`' } let(:html) { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Foo</code></p>' } @@ -37,7 +38,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'a changed markdown field' do - let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.create(title: markdown, title_html: html, cached_markdown_version: cache_version) } before do thing.title = updated_markdown diff --git a/spec/lib/gitlab/metrics/dashboard/url_spec.rb b/spec/lib/gitlab/metrics/dashboard/url_spec.rb new file mode 100644 index 00000000000..34bc6359414 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/url_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Metrics::Dashboard::Url do + describe '#regex' do + it 'returns a regular expression' do + expect(described_class.regex).to be_a Regexp + end + + it 'matches a metrics dashboard link with named params' do + url = Gitlab::Routing.url_helpers.metrics_namespace_project_environment_url('foo', 'bar', 1, start: 123345456, anchor: 'title') + + expected_params = { + 'url' => url, + 'namespace' => 'foo', + 'project' => 'bar', + 'environment' => '1', + 'query' => '?start=123345456', + 'anchor' => '#title' + } + + expect(described_class.regex).to match url + + described_class.regex.match(url) do |m| + expect(m.named_captures).to eq expected_params + end + end + + it 'does not match other gitlab urls that contain the term metrics' do + url = Gitlab::Routing.url_helpers.active_common_namespace_project_prometheus_metrics_url('foo', 'bar', :json) + + expect(described_class.regex).not_to match url + end + + it 'does not match other gitlab urls' do + url = Gitlab.config.gitlab.url + + expect(described_class.regex).not_to match url + end + + it 'does not match non-gitlab urls' do + url = 'https://www.super_awesome_site.com/' + + expect(described_class.regex).not_to match url + end + end + + describe '#build_dashboard_url' do + it 'builds the url for the dashboard endpoint' do + url = described_class.build_dashboard_url('foo', 'bar', 1) + + expect(url).to match described_class.regex + end + end +end diff --git a/spec/lib/gitlab/metrics/samplers/unicorn_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/unicorn_sampler_spec.rb index 090e456644f..4b697b2ba0f 100644 --- a/spec/lib/gitlab/metrics/samplers/unicorn_sampler_spec.rb +++ b/spec/lib/gitlab/metrics/samplers/unicorn_sampler_spec.rb @@ -4,7 +4,7 @@ describe Gitlab::Metrics::Samplers::UnicornSampler do subject { described_class.new(1.second) } describe '#sample' do - let(:unicorn) { double('unicorn') } + let(:unicorn) { Module.new } let(:raindrops) { double('raindrops') } let(:stats) { double('stats') } @@ -78,19 +78,32 @@ describe Gitlab::Metrics::Samplers::UnicornSampler do end end - context 'additional metrics' do - let(:unicorn_workers) { 2 } - + context 'unicorn workers' do before do - allow(unicorn).to receive(:listener_names).and_return([""]) - allow(::Gitlab::Metrics::System).to receive(:cpu_time).and_return(3.14) - allow(subject).to receive(:unicorn_workers_count).and_return(unicorn_workers) + allow(unicorn).to receive(:listener_names).and_return([]) end - it "sets additional metrics" do - expect(subject.metrics[:unicorn_workers]).to receive(:set).with({}, unicorn_workers) + context 'without http server' do + it "does set unicorn_workers to 0" do + expect(subject.metrics[:unicorn_workers]).to receive(:set).with({}, 0) - subject.sample + subject.sample + end + end + + context 'with http server' do + let(:http_server_class) { Struct.new(:worker_processes) } + let!(:http_server) { http_server_class.new(5) } + + before do + stub_const('Unicorn::HttpServer', http_server_class) + end + + it "sets additional metrics" do + expect(subject.metrics[:unicorn_workers]).to receive(:set).with({}, 5) + + subject.sample + end end end end diff --git a/spec/lib/gitlab/phabricator_import/cache/map_spec.rb b/spec/lib/gitlab/phabricator_import/cache/map_spec.rb index 52c7a02219f..b6629fad453 100644 --- a/spec/lib/gitlab/phabricator_import/cache/map_spec.rb +++ b/spec/lib/gitlab/phabricator_import/cache/map_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Gitlab::PhabricatorImport::Cache::Map, :clean_gitlab_redis_cache do @@ -28,6 +30,21 @@ describe Gitlab::PhabricatorImport::Cache::Map, :clean_gitlab_redis_cache do expect(ttl).to be > 10.seconds end + + it 'sets the object in redis once if a block was given and nothing was cached' do + issue = create(:issue, project: project) + + expect(map.get_gitlab_model('does not exist') { issue }).to eq(issue) + + expect { |b| map.get_gitlab_model('does not exist', &b) } + .not_to yield_control + end + + it 'does not cache `nil` objects' do + expect(map).not_to receive(:set_gitlab_model) + + map.get_gitlab_model('does not exist') { nil } + end end describe '#set_gitlab_model' do diff --git a/spec/lib/gitlab/phabricator_import/conduit/user_spec.rb b/spec/lib/gitlab/phabricator_import/conduit/user_spec.rb new file mode 100644 index 00000000000..e88eec2c393 --- /dev/null +++ b/spec/lib/gitlab/phabricator_import/conduit/user_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::PhabricatorImport::Conduit::User do + let(:user_client) do + described_class.new(phabricator_url: 'https://see-ya-later.phabricator', api_token: 'api-token') + end + + describe '#users' do + let(:fake_client) { double('Phabricator client') } + + before do + allow(user_client).to receive(:client).and_return(fake_client) + end + + it 'calls the api with the correct params' do + expected_params = { + constraints: { phids: ['phid-1', 'phid-2'] } + } + + expect(fake_client).to receive(:get).with('user.search', + params: expected_params) + + user_client.users(['phid-1', 'phid-2']) + end + + it 'returns an array of parsed responses' do + response = Gitlab::PhabricatorImport::Conduit::Response + .new(fixture_file('phabricator_responses/user.search.json')) + + allow(fake_client).to receive(:get).and_return(response) + + expect(user_client.users(%w[some phids])).to match_array([an_instance_of(Gitlab::PhabricatorImport::Conduit::UsersResponse)]) + end + + it 'performs multiple requests if more phids than the maximum page size are passed' do + stub_const('Gitlab::PhabricatorImport::Conduit::User::MAX_PAGE_SIZE', 1) + first_params = { constraints: { phids: ['phid-1'] } } + second_params = { constraints: { phids: ['phid-2'] } } + + expect(fake_client).to receive(:get).with('user.search', + params: first_params).once + expect(fake_client).to receive(:get).with('user.search', + params: second_params).once + + user_client.users(['phid-1', 'phid-2']) + end + end +end diff --git a/spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb b/spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb new file mode 100644 index 00000000000..00778ad90fd --- /dev/null +++ b/spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::PhabricatorImport::Conduit::UsersResponse do + let(:conduit_response) do + Gitlab::PhabricatorImport::Conduit::Response + .new(JSON.parse(fixture_file('phabricator_responses/user.search.json'))) + end + + subject(:response) { described_class.new(conduit_response) } + + describe '#users' do + it 'builds the correct users representation' do + tasks = response.users + + usernames = tasks.map(&:username) + + expect(usernames).to contain_exactly('jane', 'john') + end + end +end diff --git a/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb b/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb index 2412cf76f79..667321409da 100644 --- a/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb +++ b/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::PhabricatorImport::Issues::Importer do - set(:project) { create(:project) } + let(:project) { create(:project) } let(:response) do Gitlab::PhabricatorImport::Conduit::TasksResponse.new( @@ -15,7 +15,6 @@ describe Gitlab::PhabricatorImport::Issues::Importer do before do client = instance_double(Gitlab::PhabricatorImport::Conduit::Maniphest) - allow(client).to receive(:tasks).and_return(response) allow(importer).to receive(:client).and_return(client) end @@ -34,20 +33,29 @@ describe Gitlab::PhabricatorImport::Issues::Importer do importer.execute end - it 'schedules the next batch if there is one' do - expect(Gitlab::PhabricatorImport::ImportTasksWorker) - .to receive(:schedule).with(project.id, response.pagination.next_page) + context 'stubbed task import' do + before do + # Stub out the actual importing so we don't perform aditional requests + expect_next_instance_of(Gitlab::PhabricatorImport::Issues::TaskImporter) do |task_importer| + allow(task_importer).to receive(:execute) + end.at_least(1) + end - importer.execute - end + it 'schedules the next batch if there is one' do + expect(Gitlab::PhabricatorImport::ImportTasksWorker) + .to receive(:schedule).with(project.id, response.pagination.next_page) - it 'does not reschedule when there is no next page' do - allow(response.pagination).to receive(:has_next_page?).and_return(false) + importer.execute + end - expect(Gitlab::PhabricatorImport::ImportTasksWorker) - .not_to receive(:schedule) + it 'does not reschedule when there is no next page' do + allow(response.pagination).to receive(:has_next_page?).and_return(false) - importer.execute + expect(Gitlab::PhabricatorImport::ImportTasksWorker) + .not_to receive(:schedule) + + importer.execute + end end end end diff --git a/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb b/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb index 1625604e754..06ed264e781 100644 --- a/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb +++ b/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb @@ -12,6 +12,8 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do 'description' => { 'raw' => '# This is markdown\n it can contain more text.' }, + 'authorPHID' => 'PHID-USER-456', + 'ownerPHID' => 'PHID-USER-123', 'dateCreated' => '1518688921', 'dateClosed' => '1518789995' } @@ -19,9 +21,18 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do ) end + subject(:importer) { described_class.new(project, task) } + describe '#execute' do + let(:fake_user_finder) { instance_double(Gitlab::PhabricatorImport::UserFinder) } + + before do + allow(fake_user_finder).to receive(:find) + allow(importer).to receive(:user_finder).and_return(fake_user_finder) + end + it 'creates the issue with the expected attributes' do - issue = described_class.new(project, task).execute + issue = importer.execute expect(issue.project).to eq(project) expect(issue).to be_persisted @@ -34,21 +45,38 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do end it 'does not recreate the issue when called multiple times' do - expect { described_class.new(project, task).execute } + expect { importer.execute } .to change { project.issues.reload.size }.from(0).to(1) - expect { described_class.new(project, task).execute } + expect { importer.execute } .not_to change { project.issues.reload.size } end it 'does not trigger a save when the object did not change' do existing_issue = create(:issue, task.issue_attributes.merge(author: User.ghost)) - importer = described_class.new(project, task) allow(importer).to receive(:issue).and_return(existing_issue) expect(existing_issue).not_to receive(:save!) importer.execute end + + it 'links the author if the author can be found' do + author = create(:user) + expect(fake_user_finder).to receive(:find).with('PHID-USER-456').and_return(author) + + issue = importer.execute + + expect(issue.author).to eq(author) + end + + it 'links an assignee if the user can be found' do + assignee = create(:user) + expect(fake_user_finder).to receive(:find).with('PHID-USER-123').and_return(assignee) + + issue = importer.execute + + expect(issue.assignees).to include(assignee) + end end end diff --git a/spec/lib/gitlab/phabricator_import/representation/task_spec.rb b/spec/lib/gitlab/phabricator_import/representation/task_spec.rb index dfbd8c546eb..5603a6961d6 100644 --- a/spec/lib/gitlab/phabricator_import/representation/task_spec.rb +++ b/spec/lib/gitlab/phabricator_import/representation/task_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Gitlab::PhabricatorImport::Representation::Task do @@ -7,6 +9,8 @@ describe Gitlab::PhabricatorImport::Representation::Task do 'phid' => 'the-phid', 'fields' => { 'name' => 'Title'.ljust(257, '.'), # A string padded to 257 chars + 'authorPHID' => 'a phid', + 'ownerPHID' => 'another user phid', 'description' => { 'raw' => '# This is markdown\n it can contain more text.' }, @@ -30,4 +34,16 @@ describe Gitlab::PhabricatorImport::Representation::Task do expect(task.issue_attributes).to eq(expected_attributes) end end + + describe '#author_phid' do + it 'returns the correct field' do + expect(task.author_phid).to eq('a phid') + end + end + + describe '#owner_phid' do + it 'returns the correct field' do + expect(task.owner_phid).to eq('another user phid') + end + end end diff --git a/spec/lib/gitlab/phabricator_import/representation/user_spec.rb b/spec/lib/gitlab/phabricator_import/representation/user_spec.rb new file mode 100644 index 00000000000..f52467a0cf1 --- /dev/null +++ b/spec/lib/gitlab/phabricator_import/representation/user_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::PhabricatorImport::Representation::User do + subject(:user) do + described_class.new( + { + 'phid' => 'the-phid', + 'fields' => { + 'username' => 'the-username' + } + } + ) + end + + describe '#phabricator_id' do + it 'returns the phabricator id' do + expect(user.phabricator_id).to eq('the-phid') + end + end + + describe '#username' do + it 'returns the username' do + expect(user.username).to eq('the-username') + end + end +end diff --git a/spec/lib/gitlab/phabricator_import/user_finder_spec.rb b/spec/lib/gitlab/phabricator_import/user_finder_spec.rb new file mode 100644 index 00000000000..096321cda5f --- /dev/null +++ b/spec/lib/gitlab/phabricator_import/user_finder_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe Gitlab::PhabricatorImport::UserFinder, :clean_gitlab_redis_cache do + let(:project) { create(:project, namespace: create(:group)) } + subject(:finder) { described_class.new(project, ['first-phid', 'second-phid']) } + + before do + project.namespace.add_developer(existing_user) + end + + describe '#find' do + let!(:existing_user) { create(:user, username: 'existing-user') } + let(:cache) { Gitlab::PhabricatorImport::Cache::Map.new(project) } + + before do + allow(finder).to receive(:object_map).and_return(cache) + end + + context 'for a cached phid' do + before do + cache.set_gitlab_model(existing_user, 'first-phid') + end + + it 'returns the existing user' do + expect(finder.find('first-phid')).to eq(existing_user) + end + + it 'does not perform a find using the API' do + expect(finder).not_to receive(:find_user_for_phid) + + finder.find('first-phid') + end + + it 'excludes the phid from the request if one needs to be made' do + client = instance_double(Gitlab::PhabricatorImport::Conduit::User) + allow(finder).to receive(:client).and_return(client) + + expect(client).to receive(:users).with(['second-phid']).and_return([]) + + finder.find('first-phid') + finder.find('second-phid') + end + end + + context 'when the phid is not cached' do + let(:response) do + [ + instance_double( + Gitlab::PhabricatorImport::Conduit::UsersResponse, + users: [instance_double(Gitlab::PhabricatorImport::Representation::User, phabricator_id: 'second-phid', username: 'existing-user')] + ), + instance_double( + Gitlab::PhabricatorImport::Conduit::UsersResponse, + users: [instance_double(Gitlab::PhabricatorImport::Representation::User, phabricator_id: 'first-phid', username: 'other-user')] + ) + ] + end + let(:client) do + client = instance_double(Gitlab::PhabricatorImport::Conduit::User) + allow(client).to receive(:users).and_return(response) + + client + end + + before do + allow(finder).to receive(:client).and_return(client) + end + + it 'loads the users from the API once' do + expect(client).to receive(:users).and_return(response).once + + expect(finder.find('second-phid')).to eq(existing_user) + expect(finder.find('first-phid')).to be_nil + end + + it 'adds found users to the cache' do + expect { finder.find('second-phid') } + .to change { cache.get_gitlab_model('second-phid') } + .from(nil).to(existing_user) + end + + it 'only returns users that are members of the project' do + create(:user, username: 'other-user') + + expect(finder.find('first-phid')).to eq(nil) + end + end + end +end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index 5b5052de372..98838712eae 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -10,6 +10,12 @@ describe Gitlab::SQL::Pattern do it 'returns exact matching pattern' do expect(to_pattern).to eq('12') end + + context 'and ignore_minimum_char_limit is true' do + it 'returns partial matching pattern' do + expect(User.to_pattern(query, use_minimum_char_limit: false)).to eq('%12%') + end + end end context 'when a query with a escape character is shorter than 3 chars' do @@ -18,6 +24,12 @@ describe Gitlab::SQL::Pattern do it 'returns sanitized exact matching pattern' do expect(to_pattern).to eq('\_2') end + + context 'and ignore_minimum_char_limit is true' do + it 'returns sanitized partial matching pattern' do + expect(User.to_pattern(query, use_minimum_char_limit: false)).to eq('%\_2%') + end + end end context 'when a query is equal to 3 chars' do diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 253366e0789..f8b0cbfb6f6 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -353,7 +353,7 @@ describe Gitlab::UrlBlocker do end end - describe '#validate_hostname!' do + describe '#validate_hostname' do let(:ip_addresses) do [ '2001:db8:1f70::999:de8:7648:6e8', @@ -378,7 +378,7 @@ describe Gitlab::UrlBlocker do it 'does not raise error for valid Ip addresses' do ip_addresses.each do |ip| - expect { described_class.send(:validate_hostname!, ip) }.not_to raise_error + expect { described_class.send(:validate_hostname, ip) }.not_to raise_error end end end |