Merge branch 'caalberts-optional-qa-test-maintainer-review' into 'master'

Make ~test and ~QA maintainer review optional

Closes gitlab-org/quality/team-tasks#510

See merge request gitlab-org/gitlab!33494

danger/roulette/Dangerfile

@@ -36,36 +36,22 @@ Please consider creating a merge request to
 for them.
 MARKDOWN
 
-NO_REVIEWER = 'No reviewer available'.freeze
-NO_MAINTAINER = 'No maintainer available'.freeze
+OPTIONAL_REVIEW_TEMPLATE = "%{role} review is optional for %{category}".freeze
+NOT_AVAILABLE_TEMPLATE = 'No %{role} available'.freeze
 
-Spin = Struct.new(:reviewer, :maintainer)
-
-def spin_role_for_category(team, role, project, category)
-  team.select do |member|
-    member.public_send("#{role}?", project, category, gitlab.mr_labels) # rubocop:disable GitlabSecurity/PublicSend
+def note_for_category_role(spin, role)
+  if spin.optional_role == role
+    return OPTIONAL_REVIEW_TEMPLATE % { role: role.capitalize, category: helper.label_for_category(spin.category) }
   end
-end
-
-def spin_for_category(team, project, category, branch_name)
-  reviewers, traintainers, maintainers =
-    %i[reviewer traintainer maintainer].map do |role|
-      spin_role_for_category(team, role, project, category)
-    end
 
-  # TODO: take CODEOWNERS into account?
-  # https://gitlab.com/gitlab-org/gitlab/issues/26723
-
-  # Make traintainers have triple the chance to be picked as a reviewer
-  random = roulette.new_random(branch_name)
-  reviewer = roulette.spin_for_person(reviewers + traintainers + traintainers, random: random)
-  maintainer = roulette.spin_for_person(maintainers, random: random)
-
-  Spin.new(reviewer, maintainer)
+  spin.public_send(role)&.markdown_name || NOT_AVAILABLE_TEMPLATE % { role: role } # rubocop:disable GitlabSecurity/PublicSend
 end
 
-def markdown_row_for_category(category, reviewer, maintainer)
-  "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name || NO_REVIEWER} | #{maintainer&.markdown_name || NO_MAINTAINER} |"
+def markdown_row_for_spin(spin)
+  reviewer_note = note_for_category_role(spin, :reviewer)
+  maintainer_note = note_for_category_role(spin, :maintainer)
+
+  "| #{helper.label_for_category(spin.category)} | #{reviewer_note} | #{maintainer_note} |"
 end
 
 changes = helper.changes_by_category
@@ -78,43 +64,13 @@ categories = changes.keys - [:unknown]
 categories << :database if gitlab.mr_labels.include?('database') && !categories.include?(:database)
 
 if changes.any?
-  # Strip leading and trailing CE/EE markers
-  canonical_branch_name =
-    roulette.canonical_branch_name(gitlab.mr_json['source_branch'])
-
-  team =
-    begin
-      roulette.project_team(helper.project_name)
-    rescue => err
-      warn("Reviewer roulette failed to load team data: #{err.message}")
-      []
-    end
-
   project = helper.project_name
-  unknown = changes.fetch(:unknown, [])
-  spin_per_category = categories.each_with_object({}) do |category, memo|
-    memo[category] = spin_for_category(team, project, category, canonical_branch_name)
-  end
+  branch_name = gitlab.mr_json['source_branch']
 
-  rows = spin_per_category.map do |category, spin|
-    reviewer = spin.reviewer
-    maintainer = spin.maintainer
-
-    case category
-    when :test
-      if reviewer.nil?
-        # Fetch an already picked backend reviewer, or pick one otherwise
-        reviewer = spin_per_category[:backend]&.reviewer || spin_for_category(team, project, :backend, canonical_branch_name).reviewer
-      end
-    when :engineering_productivity
-      if maintainer.nil?
-        # Fetch an already picked backend maintainer, or pick one otherwise
-        maintainer = spin_per_category[:backend]&.maintainer || spin_for_category(team, project, :backend, canonical_branch_name).maintainer
-      end
-    end
-
-    markdown_row_for_category(category, reviewer, maintainer)
-  end
+  roulette_spins = roulette.spin(project, categories, branch_name)
+  rows = roulette_spins.map { |spin| markdown_row_for_spin(spin) }
+
+  unknown = changes.fetch(:unknown, [])
 
   markdown(MESSAGE)
   markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty?

