6 files changed, 162 insertions, 81 deletions

diff --git a/changelogs/unreleased/refactor-roulette-testing.yml b/changelogs/unreleased/refactor-roulette-testing.yml
new file mode 100644
index 00000000000..02236a4c135
--- /dev/null
+++ b/changelogs/unreleased/refactor-roulette-testing.yml
@@ -0,0 +1,4 @@
+---
+title: Clean up roulette testing
+merge_request: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29939
+type: other
diff --git a/danger/plugins/roulette.rb b/danger/plugins/roulette.rb
index 7c62cff0c92..4f3d1d73428 100644
--- a/danger/plugins/roulette.rb
+++ b/danger/plugins/roulette.rb
@@ -4,7 +4,12 @@ require_relative '../../lib/gitlab/danger/roulette'
 
 module Danger
   class Roulette < Plugin
+    ROULETTE_DATA_URL = 'https://about.gitlab.com/roulette.json'
     # Put the helper code somewhere it can be tested
     include Gitlab::Danger::Roulette
+
+    def roulette_data
+      http_get_json(ROULETTE_DATA_URL)
+    end
   end
 end
diff --git a/lib/gitlab/danger/roulette.rb b/lib/gitlab/danger/roulette.rb
index 25de0a87c9d..2c7826f0302 100644
--- a/lib/gitlab/danger/roulette.rb
+++ b/lib/gitlab/danger/roulette.rb
@@ -1,29 +1,24 @@
 # frozen_string_literal: true
 
-require 'net/http'
-require 'json'
 require 'cgi'
 
-require_relative 'teammate'
+require_dependency 'gitlab/get_json'
+require_dependency 'gitlab/danger/teammate'
 
+# To use this module, you need to implement a `roulette_data :: Array<Hash>` method, which
+# returns the data needed to play reviewer roulette.
+#
+# For an example of this, see: `danger/plugins/roulette.rb`
 module Gitlab
   module Danger
     module Roulette
-      ROULETTE_DATA_URL = 'https://about.gitlab.com/roulette.json'
-      HTTPError = Class.new(RuntimeError)
-
+      include ::Gitlab::GetJSON
       # Looks up the current list of GitLab team members and parses it into a
       # useful form
       #
       # @return [Array<Teammate>]
       def team
-        @team ||=
-          begin
-            data = http_get_json(ROULETTE_DATA_URL)
-            data.map { |hash| ::Gitlab::Danger::Teammate.new(hash) }
-          rescue JSON::ParserError
-            raise "Failed to parse JSON response from #{ROULETTE_DATA_URL}"
-          end
+        @team ||= roulette_data.map { |hash| ::Gitlab::Danger::Teammate.new(hash) }
       end
 
       # Like +team+, but only returns teammates in the current project, based on
@@ -64,19 +59,9 @@ module Gitlab
         api_endpoint = "https://gitlab.com/api/v4/users/#{username}/status"
         response = http_get_json(api_endpoint)
         response["message"]&.match?(/OOO/i)
-      rescue HTTPError, JSON::ParserError
+      rescue Gitlab::GetJSON::Error
         false # this is no worse than not checking for OOO
       end
-
-      def http_get_json(url)
-        rsp = Net::HTTP.get_response(URI.parse(url))
-
-        unless rsp.is_a?(Net::HTTPSuccess)
-          raise HTTPError, "Failed to read #{url}: #{rsp.code} #{rsp.message}"
-        end
-
-        JSON.parse(rsp.body)
-      end
     end
   end
 end
