diff options
Diffstat (limited to 'spec/lib/gitlab/danger')
-rw-r--r-- | spec/lib/gitlab/danger/changelog_spec.rb | 42 | ||||
-rw-r--r-- | spec/lib/gitlab/danger/commit_linter_spec.rb | 14 | ||||
-rw-r--r-- | spec/lib/gitlab/danger/emoji_checker_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/danger/helper_spec.rb | 328 | ||||
-rw-r--r-- | spec/lib/gitlab/danger/roulette_spec.rb | 265 | ||||
-rw-r--r-- | spec/lib/gitlab/danger/sidekiq_queues_spec.rb | 82 | ||||
-rw-r--r-- | spec/lib/gitlab/danger/teammate_spec.rb | 125 |
7 files changed, 567 insertions, 291 deletions
diff --git a/spec/lib/gitlab/danger/changelog_spec.rb b/spec/lib/gitlab/danger/changelog_spec.rb index 130a4708cec..f5954cd8c1e 100644 --- a/spec/lib/gitlab/danger/changelog_spec.rb +++ b/spec/lib/gitlab/danger/changelog_spec.rb @@ -1,13 +1,11 @@ # frozen_string_literal: true require 'fast_spec_helper' -require 'rspec-parameterized' require_relative 'danger_spec_helper' require 'gitlab/danger/changelog' -describe Gitlab::Danger::Changelog do - using RSpec::Parameterized::TableSyntax +RSpec.describe Gitlab::Danger::Changelog do include DangerSpecHelper let(:added_files) { nil } @@ -26,34 +24,36 @@ describe Gitlab::Danger::Changelog do subject(:changelog) { fake_danger.new(git: fake_git, gitlab: fake_gitlab, helper: fake_helper) } describe '#needed?' do - subject { changelog.needed? } + let(:category_with_changelog) { :backend } + let(:label_with_changelog) { 'frontend' } + let(:category_without_changelog) { Gitlab::Danger::Changelog::NO_CHANGELOG_CATEGORIES.first } + let(:label_without_changelog) { Gitlab::Danger::Changelog::NO_CHANGELOG_LABELS.first } - where(:categories, :labels) do - { backend: nil } | %w[backend backstage] - { frontend: nil, docs: nil } | ['ci-build'] - { engineering_productivity: nil, none: nil } | ['meta'] - end + subject { changelog.needed? } - with_them do - let(:changes_by_category) { categories } - let(:mr_labels) { labels } + context 'when MR contains only categories requiring no changelog' do + let(:changes_by_category) { { category_without_changelog => nil } } + let(:mr_labels) { [] } - it "is falsy when categories and labels require no changelog" do + it 'is falsey' do is_expected.to be_falsy end end - where(:categories, :labels) do - { frontend: nil, docs: nil } | ['database::review pending', 'feature'] - { backend: nil } | ['backend', 'technical debt'] - { engineering_productivity: nil, none: nil } | ['frontend'] + context 'when MR contains a label that require no changelog' do + let(:changes_by_category) { { category_with_changelog => nil } } + let(:mr_labels) { [label_with_changelog, label_without_changelog] } + + it 'is falsey' do + is_expected.to be_falsy + end end - with_them do - let(:changes_by_category) { categories } - let(:mr_labels) { labels } + context 'when MR contains a category that require changelog and a category that require no changelog' do + let(:changes_by_category) { { category_with_changelog => nil, category_without_changelog => nil } } + let(:mr_labels) { [] } - it "is truthy when categories and labels require a changelog" do + it 'is truthy' do is_expected.to be_truthy end end diff --git a/spec/lib/gitlab/danger/commit_linter_spec.rb b/spec/lib/gitlab/danger/commit_linter_spec.rb index e57ccd12fa5..06bec6f793d 100644 --- a/spec/lib/gitlab/danger/commit_linter_spec.rb +++ b/spec/lib/gitlab/danger/commit_linter_spec.rb @@ -6,7 +6,7 @@ require_relative 'danger_spec_helper' require 'gitlab/danger/commit_linter' -describe Gitlab::Danger::CommitLinter do +RSpec.describe Gitlab::Danger::CommitLinter do using RSpec::Parameterized::TableSyntax let(:total_files_changed) { 2 } @@ -156,7 +156,7 @@ describe Gitlab::Danger::CommitLinter do context 'when subject is a WIP' do let(:final_message) { 'A B C' } # commit message with prefix will be over max length. commit message without prefix will be of maximum size - let(:commit_message) { described_class::WIP_PREFIX + final_message + 'D' * (described_class::WARN_SUBJECT_LENGTH - final_message.size) } + let(:commit_message) { described_class::WIP_PREFIX + final_message + 'D' * (described_class::MAX_LINE_LENGTH - final_message.size) } it 'does not have any problems' do commit_linter.lint @@ -176,16 +176,6 @@ describe Gitlab::Danger::CommitLinter do end end - context 'when subject is above warning' do - let(:commit_message) { 'A B ' + 'C' * described_class::WARN_SUBJECT_LENGTH } - - it 'adds a problem' do - expect(commit_linter).to receive(:add_problem).with(:subject_above_warning, described_class::DEFAULT_SUBJECT_DESCRIPTION) - - commit_linter.lint - end - end - context 'when subject starts with lowercase' do let(:commit_message) { 'a B C' } diff --git a/spec/lib/gitlab/danger/emoji_checker_spec.rb b/spec/lib/gitlab/danger/emoji_checker_spec.rb index 0cdc18ce626..6092c751e1c 100644 --- a/spec/lib/gitlab/danger/emoji_checker_spec.rb +++ b/spec/lib/gitlab/danger/emoji_checker_spec.rb @@ -5,7 +5,7 @@ require 'rspec-parameterized' require 'gitlab/danger/emoji_checker' -describe Gitlab::Danger::EmojiChecker do +RSpec.describe Gitlab::Danger::EmojiChecker do using RSpec::Parameterized::TableSyntax describe '#includes_text_emoji?' do diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb index 809064a540c..e73742b5911 100644 --- a/spec/lib/gitlab/danger/helper_spec.rb +++ b/spec/lib/gitlab/danger/helper_spec.rb @@ -6,7 +6,7 @@ require_relative 'danger_spec_helper' require 'gitlab/danger/helper' -describe Gitlab::Danger::Helper do +RSpec.describe Gitlab::Danger::Helper do using RSpec::Parameterized::TableSyntax include DangerSpecHelper @@ -165,125 +165,152 @@ describe Gitlab::Danger::Helper do end end - describe '#category_for_file' do - where(:path, :expected_category) do - 'doc/foo' | :none - 'CONTRIBUTING.md' | :none - 'LICENSE' | :none - 'MAINTENANCE.md' | :none - 'PHILOSOPHY.md' | :none - 'PROCESS.md' | :none - 'README.md' | :none - - 'ee/doc/foo' | :unknown - 'ee/README' | :unknown - - 'app/assets/foo' | :frontend - 'app/views/foo' | :frontend - 'public/foo' | :frontend - 'scripts/frontend/foo' | :frontend - 'spec/javascripts/foo' | :frontend - 'spec/frontend/bar' | :frontend - 'vendor/assets/foo' | :frontend - 'babel.config.js' | :frontend - 'jest.config.js' | :frontend - 'package.json' | :frontend - 'yarn.lock' | :frontend - 'config/foo.js' | :frontend - 'config/deep/foo.js' | :frontend - - 'ee/app/assets/foo' | :frontend - 'ee/app/views/foo' | :frontend - 'ee/spec/javascripts/foo' | :frontend - 'ee/spec/frontend/bar' | :frontend - - 'app/models/foo' | :backend - 'bin/foo' | :backend - 'config/foo' | :backend - 'lib/foo' | :backend - 'rubocop/foo' | :backend - 'spec/foo' | :backend - 'spec/foo/bar' | :backend - - 'ee/app/foo' | :backend - 'ee/bin/foo' | :backend - 'ee/spec/foo' | :backend - 'ee/spec/foo/bar' | :backend - - 'generator_templates/foo' | :backend - 'vendor/languages.yml' | :backend - 'vendor/licenses.csv' | :backend - 'file_hooks/examples/' | :backend - - 'Gemfile' | :backend - 'Gemfile.lock' | :backend - 'Rakefile' | :backend - 'FOO_VERSION' | :backend - - 'Dangerfile' | :engineering_productivity - 'danger/commit_messages/Dangerfile' | :engineering_productivity - 'ee/danger/commit_messages/Dangerfile' | :engineering_productivity - 'danger/commit_messages/' | :engineering_productivity - 'ee/danger/commit_messages/' | :engineering_productivity - '.gitlab-ci.yml' | :engineering_productivity - '.gitlab/ci/cng.gitlab-ci.yml' | :engineering_productivity - '.gitlab/ci/ee-specific-checks.gitlab-ci.yml' | :engineering_productivity - 'scripts/foo' | :engineering_productivity - 'lib/gitlab/danger/foo' | :engineering_productivity - 'ee/lib/gitlab/danger/foo' | :engineering_productivity - '.overcommit.yml.example' | :engineering_productivity - '.editorconfig' | :engineering_productivity - 'tooling/overcommit/foo' | :engineering_productivity - '.codeclimate.yml' | :engineering_productivity - - 'lib/gitlab/ci/templates/Security/SAST.gitlab-ci.yml' | :backend - - 'ee/FOO_VERSION' | :unknown - - 'db/schema.rb' | :database - 'db/structure.sql' | :database - 'db/migrate/foo' | :database - 'db/post_migrate/foo' | :database - 'ee/db/migrate/foo' | :database - 'ee/db/post_migrate/foo' | :database - 'ee/db/geo/migrate/foo' | :database - 'ee/db/geo/post_migrate/foo' | :database - 'app/models/project_authorization.rb' | :database - 'app/services/users/refresh_authorized_projects_service.rb' | :database - 'lib/gitlab/background_migration.rb' | :database - 'lib/gitlab/background_migration/foo' | :database - 'ee/lib/gitlab/background_migration/foo' | :database - 'lib/gitlab/database.rb' | :database - 'lib/gitlab/database/foo' | :database - 'ee/lib/gitlab/database/foo' | :database - 'lib/gitlab/github_import.rb' | :database - 'lib/gitlab/github_import/foo' | :database - 'lib/gitlab/sql/foo' | :database - 'rubocop/cop/migration/foo' | :database - - 'db/fixtures/foo.rb' | :backend - 'ee/db/fixtures/foo.rb' | :backend - - 'qa/foo' | :qa - 'ee/qa/foo' | :qa - - 'changelogs/foo' | :none - 'ee/changelogs/foo' | :none - 'locale/gitlab.pot' | :none - - 'FOO' | :unknown - 'foo' | :unknown - - 'foo/bar.rb' | :backend - 'foo/bar.js' | :frontend - 'foo/bar.txt' | :none - 'foo/bar.md' | :none + describe '#categories_for_file' do + before do + allow(fake_git).to receive(:diff_for_file).with('usage_data.rb') { double(:diff, patch: "+ count(User.active)") } + end + + where(:path, :expected_categories) do + 'usage_data.rb' | [:database, :backend] + 'doc/foo.md' | [:docs] + 'CONTRIBUTING.md' | [:docs] + 'LICENSE' | [:docs] + 'MAINTENANCE.md' | [:docs] + 'PHILOSOPHY.md' | [:docs] + 'PROCESS.md' | [:docs] + 'README.md' | [:docs] + + 'ee/doc/foo' | [:unknown] + 'ee/README' | [:unknown] + + 'app/assets/foo' | [:frontend] + 'app/views/foo' | [:frontend] + 'public/foo' | [:frontend] + 'scripts/frontend/foo' | [:frontend] + 'spec/javascripts/foo' | [:frontend] + 'spec/frontend/bar' | [:frontend] + 'vendor/assets/foo' | [:frontend] + 'babel.config.js' | [:frontend] + 'jest.config.js' | [:frontend] + 'package.json' | [:frontend] + 'yarn.lock' | [:frontend] + 'config/foo.js' | [:frontend] + 'config/deep/foo.js' | [:frontend] + + 'ee/app/assets/foo' | [:frontend] + 'ee/app/views/foo' | [:frontend] + 'ee/spec/javascripts/foo' | [:frontend] + 'ee/spec/frontend/bar' | [:frontend] + + '.gitlab/ci/frontend.gitlab-ci.yml' | %i[frontend engineering_productivity] + + 'app/models/foo' | [:backend] + 'bin/foo' | [:backend] + 'config/foo' | [:backend] + 'lib/foo' | [:backend] + 'rubocop/foo' | [:backend] + 'spec/foo' | [:backend] + 'spec/foo/bar' | [:backend] + + 'ee/app/foo' | [:backend] + 'ee/bin/foo' | [:backend] + 'ee/spec/foo' | [:backend] + 'ee/spec/foo/bar' | [:backend] + + 'generator_templates/foo' | [:backend] + 'vendor/languages.yml' | [:backend] + 'vendor/licenses.csv' | [:backend] + 'file_hooks/examples/' | [:backend] + + 'Gemfile' | [:backend] + 'Gemfile.lock' | [:backend] + 'Rakefile' | [:backend] + 'FOO_VERSION' | [:backend] + + 'Dangerfile' | [:engineering_productivity] + 'danger/commit_messages/Dangerfile' | [:engineering_productivity] + 'ee/danger/commit_messages/Dangerfile' | [:engineering_productivity] + 'danger/commit_messages/' | [:engineering_productivity] + 'ee/danger/commit_messages/' | [:engineering_productivity] + '.gitlab-ci.yml' | [:engineering_productivity] + '.gitlab/ci/cng.gitlab-ci.yml' | [:engineering_productivity] + '.gitlab/ci/ee-specific-checks.gitlab-ci.yml' | [:engineering_productivity] + 'scripts/foo' | [:engineering_productivity] + 'lib/gitlab/danger/foo' | [:engineering_productivity] + 'ee/lib/gitlab/danger/foo' | [:engineering_productivity] + '.overcommit.yml.example' | [:engineering_productivity] + '.editorconfig' | [:engineering_productivity] + 'tooling/overcommit/foo' | [:engineering_productivity] + '.codeclimate.yml' | [:engineering_productivity] + + 'lib/gitlab/ci/templates/Security/SAST.gitlab-ci.yml' | [:backend] + + 'ee/FOO_VERSION' | [:unknown] + + 'db/schema.rb' | [:database] + 'db/structure.sql' | [:database] + 'db/migrate/foo' | [:database] + 'db/post_migrate/foo' | [:database] + 'ee/db/migrate/foo' | [:database] + 'ee/db/post_migrate/foo' | [:database] + 'ee/db/geo/migrate/foo' | [:database] + 'ee/db/geo/post_migrate/foo' | [:database] + 'app/models/project_authorization.rb' | [:database] + 'app/services/users/refresh_authorized_projects_service.rb' | [:database] + 'lib/gitlab/background_migration.rb' | [:database] + 'lib/gitlab/background_migration/foo' | [:database] + 'ee/lib/gitlab/background_migration/foo' | [:database] + 'lib/gitlab/database.rb' | [:database] + 'lib/gitlab/database/foo' | [:database] + 'ee/lib/gitlab/database/foo' | [:database] + 'lib/gitlab/github_import.rb' | [:database] + 'lib/gitlab/github_import/foo' | [:database] + 'lib/gitlab/sql/foo' | [:database] + 'rubocop/cop/migration/foo' | [:database] + + 'db/fixtures/foo.rb' | [:backend] + 'ee/db/fixtures/foo.rb' | [:backend] + + 'qa/foo' | [:qa] + 'ee/qa/foo' | [:qa] + + 'changelogs/foo' | [:none] + 'ee/changelogs/foo' | [:none] + 'locale/gitlab.pot' | [:none] + + 'FOO' | [:unknown] + 'foo' | [:unknown] + + 'foo/bar.rb' | [:backend] + 'foo/bar.js' | [:frontend] + 'foo/bar.txt' | [:none] + 'foo/bar.md' | [:none] end with_them do - subject { helper.category_for_file(path) } + subject { helper.categories_for_file(path) } - it { is_expected.to eq(expected_category) } + it { is_expected.to eq(expected_categories) } + end + + context 'having specific changes' do + it 'has database and backend categories' do + allow(fake_git).to receive(:diff_for_file).with('usage_data.rb') { double(:diff, patch: "+ count(User.active)") } + + expect(helper.categories_for_file('usage_data.rb')).to eq([:database, :backend]) + end + + it 'has backend category' do + allow(fake_git).to receive(:diff_for_file).with('usage_data.rb') { double(:diff, patch: "+ alt_usage_data(User.active)") } + + expect(helper.categories_for_file('usage_data.rb')).to eq([:backend]) + end + + it 'has backend category for changes outside usage_data files' do + allow(fake_git).to receive(:diff_for_file).with('user.rb') { double(:diff, patch: "+ count(User.active)") } + + expect(helper.categories_for_file('user.rb')).to eq([:backend]) + end end end @@ -296,6 +323,7 @@ describe Gitlab::Danger::Helper do :frontend | '~frontend' :none | '' :qa | '~QA' + :engineering_productivity | '~"Engineering Productivity" for CI, Danger' end with_them do @@ -335,6 +363,11 @@ describe Gitlab::Danger::Helper do where(:mr_title, :expected_mr_title) do 'My MR title' | 'My MR title' 'WIP: My MR title' | 'My MR title' + 'Draft: My MR title' | 'My MR title' + '(Draft) My MR title' | 'My MR title' + '[Draft] My MR title' | 'My MR title' + '[DRAFT] My MR title' | 'My MR title' + 'DRAFT: My MR title' | 'My MR title' end with_them do @@ -366,6 +399,69 @@ describe Gitlab::Danger::Helper do end end + describe '#cherry_pick_mr?' do + it 'returns false when `gitlab_helper` is unavailable' do + expect(helper).to receive(:gitlab_helper).and_return(nil) + + expect(helper).not_to be_cherry_pick_mr + end + + context 'when MR title does not mention a cherry-pick' do + it 'returns false' do + expect(fake_gitlab).to receive(:mr_json) + .and_return('title' => 'Add feature xyz') + + expect(helper).not_to be_cherry_pick_mr + end + end + + context 'when MR title mentions a cherry-pick' do + [ + 'Cherry Pick !1234', + 'cherry-pick !1234', + 'CherryPick !1234' + ].each do |mr_title| + it 'returns true' do + expect(fake_gitlab).to receive(:mr_json) + .and_return('title' => mr_title) + + expect(helper).to be_cherry_pick_mr + end + end + end + end + + describe '#stable_branch?' do + it 'returns false when `gitlab_helper` is unavailable' do + expect(helper).to receive(:gitlab_helper).and_return(nil) + + expect(helper).not_to be_stable_branch + end + + context 'when MR target branch is not a stable branch' do + it 'returns false' do + expect(fake_gitlab).to receive(:mr_json) + .and_return('target_branch' => 'my-feature-branch') + + expect(helper).not_to be_stable_branch + end + end + + context 'when MR target branch is a stable branch' do + %w[ + 13-1-stable-ee + 13-1-stable-ee-patch-1 + ].each do |target_branch| + it 'returns true' do + expect(fake_gitlab).to receive(:mr_json) + .and_return('target_branch' => target_branch) + + expect(helper).to be_stable_branch + end + end + end + end + describe '#mr_has_label?' do it 'returns false when `gitlab_helper` is unavailable' do expect(helper).to receive(:gitlab_helper).and_return(nil) diff --git a/spec/lib/gitlab/danger/roulette_spec.rb b/spec/lib/gitlab/danger/roulette_spec.rb index b6148cd1407..676edca2459 100644 --- a/spec/lib/gitlab/danger/roulette_spec.rb +++ b/spec/lib/gitlab/danger/roulette_spec.rb @@ -2,16 +2,23 @@ require 'fast_spec_helper' require 'webmock/rspec' +require 'timecop' require 'gitlab/danger/roulette' -describe Gitlab::Danger::Roulette do +RSpec.describe Gitlab::Danger::Roulette do + around do |example| + Timecop.freeze(Time.utc(2020, 06, 22, 10)) { example.run } + end + let(:backend_maintainer) do { username: 'backend-maintainer', name: 'Backend maintainer', role: 'Backend engineer', - projects: { 'gitlab' => 'maintainer backend' } + projects: { 'gitlab' => 'maintainer backend' }, + available: true, + tz_offset_hours: 2.0 } end let(:frontend_reviewer) do @@ -19,7 +26,9 @@ describe Gitlab::Danger::Roulette do username: 'frontend-reviewer', name: 'Frontend reviewer', role: 'Frontend engineer', - projects: { 'gitlab' => 'reviewer frontend' } + projects: { 'gitlab' => 'reviewer frontend' }, + available: true, + tz_offset_hours: 2.0 } end let(:frontend_maintainer) do @@ -27,7 +36,9 @@ describe Gitlab::Danger::Roulette do username: 'frontend-maintainer', name: 'Frontend maintainer', role: 'Frontend engineer', - projects: { 'gitlab' => "maintainer frontend" } + projects: { 'gitlab' => "maintainer frontend" }, + available: true, + tz_offset_hours: 2.0 } end let(:software_engineer_in_test) do @@ -38,7 +49,9 @@ describe Gitlab::Danger::Roulette do projects: { 'gitlab' => 'reviewer qa', 'gitlab-qa' => 'maintainer' - } + }, + available: true, + tz_offset_hours: 2.0 } end let(:engineering_productivity_reviewer) do @@ -46,7 +59,9 @@ describe Gitlab::Danger::Roulette do username: 'eng-prod-reviewer', name: 'EP engineer', role: 'Engineering Productivity', - projects: { 'gitlab' => 'reviewer backend' } + projects: { 'gitlab' => 'reviewer backend' }, + available: true, + tz_offset_hours: 2.0 } end @@ -73,10 +88,17 @@ describe Gitlab::Danger::Roulette do def matching_spin(category, reviewer: { username: nil }, maintainer: { username: nil }, optional: nil) satisfy do |spin| - spin.category == category && - spin.reviewer&.username == reviewer[:username] && - spin.maintainer&.username == maintainer[:username] && - spin.optional_role == optional + bool = spin.category == category + bool &&= spin.reviewer&.username == reviewer[:username] + + bool &&= + if maintainer + spin.maintainer&.username == maintainer[:username] + else + spin.maintainer.nil? + end + + bool && spin.optional_role == optional end end @@ -85,67 +107,114 @@ describe Gitlab::Danger::Roulette do let!(:branch_name) { 'a-branch' } let!(:mr_labels) { ['backend', 'devops::create'] } let!(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') } - - before do - [ - backend_maintainer, - frontend_reviewer, - frontend_maintainer, - software_engineer_in_test, - engineering_productivity_reviewer - ].each do |person| - stub_person_status(instance_double(Gitlab::Danger::Teammate, username: person[:username]), message: 'making GitLab magic') - end - + let(:timezone_experiment) { false } + let(:spins) do + # Stub the request at the latest time so that we can modify the raw data, e.g. available fields. WebMock .stub_request(:get, described_class::ROULETTE_DATA_URL) .to_return(body: teammate_json) + + subject.spin(project, categories, branch_name, timezone_experiment: timezone_experiment) + end + + before do allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username) allow(subject).to receive_message_chain(:gitlab, :mr_labels).and_return(mr_labels) end - context 'when change contains backend category' do - it 'assigns backend reviewer and maintainer' do - categories = [:backend] - spins = subject.spin(project, categories, branch_name) + context 'when timezone_experiment == false' do + context 'when change contains backend category' do + let(:categories) { [:backend] } - expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + it 'assigns backend reviewer and maintainer' do + expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + end + + context 'when teammate is not available' do + before do + backend_maintainer[:available] = false + end + + it 'assigns backend reviewer and no maintainer' do + expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil)) + end + end end - end - context 'when change contains frontend category' do - it 'assigns frontend reviewer and maintainer' do - categories = [:frontend] - spins = subject.spin(project, categories, branch_name) + context 'when change contains frontend category' do + let(:categories) { [:frontend] } - expect(spins).to contain_exactly(matching_spin(:frontend, reviewer: frontend_reviewer, maintainer: frontend_maintainer)) + it 'assigns frontend reviewer and maintainer' do + expect(spins).to contain_exactly(matching_spin(:frontend, reviewer: frontend_reviewer, maintainer: frontend_maintainer)) + end end - end - context 'when change contains QA category' do - it 'assigns QA reviewer and sets optional QA maintainer' do - categories = [:qa] - spins = subject.spin(project, categories, branch_name) + context 'when change contains QA category' do + let(:categories) { [:qa] } - expect(spins).to contain_exactly(matching_spin(:qa, reviewer: software_engineer_in_test, optional: :maintainer)) + it 'assigns QA reviewer' do + expect(spins).to contain_exactly(matching_spin(:qa, reviewer: software_engineer_in_test)) + end end - end - context 'when change contains Engineering Productivity category' do - it 'assigns Engineering Productivity reviewer and fallback to backend maintainer' do - categories = [:engineering_productivity] - spins = subject.spin(project, categories, branch_name) + context 'when change contains Engineering Productivity category' do + let(:categories) { [:engineering_productivity] } - expect(spins).to contain_exactly(matching_spin(:engineering_productivity, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + it 'assigns Engineering Productivity reviewer and fallback to backend maintainer' do + expect(spins).to contain_exactly(matching_spin(:engineering_productivity, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + end + end + + context 'when change contains test category' do + let(:categories) { [:test] } + + it 'assigns corresponding SET' do + expect(spins).to contain_exactly(matching_spin(:test, reviewer: software_engineer_in_test)) + end end end - context 'when change contains test category' do - it 'assigns corresponding SET and sets optional test maintainer' do - categories = [:test] - spins = subject.spin(project, categories, branch_name) + context 'when timezone_experiment == true' do + let(:timezone_experiment) { true } + + context 'when change contains backend category' do + let(:categories) { [:backend] } + + it 'assigns backend reviewer and maintainer' do + expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + end + + context 'when teammate is not in a good timezone' do + before do + backend_maintainer[:tz_offset_hours] = 5.0 + end - expect(spins).to contain_exactly(matching_spin(:test, reviewer: software_engineer_in_test, optional: :maintainer)) + it 'assigns backend reviewer and no maintainer' do + expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil)) + end + end + end + + context 'when change includes a category with timezone disabled' do + let(:categories) { [:backend] } + + before do + stub_const("#{described_class}::INCLUDE_TIMEZONE_FOR_CATEGORY", backend: false) + end + + it 'assigns backend reviewer and maintainer' do + expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + end + + context 'when teammate is not in a good timezone' do + before do + backend_maintainer[:tz_offset_hours] = 5.0 + end + + it 'assigns backend reviewer and maintainer' do + expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer)) + end + end end end end @@ -217,51 +286,83 @@ describe Gitlab::Danger::Roulette do end describe '#spin_for_person' do - let(:person1) { Gitlab::Danger::Teammate.new('username' => 'rymai') } - let(:person2) { Gitlab::Danger::Teammate.new('username' => 'godfat') } - let(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') } - let(:ooo) { Gitlab::Danger::Teammate.new('username' => 'jacopo-beschi') } - let(:no_capacity) { Gitlab::Danger::Teammate.new('username' => 'uncharged') } + let(:person_tz_offset_hours) { 0.0 } + let(:person1) do + Gitlab::Danger::Teammate.new( + 'username' => 'rymai', + 'available' => true, + 'tz_offset_hours' => person_tz_offset_hours + ) + end + let(:person2) do + Gitlab::Danger::Teammate.new( + 'username' => 'godfat', + 'available' => true, + 'tz_offset_hours' => person_tz_offset_hours) + end + let(:author) do + Gitlab::Danger::Teammate.new( + 'username' => 'filipa', + 'available' => true, + 'tz_offset_hours' => 0.0) + end + let(:unavailable) do + Gitlab::Danger::Teammate.new( + 'username' => 'jacopo-beschi', + 'available' => false, + 'tz_offset_hours' => 0.0) + end before do - stub_person_status(person1, message: 'making GitLab magic') - stub_person_status(person2, message: 'making GitLab magic') - stub_person_status(ooo, message: 'OOO till 15th') - stub_person_status(no_capacity, message: 'At capacity for the next few days', emoji: 'red_circle') - # we don't stub Filipa, as she is the author and - # we should not fire request checking for her - allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username) end - it 'returns a random person' do - persons = [person1, person2] + (-4..4).each do |utc_offset| + context "when local hour for person is #{10 + utc_offset} (offset: #{utc_offset})" do + let(:person_tz_offset_hours) { utc_offset } - selected = subject.spin_for_person(persons, random: Random.new) + [false, true].each do |timezone_experiment| + context "with timezone_experiment == #{timezone_experiment}" do + it 'returns a random person' do + persons = [person1, person2] - expect(selected.username).to be_in(persons.map(&:username)) - end + selected = subject.spin_for_person(persons, random: Random.new, timezone_experiment: timezone_experiment) - it 'excludes OOO persons' do - expect(subject.spin_for_person([ooo], random: Random.new)).to be_nil + expect(selected.username).to be_in(persons.map(&:username)) + end + end + end + end end - it 'excludes mr.author' do - expect(subject.spin_for_person([author], random: Random.new)).to be_nil + ((-12..-5).to_a + (5..12).to_a).each do |utc_offset| + context "when local hour for person is #{10 + utc_offset} (offset: #{utc_offset})" do + let(:person_tz_offset_hours) { utc_offset } + + [false, true].each do |timezone_experiment| + context "with timezone_experiment == #{timezone_experiment}" do + it 'returns a random person or nil' do + persons = [person1, person2] + + selected = subject.spin_for_person(persons, random: Random.new, timezone_experiment: timezone_experiment) + + if timezone_experiment + expect(selected).to be_nil + else + expect(selected.username).to be_in(persons.map(&:username)) + end + end + end + end + end end - it 'excludes person with no capacity' do - expect(subject.spin_for_person([no_capacity], random: Random.new)).to be_nil + it 'excludes unavailable persons' do + expect(subject.spin_for_person([unavailable], random: Random.new)).to be_nil end - end - private - - def stub_person_status(person, message: 'dummy message', emoji: 'unicorn') - body = { message: message, emoji: emoji }.to_json - - WebMock - .stub_request(:get, "https://gitlab.com/api/v4/users/#{person.username}/status") - .to_return(body: body) + it 'excludes mr.author' do + expect(subject.spin_for_person([author], random: Random.new)).to be_nil + end end end diff --git a/spec/lib/gitlab/danger/sidekiq_queues_spec.rb b/spec/lib/gitlab/danger/sidekiq_queues_spec.rb new file mode 100644 index 00000000000..7dd1a2e6924 --- /dev/null +++ b/spec/lib/gitlab/danger/sidekiq_queues_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' +require_relative 'danger_spec_helper' + +require 'gitlab/danger/sidekiq_queues' + +RSpec.describe Gitlab::Danger::SidekiqQueues do + using RSpec::Parameterized::TableSyntax + include DangerSpecHelper + + let(:fake_git) { double('fake-git') } + let(:fake_danger) { new_fake_danger.include(described_class) } + + subject(:sidekiq_queues) { fake_danger.new(git: fake_git) } + + describe '#changed_queue_files' do + where(:modified_files, :changed_queue_files) do + %w(app/workers/all_queues.yml ee/app/workers/all_queues.yml foo) | %w(app/workers/all_queues.yml ee/app/workers/all_queues.yml) + %w(app/workers/all_queues.yml ee/app/workers/all_queues.yml) | %w(app/workers/all_queues.yml ee/app/workers/all_queues.yml) + %w(app/workers/all_queues.yml foo) | %w(app/workers/all_queues.yml) + %w(ee/app/workers/all_queues.yml foo) | %w(ee/app/workers/all_queues.yml) + %w(foo) | %w() + %w() | %w() + end + + with_them do + it do + allow(fake_git).to receive(:modified_files).and_return(modified_files) + + expect(sidekiq_queues.changed_queue_files).to match_array(changed_queue_files) + end + end + end + + describe '#added_queue_names' do + it 'returns queue names added by this change' do + old_queues = { post_receive: nil } + + allow(sidekiq_queues).to receive(:old_queues).and_return(old_queues) + allow(sidekiq_queues).to receive(:new_queues).and_return(old_queues.merge(merge: nil, process_commit: nil)) + + expect(sidekiq_queues.added_queue_names).to contain_exactly(:merge, :process_commit) + end + end + + describe '#changed_queue_names' do + it 'returns names for queues whose attributes were changed' do + old_queues = { + merge: { name: :merge, urgency: :low }, + post_receive: { name: :post_receive, urgency: :high }, + process_commit: { name: :process_commit, urgency: :high } + } + + new_queues = old_queues.merge(mailers: { name: :mailers, urgency: :high }, + post_receive: { name: :post_receive, urgency: :low }, + process_commit: { name: :process_commit, urgency: :low }) + + allow(sidekiq_queues).to receive(:old_queues).and_return(old_queues) + allow(sidekiq_queues).to receive(:new_queues).and_return(new_queues) + + expect(sidekiq_queues.changed_queue_names).to contain_exactly(:post_receive, :process_commit) + end + + it 'ignores removed queues' do + old_queues = { + merge: { name: :merge, urgency: :low }, + post_receive: { name: :post_receive, urgency: :high } + } + + new_queues = { + post_receive: { name: :post_receive, urgency: :low } + } + + allow(sidekiq_queues).to receive(:old_queues).and_return(old_queues) + allow(sidekiq_queues).to receive(:new_queues).and_return(new_queues) + + expect(sidekiq_queues.changed_queue_names).to contain_exactly(:post_receive) + end + end +end diff --git a/spec/lib/gitlab/danger/teammate_spec.rb b/spec/lib/gitlab/danger/teammate_spec.rb index ea5aecbc597..a0540a9fbf5 100644 --- a/spec/lib/gitlab/danger/teammate_spec.rb +++ b/spec/lib/gitlab/danger/teammate_spec.rb @@ -2,14 +2,27 @@ require 'fast_spec_helper' +require 'timecop' require 'rspec-parameterized' require 'gitlab/danger/teammate' -describe Gitlab::Danger::Teammate do +RSpec.describe Gitlab::Danger::Teammate do + using RSpec::Parameterized::TableSyntax + subject { described_class.new(options.stringify_keys) } - let(:options) { { username: 'luigi', projects: projects, role: role } } + let(:tz_offset_hours) { 2.0 } + let(:options) do + { + username: 'luigi', + projects: projects, + role: role, + markdown_name: '[Luigi](https://gitlab.com/luigi) (`@luigi`)', + tz_offset_hours: tz_offset_hours + } + end + let(:capabilities) { ['reviewer backend'] } let(:projects) { { project => capabilities } } let(:role) { 'Engineer, Manage' } let(:labels) { [] } @@ -115,78 +128,72 @@ describe Gitlab::Danger::Teammate do end end - describe '#status' do - let(:capabilities) { ['dish washing'] } - - context 'with empty cache' do - context 'for successful request' do - it 'returns the response' do - mock_status = double(does_not: 'matter') - expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json) - .and_return(mock_status) + describe '#local_hour' do + around do |example| + Timecop.freeze(Time.utc(2020, 6, 23, 10)) { example.run } + end - expect(subject.status).to be mock_status - end + context 'when author is given' do + where(:tz_offset_hours, :expected_local_hour) do + -12 | 22 + -10 | 0 + 2 | 12 + 4 | 14 + 12 | 22 end - context 'for failing request' do - it 'returns nil' do - expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json) - .and_raise(Gitlab::Danger::RequestHelper::HTTPError.new) - - expect(subject.status).to be nil + with_them do + it 'returns the correct local_hour' do + expect(subject.local_hour).to eq(expected_local_hour) end end end + end - context 'with filled cache' do - it 'returns the cached response' do - mock_status = double(does_not: 'matter') - expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json) - .and_return(mock_status) - subject.status - - expect(Gitlab::Danger::RequestHelper).not_to receive(:http_get_json) - expect(subject.status).to be mock_status + describe '#markdown_name' do + context 'when timezone_experiment == false' do + it 'returns markdown name as-is' do + expect(subject.markdown_name).to eq(options[:markdown_name]) + expect(subject.markdown_name(timezone_experiment: false)).to eq(options[:markdown_name]) end end - end - describe '#available?' do - using RSpec::Parameterized::TableSyntax - - let(:capabilities) { ['dry head'] } - - where(:status, :result) do - {} | true - { message: 'dear reader' } | true - { message: 'OOO: massage' } | false - { message: 'love it SOOO much' } | false - { emoji: 'red_circle' } | false - { emoji: 'palm_tree' } | false - { emoji: 'beach' } | false - { emoji: 'beach_umbrella' } | false - { emoji: 'beach_with_umbrella' } | false - { emoji: nil } | true - { emoji: '' } | true - { emoji: 'dancer' } | true - end + context 'when timezone_experiment == true' do + it 'returns markdown name with timezone info' do + expect(subject.markdown_name(timezone_experiment: true)).to eq("#{options[:markdown_name]} (UTC+2)") + end + + context 'when offset is 1.5' do + let(:tz_offset_hours) { 1.5 } - with_them do - before do - expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json) - .and_return(status&.stringify_keys) + it 'returns markdown name with timezone info, not truncated' do + expect(subject.markdown_name(timezone_experiment: true)).to eq("#{options[:markdown_name]} (UTC+1.5)") + end end - it { expect(subject.available?).to be result } - end + context 'when author is given' do + where(:tz_offset_hours, :author_offset, :diff_text) do + -12 | -10 | "2 hours behind `@mario`" + -10 | -12 | "2 hours ahead `@mario`" + -10 | 2 | "12 hours behind `@mario`" + 2 | 4 | "2 hours behind `@mario`" + 4 | 2 | "2 hours ahead `@mario`" + 2 | 3 | "1 hour behind `@mario`" + 3 | 2 | "1 hour ahead `@mario`" + 2 | 2 | "same timezone as `@mario`" + end - it 'returns true if request fails' do - expect(Gitlab::Danger::RequestHelper) - .to receive(:http_get_json) - .and_raise(Gitlab::Danger::RequestHelper::HTTPError.new) + with_them do + it 'returns markdown name with timezone info' do + author = described_class.new(options.merge(username: 'mario', tz_offset_hours: author_offset).stringify_keys) - expect(subject.available?).to be true + floored_offset_hours = subject.__send__(:floored_offset_hours) + utc_offset = floored_offset_hours >= 0 ? "+#{floored_offset_hours}" : floored_offset_hours + + expect(subject.markdown_name(timezone_experiment: true, author: author)).to eq("#{options[:markdown_name]} (UTC#{utc_offset}, #{diff_text})") + end + end + end end end end |