lib/gitlab/danger/roulette.rb

@@ -6,6 +6,46 @@ module Gitlab
   module Danger
     module Roulette
       ROULETTE_DATA_URL = 'https://about.gitlab.com/roulette.json'
+      OPTIONAL_CATEGORIES = [:qa, :test].freeze
+
+      Spin = Struct.new(:category, :reviewer, :maintainer, :optional_role)
+
+      # Assigns GitLab team members to be reviewer and maintainer
+      # for each change category that a Merge Request contains.
+      #
+      # @return [Array<Spin>]
+      def spin(project, categories, branch_name)
+        team =
+          begin
+            project_team(project)
+          rescue => err
+            warn("Reviewer roulette failed to load team data: #{err.message}")
+            []
+          end
+
+        canonical_branch_name = canonical_branch_name(branch_name)
+
+        spin_per_category = categories.each_with_object({}) do |category, memo|
+          memo[category] = spin_for_category(team, project, category, canonical_branch_name)
+        end
+
+        spin_per_category.map do |category, spin|
+          case category
+          when :test
+            if spin.reviewer.nil?
+              # Fetch an already picked backend reviewer, or pick one otherwise
+              spin.reviewer = spin_per_category[:backend]&.reviewer || spin_for_category(team, project, :backend, canonical_branch_name).reviewer
+            end
+          when :engineering_productivity
+            if spin.maintainer.nil?
+              # Fetch an already picked backend maintainer, or pick one otherwise
+              spin.maintainer = spin_per_category[:backend]&.maintainer || spin_for_category(team, project, :backend, canonical_branch_name).maintainer
+            end
+          end
+
+          spin
+        end
+      end
 
       # Looks up the current list of GitLab team members and parses it into a
       # useful form
@@ -58,6 +98,33 @@ module Gitlab
       def mr_author?(person)
         person.username == gitlab.mr_author
       end
+
+      def spin_role_for_category(team, role, project, category)
+        team.select do |member|
+          member.public_send("#{role}?", project, category, gitlab.mr_labels) # rubocop:disable GitlabSecurity/PublicSend
+        end
+      end
+
+      def spin_for_category(team, project, category, branch_name)
+        reviewers, traintainers, maintainers =
+          %i[reviewer traintainer maintainer].map do |role|
+            spin_role_for_category(team, role, project, category)
+          end
+
+        # TODO: take CODEOWNERS into account?
+        # https://gitlab.com/gitlab-org/gitlab/issues/26723
+
+        # Make traintainers have triple the chance to be picked as a reviewer
+        random = new_random(branch_name)
+        reviewer = spin_for_person(reviewers + traintainers + traintainers, random: random)
+        maintainer = spin_for_person(maintainers, random: random)
+
+        Spin.new(category, reviewer, maintainer).tap do |spin|
+          if OPTIONAL_CATEGORIES.include?(category)
+            spin.optional_role = :maintainer
+          end
+        end
+      end
     end
   end
 end

spec/lib/gitlab/danger/roulette_spec.rb

@@ -6,40 +6,149 @@ require 'webmock/rspec'
 require 'gitlab/danger/roulette'
 
 describe Gitlab::Danger::Roulette do
+  let(:backend_maintainer) do
+    {
+      username: 'backend-maintainer',
+      name: 'Backend maintainer',
+      role: 'Backend engineer',
+      projects: { 'gitlab' => 'maintainer backend' }
+    }
+  end
+  let(:frontend_reviewer) do
+    {
+      username: 'frontend-reviewer',
+      name: 'Frontend reviewer',
+      role: 'Frontend engineer',
+      projects: { 'gitlab' => 'reviewer frontend' }
+    }
+  end
+  let(:frontend_maintainer) do
+    {
+      username: 'frontend-maintainer',
+      name: 'Frontend maintainer',
+      role: 'Frontend engineer',
+      projects: { 'gitlab' => "maintainer frontend" }
+    }
+  end
+  let(:software_engineer_in_test) do
+    {
+      username: 'software-engineer-in-test',
+      name: 'Software Engineer in Test',
+      role: 'Software Engineer in Test, Create:Source Code',
+      projects: {
+        'gitlab' => 'reviewer qa',
+        'gitlab-qa' => 'maintainer'
+      }
+    }
+  end
+  let(:engineering_productivity_reviewer) do
+    {
+      username: 'eng-prod-reviewer',
+      name: 'EP engineer',
+      role: 'Engineering Productivity',
+      projects: { 'gitlab' => 'reviewer backend' }
+    }
+  end
+
   let(:teammate_json) do