diff --git a/lib/gitlab/get_json.rb b/lib/gitlab/get_json.rb
new file mode 100644
index 00000000000..215767b6156
--- /dev/null
+++ b/lib/gitlab/get_json.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'json'
+require 'faraday'
+
+# Gitlab::GetJSON is a very thin layer over Faraday to `GET` a resource
+# at a URL, and decode it into JSON. HTTP errors and JSON decoding errors
+# are treated as exceptional states and errors are thrown.
+module Gitlab
+  module GetJSON
+    Error = Class.new(StandardError)
+    HTTPError = Class.new(::Gitlab::GetJSON::Error)
+    JSONError = Class.new(::Gitlab::GetJSON::Error)
+
+    def http_get_json(url)
+      rsp = Faraday.get(url)
+
+      unless rsp.success?
+        raise HTTPError, "Failed to read #{url}: #{rsp.status}"
+      end
+
+      JSON.parse(rsp.body)
+    rescue Faraday::ConnectionFailed
+      raise HTTPError, "Could not connect to #{url}"
+    rescue JSON::ParserError
+      raise JSONError, "Failed to parse JSON response from #{url}"
+    end
+  end
+end
diff --git a/spec/lib/gitlab/danger/roulette_spec.rb b/spec/lib/gitlab/danger/roulette_spec.rb
index 121c5d8ecd9..40908556207 100644
--- a/spec/lib/gitlab/danger/roulette_spec.rb
+++ b/spec/lib/gitlab/danger/roulette_spec.rb
@@ -6,81 +6,69 @@ require 'webmock/rspec'
 require 'gitlab/danger/roulette'
 
 describe Gitlab::Danger::Roulette do
-  let(:teammate_json) do
-    <<~JSON
+  class MockRoulette
+    include Gitlab::Danger::Roulette
+  end
+
+  let(:teammate_data) do
     [
       {
-        "username": "in-gitlab-ce",
-        "name": "CE maintainer",
-        "projects":{ "gitlab-ce": "maintainer backend" }
+        "username" => "in-gitlab-ce",
+        "name" => "CE maintainer",
+        "projects" => { "gitlab-ce" => "maintainer backend" }
       },
       {
-        "username": "in-gitlab-ee",
-        "name": "EE reviewer",
-        "projects":{ "gitlab-ee": "reviewer frontend" }
+        "username" => "in-gitlab-ee",
+        "name" => "EE reviewer",
+        "projects" => { "gitlab-ee" => "reviewer frontend" }
       }
     ]
-    JSON
+  end
+
+  before do
+    allow(roulette).to receive(:roulette_data) { teammate_data }
   end
 
   let(:ce_teammate_matcher) do
-    satisfy do |teammate|
-      teammate.username == 'in-gitlab-ce' &&
-        teammate.name == 'CE maintainer' &&
-        teammate.projects == { 'gitlab-ce' => 'maintainer backend' }
-    end
+    have_attributes(
+      username: 'in-gitlab-ce',
+      name: 'CE maintainer',
+      projects: { 'gitlab-ce' => 'maintainer backend' })
   end
 
   let(:ee_teammate_matcher) do
-    satisfy do |teammate|
-      teammate.username == 'in-gitlab-ee' &&
-        teammate.name == 'EE reviewer' &&
-        teammate.projects == { 'gitlab-ee' => 'reviewer frontend' }
-    end
+    have_attributes(
+      username: 'in-gitlab-ee',
+      name: 'EE reviewer',
+      projects: { 'gitlab-ee' => 'reviewer frontend' })
   end
 
-  subject(:roulette) { Object.new.extend(described_class) }
+  subject(:roulette) { MockRoulette.new }
 
+  # We don't need to test that `http_get_json` does what it says it does - it
+  # is not our code after all. Since we are propagating errors, we just need to
+  # make sure we don't swallow them.
   describe '#team' do
     subject(:team) { roulette.team }
 
-    context 'HTTP failure' do
-      before do
-        WebMock
-          .stub_request(:get, described_class::ROULETTE_DATA_URL)
-          .to_return(status: 404)
-      end
-
-      it 'raises a pretty error' do
-        expect { team }.to raise_error(/Failed to read/)
-      end
-    end
-
-    context 'JSON failure' do
-      before do
-        WebMock
-          .stub_request(:get, described_class::ROULETTE_DATA_URL)
-          .to_return(body: 'INVALID JSON')
+    context 'on error' do
+      let(:teammate_data) do
+        raise "BOOM!"
       end
 
-      it 'raises a pretty error' do
-        expect { team }.to raise_error(/Failed to parse/)
+      it 'propagates the error' do
+        expect { team }.to raise_error(/BOOM/)
       end
     end
 
     context 'success' do
