Clean up roulette testingrefactor-roulette-testing

This factors out the IO into its own testable module.

The advantage of this is that we can easily instantiate versions of the
plugin that have different team sources, without extensive mocking.
This proved to be necessary to test an update to the `roulette.json`
production in the handbook.

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