-    <<~JSON
     [
-      {
-        "username": "in-gitlab-ce",
-        "name": "CE maintainer",
-        "projects":{ "gitlab-ce": "maintainer backend" }
-      },
-      {
-        "username": "in-gitlab-ee",
-        "name": "EE reviewer",
-        "projects":{ "gitlab-ee": "reviewer frontend" }
-      }
-    ]
-    JSON
+      backend_maintainer,
+      frontend_maintainer,
+      frontend_reviewer,
+      software_engineer_in_test,
+      engineering_productivity_reviewer
+    ].to_json
   end
 
-  let(:ce_teammate_matcher) do
+  subject(:roulette) { Object.new.extend(described_class) }
+
+  def matching_teammate(person)
     satisfy do |teammate|
-      teammate.username == 'in-gitlab-ce' &&
-        teammate.name == 'CE maintainer' &&
-        teammate.projects == { 'gitlab-ce' => 'maintainer backend' }
+      teammate.username == person[:username] &&
+        teammate.name == person[:name] &&
+        teammate.role == person[:role] &&
+        teammate.projects == person[:projects]
     end
   end
 
-  let(:ee_teammate_matcher) do
-    satisfy do |teammate|
-      teammate.username == 'in-gitlab-ee' &&
-        teammate.name == 'EE reviewer' &&
-        teammate.projects == { 'gitlab-ee' => 'reviewer frontend' }
+  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
     end
   end
 
-  subject(:roulette) { Object.new.extend(described_class) }
+  describe '#spin' do
+    let!(:project) { 'gitlab' }
+    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
+
+      WebMock
+        .stub_request(:get, described_class::ROULETTE_DATA_URL)
+        .to_return(body: teammate_json)
+      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)
+
+        expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
+      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)
+
+        expect(spins).to contain_exactly(matching_spin(:frontend, reviewer: frontend_reviewer, maintainer: frontend_maintainer))
+      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)
+
+        expect(spins).to contain_exactly(matching_spin(:qa, reviewer: software_engineer_in_test, optional: :maintainer))
+      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)
+
+        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
+      it 'assigns corresponding SET and sets optional test maintainer' do
+        categories = [:test]
+        spins = subject.spin(project, categories, branch_name)
+
+        expect(spins).to contain_exactly(matching_spin(:test, reviewer: software_engineer_in_test, optional: :maintainer))
+      end
+    end
+  end
 
   describe '#team' do
     subject(:team) { roulette.team }
@@ -76,7 +185,15 @@ describe Gitlab::Danger::Roulette do
       end
 
       it 'returns an array of teammates' do
-        is_expected.to contain_exactly(ce_teammate_matcher, ee_teammate_matcher)
+        expected_teammates = [
+          matching_teammate(backend_maintainer),
+          matching_teammate(frontend_reviewer),
+          matching_teammate(frontend_maintainer),
+          matching_teammate(software_engineer_in_test),
+          matching_teammate(engineering_productivity_reviewer)
+        ]
+
+        is_expected.to contain_exactly(*expected_teammates)
       end
 
       it 'memoizes the result' do
@@ -86,7 +203,7 @@ describe Gitlab::Danger::Roulette do
   end
 
   describe '#project_team' do
-    subject { roulette.project_team('gitlab-ce') }
+    subject { roulette.project_team('gitlab-qa') }
 
     before do
       WebMock
@@ -95,7 +212,7 @@ describe Gitlab::Danger::Roulette do
     end
 
     it 'filters team by project_name' do
-      is_expected.to contain_exactly(ce_teammate_matcher)
+      is_expected.to contain_exactly(matching_teammate(software_engineer_in_test))
     end
   end
 
@@ -136,15 +253,15 @@ describe Gitlab::Danger::Roulette do
     it 'excludes person with no capacity' do
       expect(subject.spin_for_person([no_capacity], random: Random.new)).to be_nil
     end
+  end
 
-    private
+  private
 
-    def stub_person_status(person, message: 'dummy message', emoji: 'unicorn')
-      body = { message: message, emoji: emoji }.to_json
+  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)
-    end
+    WebMock
+      .stub_request(:get, "https://gitlab.com/api/v4/users/#{person.username}/status")
+      .to_return(body: body)
   end
 end