-      before do
-        WebMock
-          .stub_request(:get, described_class::ROULETTE_DATA_URL)
-          .to_return(body: teammate_json)
-      end
-
       it 'returns an array of teammates' do
         is_expected.to contain_exactly(ce_teammate_matcher, ee_teammate_matcher)
       end
 
       it 'memoizes the result' do
-        expect(team.object_id).to eq(roulette.team.object_id)
+        expect(roulette).to receive(:roulette_data).at_most(:once)
+        expect(team).to eq(roulette.team)
       end
     end
   end
@@ -88,12 +76,6 @@ describe Gitlab::Danger::Roulette do
   describe '#project_team' do
     subject { roulette.project_team('gitlab-ce') }
 
-    before do
-      WebMock
-        .stub_request(:get, described_class::ROULETTE_DATA_URL)
-        .to_return(body: teammate_json)
-    end
-
     it 'filters team by project_name' do
       is_expected.to contain_exactly(ce_teammate_matcher)
     end
@@ -117,22 +99,29 @@ describe Gitlab::Danger::Roulette do
 
     it 'returns a random person' do
       persons = [person1, person2]
+      names = persons.map(&:username)
+      expect(subject).to receive(:out_of_office?)
+        .exactly(:once)
+        .and_call_original
 
-      selected = subject.spin_for_person(persons, random: Random.new)
-
-      expect(selected.username).to be_in(persons.map(&:username))
+      expect(spin(persons)).to have_attributes(username: be_in(names))
     end
 
     it 'excludes OOO persons' do
-      expect(subject.spin_for_person([ooo], random: Random.new)).to be_nil
+      expect(spin([ooo])).to be_nil
     end
 
     it 'excludes mr.author' do
-      expect(subject.spin_for_person([author], random: Random.new)).to be_nil
+      expect(subject).not_to receive(:out_of_office?)
+      expect(spin([author])).to be_nil
     end
 
     private
 
+    def spin(people)
+      subject.spin_for_person(people, random: Random.new)
+    end
+
     def stub_person_message(person, message)
       body = { message: message }.to_json
 
diff --git a/spec/lib/gitlab/get_json_spec.rb b/spec/lib/gitlab/get_json_spec.rb
new file mode 100644
index 00000000000..48dc6cf192e
--- /dev/null
+++ b/spec/lib/gitlab/get_json_spec.rb
@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'ostruct'
+
+require 'gitlab/get_json'
+
+describe Gitlab::GetJSON do
+  subject do
+    mod = described_class
+    Class.new { include mod }.new
+  end
+
+  before do
+    allow(Faraday).to receive(:get) { response }
+  end
+
+  context 'the response is successful' do
+    let(:response) do
+      OpenStruct.new(success?: true, body: data.to_json)
+    end
+
+    let(:data) do
+      {
+        a: true,
+        b: 3.14159,
+        c: %w[one two three]
+      }
+    end
+
+    it 'returns the data back to us' do
+      expect(subject.http_get_json('some-url')).to eq data.transform_keys(&:to_s)
+    end
+  end
+
+  context 'we cannot connect' do
+    let(:response) do
+      raise Faraday::ConnectionFailed, 'for some reason'
+    end
+
+    it 'raises an HTTPError' do
+      expect { subject.http_get_json('url') }.to raise_error(Gitlab::GetJSON::HTTPError)
+    end
+  end
+
+  context 'the response has bad JSON data' do
+    let(:response) do
+      OpenStruct.new(success?: true, body: 'I am not valid JSON')
+    end
+
+    it 'raises a JSONError' do
+      expect { subject.http_get_json('url') }.to raise_error(Gitlab::GetJSON::JSONError)
+    end
+  end
+
+  context 'the response is 404' do
+    let(:response) do
+      OpenStruct.new(success?: false, status: 'wibble')
+    end
+
+    it 'raises an HTTPError' do
+      expect { subject.http_get_json('wobble') }.to raise_error(Gitlab::GetJSON::HTTPError)
+    end
+
+    it 'raises an error that mentions the status' do
+      expect { subject.http_get_json('wobble') }.to raise_error(/wibble/)
+    end
+  end
+end