Auto merge of #4227 - RochesterinNYC:add-unit-tests-for-bundler-fetcher-dependency, r=indirect

Refactor and add unit test coverage for `Bundler::Fetcher::Dependency`

- Extract some parts of `#dependency_specs` and `#specs` into more modular sub-methods

This class was considerably more difficult to unit test out than the previous ones I've dealt with so far, even after breaking up some of the more monolithic methods. Bit of a code smell? Although, it does seem that the complexity of this class puts it among the more complex classes (functionality-wise, not necessarily implementation-wise) in the Bundler codebase.

2 files changed, 360 insertions, 13 deletions

diff --git a/lib/bundler/fetcher/dependency.rb b/lib/bundler/fetcher/dependency.rb
index 84868ff719..29af0dc318 100644
--- a/lib/bundler/fetcher/dependency.rb
+++ b/lib/bundler/fetcher/dependency.rb
@@ -21,38 +21,40 @@ module Bundler
       def specs(gem_names, full_dependency_list = [], last_spec_list = [])
         query_list = gem_names - full_dependency_list
 
-        # only display the message on the first run
-        if Bundler.ui.debug?
-          Bundler.ui.debug "Query List: #{query_list.inspect}"
-        else
-          Bundler.ui.info ".", false
-        end
+        log_specs(query_list)
 
         return { remote_uri => last_spec_list } if query_list.empty?
 
-        remote_specs = Bundler::Retry.new("dependency api", AUTH_ERRORS).attempts do
+        spec_list, deps_list = Bundler::Retry.new("dependency api", AUTH_ERRORS).attempts do
           dependency_specs(query_list)
         end
 
-        spec_list, deps_list = remote_specs
         returned_gems = spec_list.map(&:first).uniq
         specs(deps_list, full_dependency_list + returned_gems, spec_list + last_spec_list)
       rescue HTTPError, MarshalError, GemspecError
         Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over
         Bundler.ui.debug "could not fetch from the dependency API, trying the full index"
-        return nil
+        nil
       end
 
       def dependency_specs(gem_names)
         Bundler.ui.debug "Query Gemcutter Dependency Endpoint API: #{gem_names.join(",")}"
-        gem_list = []
-        deps_list = []
 
+        gem_list = unmarshalled_dep_gems(gem_names)
+        get_formatted_specs_and_deps(gem_list)
+      end
+
+      def unmarshalled_dep_gems(gem_names)
+        gem_list = []
         gem_names.each_slice(Source::Rubygems::API_REQUEST_SIZE) do |names|
           marshalled_deps = downloader.fetch dependency_api_uri(names)
           gem_list += Bundler.load_marshal(marshalled_deps)
         end
+        gem_list
+      end
 
+      def get_formatted_specs_and_deps(gem_list)
+        deps_list = []
         spec_list = gem_list.map do |s|
           dependencies = s[:dependencies].map do |name, requirement|
             dep = well_formed_dependency(name, requirement.split(", "))
@@ -62,7 +64,6 @@ module Bundler
 
           [s[:name], Gem::Version.new(s[:number]), s[:platform], dependencies]
         end
-
         [spec_list, deps_list.uniq]
       end
 
@@ -79,10 +80,21 @@ module Bundler
         raise e unless e.message.include?(illformed)
         puts # we shouldn't print the error message on the "fetching info" status line
         raise GemspecError,
-          "Unfortunately, the gem #{s[:name]} (#{s[:number]}) has an invalid " \
+          "Unfortunately, the gem #{name} has an invalid " \
           "gemspec. \nPlease ask the gem author to yank the bad version to fix " \
           "this issue. For more information, see http://bit.ly/syck-defaultkey."
       end
+
+    private
+
+      def log_specs(query_list)
+        # only display the message on the first run
+        if Bundler.ui.debug?
+          Bundler.ui.debug "Query List: #{query_list.inspect}"
+        else
+          Bundler.ui.info ".", false
+        end
+      end
     end
   end
 end
diff --git a/spec/bundler/fetcher/dependency_spec.rb b/spec/bundler/fetcher/dependency_spec.rb
new file mode 100644
index 0000000000..64c3902cfa
--- /dev/null
+++ b/spec/bundler/fetcher/dependency_spec.rb
@@ -0,0 +1,335 @@
+require "spec_helper"
+
+describe Bundler::Fetcher::Dependency do
+  let(:downloader)  { double(:downloader) }
+  let(:remote)      { nil }
+  let(:display_uri) { "http://sample_uri.com" }
+
+  subject { described_class.new(downloader, remote, display_uri) }
+
+  describe "#api_available?" do
+    let(:dependency_api_uri) { double(:dependency_api_uri) }
+    let(:fetched_spec)       { double(:fetched_spec) }
+
+    before do
+      allow(subject).to receive(:dependency_api_uri).and_return(dependency_api_uri)
+      allow(downloader).to receive(:fetch).with(dependency_api_uri).and_return(fetched_spec)
+    end
+
+    it "should be truthy" do
+      expect(subject.api_available?).to be_truthy
+    end
+
+    context "when there is no network access" do
+      before do
+        allow(downloader).to receive(:fetch).with(dependency_api_uri) {
+          raise Bundler::Fetcher::NetworkDownError.new("Network Down Message")
+        }
+      end
+
+      it "should raise an HTTPError with the original message" do
+        expect { subject.api_available? }.to raise_error(Bundler::HTTPError, "Network Down Message")
+      end
+    end
+
+    context "when authentication is required" do
+      let(:remote_uri) { "http://remote_uri.org" }
+
+      before do
+        allow(downloader).to receive(:fetch).with(dependency_api_uri) {
+          raise Bundler::Fetcher::AuthenticationRequiredError.new(remote_uri)
+        }
+      end
+
+      it "should raise the original error" do
+        expect { subject.api_available? }.to raise_error(Bundler::Fetcher::AuthenticationRequiredError,
+          %r{Authentication is required for http://remote_uri.org})
+      end
+    end
+
+    context "when there is an http error" do
+      before { allow(downloader).to receive(:fetch).with(dependency_api_uri) { raise Bundler::HTTPError.new } }
+
+      it "should be falsey" do
+        expect(subject.api_available?).to be_falsey
+      end
+    end
+  end
+
+  describe "#api_fetcher?" do
+    it "should return true" do
+      expect(subject.api_fetcher?).to be_truthy
+    end
+  end
+
+  describe "#specs" do
+    let(:gem_names)            { %w(foo bar) }
+    let(:full_dependency_list) { ["bar"] }
+    let(:last_spec_list)       { [["boulder", gem_version1, "ruby", resque]] }
+    let(:auth_errors)          { double(:auth_errors) }
+    let(:bundler_retry)        { double(:bundler_retry) }
+    let(:gem_version1)         { double(:gem_version1) }
+    let(:resque)               { double(:resque) }
+    let(:remote_uri)           { "http://remote-uri.org" }
+
+    before do
+      stub_const("Bundler::Fetcher::AUTH_ERRORS", auth_errors)
+      allow(Bundler::Retry).to receive(:new).with("dependency api", auth_errors).and_return(bundler_retry)
+      allow(bundler_retry).to receive(:attempts) {|&block| block.call }
+      allow(subject).to receive(:log_specs) {}
+      allow(subject).to receive(:remote_uri).and_return(remote_uri)
+    end
+
+    context "when there are given gem names that are not in the full dependency list" do
+      let(:spec_list)        { [["top", gem_version2, "ruby", faraday]] }
+      let(:deps_list)        { [] }
+      let(:dependency_specs) { [spec_list, deps_list] }
+      let(:gem_version2)     { double(:gem_version2) }
+      let(:faraday)          { double(:faraday) }
+
+      before { allow(subject).to receive(:dependency_specs).with(["foo"]).and_return(dependency_specs) }
+
+      it "should return a hash with the remote_uri and the list of specs" do
+        expect(subject.specs(gem_names, full_dependency_list, last_spec_list)).to eq("http://remote-uri.org" => [
+          ["top", gem_version2, "ruby", faraday],
+          ["boulder", gem_version1, "ruby", resque]
+        ])
+      end
+    end
+
+    context "when all given gem names are in the full dependency list" do
+      let(:gem_names)            { ["foo"] }
+      let(:full_dependency_list) { %w(foo bar) }
+      let(:last_spec_list)       { ["boulder"] }
+
+      it "should return a hash with the remote_uri and the last spec list" do
+        expect(subject.specs(gem_names, full_dependency_list, last_spec_list)).to eq("http://remote-uri.org" => ["boulder"])
+      end
+    end
+
+    context "logging" do
+      before { allow(subject).to receive(:log_specs).and_call_original }
+
+      context "with debug on" do
+        before do
+          allow(Bundler).to receive_message_chain(:ui, :debug?).and_return(true)
+          allow(subject).to receive(:dependency_specs).with(["foo"]).and_return([[], []])
+        end
+
+        it "should log the query list at debug level" do
+          expect(Bundler).to receive_message_chain(:ui, :debug).with("Query List: [\"foo\"]")
+          expect(Bundler).to receive_message_chain(:ui, :debug).with("Query List: []")
+          subject.specs(gem_names, full_dependency_list, last_spec_list)
+        end
+      end
+
+      context "with debug off" do
+        before do
+          allow(Bundler).to receive_message_chain(:ui, :debug?).and_return(false)
+          allow(subject).to receive(:dependency_specs).with(["foo"]).and_return([[], []])
+        end
+
+        it "should log at info level" do
+          expect(Bundler).to receive_message_chain(:ui, :info).with(".", false)
+          expect(Bundler).to receive_message_chain(:ui, :info).with(".", false)
+          subject.specs(gem_names, full_dependency_list, last_spec_list)
+        end
+      end
+    end
+
+    shared_examples_for "the error is properly handled" do
+      before do
+        allow(Bundler).to receive_message_chain(:ui, :debug?)
+        allow(Bundler).to receive_message_chain(:ui, :info)
+        allow(Bundler).to receive_message_chain(:ui, :debug)
+      end
+
+      it "should return nil" do
+        expect(subject.specs(gem_names, full_dependency_list, last_spec_list)).to be_nil
+      end
+
+      it "should log the inability to fetch from API at debug level" do
+        expect(Bundler).to receive_message_chain(:ui, :debug).with("could not fetch from the dependency API, trying the full index")
+        subject.specs(gem_names, full_dependency_list, last_spec_list)
+      end
+
+      context "debug logging is not on" do
+        before { allow(Bundler).to receive_message_chain(:ui, :debug?).and_return(false) }
+
+        it "should log a new line to info" do
+          expect(Bundler).to receive_message_chain(:ui, :info).with("")
+          subject.specs(gem_names, full_dependency_list, last_spec_list)
+        end
+      end
+    end
+
+    context "when an HTTPError occurs" do
+      before { allow(subject).to receive(:dependency_specs) { raise Bundler::HTTPError.new } }
+
+      it_behaves_like "the error is properly handled"
+    end
+
+    context "when a MarshalError occurs" do
+      before { allow(subject).to receive(:dependency_specs) { raise Bundler::MarshalError.new } }
+
+      it_behaves_like "the error is properly handled"
+    end
+
+    context "when a GemspecError occurs" do
+      before { allow(subject).to receive(:dependency_specs) { raise Bundler::GemspecError.new } }
+
+      it_behaves_like "the error is properly handled"
+    end
+  end
+
+  describe "#dependency_specs" do
+    let(:gem_names)                { [%w(foo bar), %w(bundler rubocop)] }
+    let(:gem_list)                 { double(:gem_list) }
+    let(:formatted_specs_and_deps) { double(:formatted_specs_and_deps) }
+
+    before do
+      allow(subject).to receive(:unmarshalled_dep_gems).with(gem_names).and_return(gem_list)
+      allow(subject).to receive(:get_formatted_specs_and_deps).with(gem_list).and_return(formatted_specs_and_deps)
+    end
+
+    it "should log the query list at debug level" do
+      expect(Bundler).to receive_message_chain(:ui, :debug).with(
+        "Query Gemcutter Dependency Endpoint API: foo,bar,bundler,rubocop")
+      subject.dependency_specs(gem_names)
+    end
+
+    it "should return formatted specs and a unique list of dependencies" do
+      expect(subject.dependency_specs(gem_names)).to eq(formatted_specs_and_deps)
+    end
+  end
+
+  describe "#unmarshalled_dep_gems" do
+    let(:gem_names)         { [%w(foo bar), %w(bundler rubocop)] }
+    let(:dep_api_uri)       { double(:dep_api_uri) }
+    let(:marshalled_deps)   { double(:marshalled_deps) }
+    let(:unmarshalled_gems) { double(:unmarshalled_gems) }
+    let(:rubygems_limit)    { 50 }
+
+    before { allow(subject).to receive(:dependency_api_uri).with(gem_names).and_return(dep_api_uri) }
+
+    it "should fetch dependencies from Rubygems and unmarshal them" do
+      expect(gem_names).to receive(:each_slice).with(rubygems_limit).and_call_original
+      expect(downloader).to receive(:fetch).with(dep_api_uri).and_return([marshalled_deps])
+      expect(Bundler).to receive(:load_marshal).with([marshalled_deps]).and_return([unmarshalled_gems])
+      expect(subject.unmarshalled_dep_gems(gem_names)).to eq([unmarshalled_gems])
+    end
+  end
+
+  describe "#get_formatted_specs_and_deps" do
+    let(:gem_list) do
+      [
+        {
+          :dependencies => {
+            "resque" => "req3,req4"
+          },
+          :name => "typhoeus",
+          :number => "1.0.1",
+          :platform => "ruby"
+        },
+        {
+          :dependencies => {
+            "faraday" => "req1,req2"
+          },
+          :name => "grape",
+          :number => "2.0.2",
+          :platform => "jruby"
+        }
+      ]
+    end
+    let(:gem_version1) { double(:gem_version1) }
+    let(:gem_version2) { double(:gem_version2) }
+
+    before do
+      %w(faraday resque).each do |dep_name|
+        instance_variable_set(:"@#{dep_name}", double(dep_name.to_sym))
+        dep_double = instance_variable_get(:"@#{dep_name}")
+        allow(subject).to receive(:well_formed_dependency).with(dep_name, anything).and_return(dep_double)
+        allow(dep_double).to receive(:name).and_return(dep_name)
+      end
+      allow(Gem::Version).to receive(:new).with("1.0.1").and_return(gem_version1)
+      allow(Gem::Version).to receive(:new).with("2.0.2").and_return(gem_version2)
+    end
+
+    it "should return formatted specs and a unique list of dependencies" do
+      spec_list, deps_list = subject.get_formatted_specs_and_deps(gem_list)
+      expect(spec_list).to eq([
+        ["typhoeus", gem_version1, "ruby", [@resque]],
+        ["grape", gem_version2, "jruby", [@faraday]]
+      ])
+      expect(deps_list).to eq(%w(resque faraday))
+    end
+  end
+
+  describe "#dependency_api_uri" do
+    let(:uri) { URI("http://gem-api.com") }
+
+    context "with gem names" do
+      let(:gem_names) { [%w(foo bar), %w(bundler rubocop)] }
+
+      before { allow(subject).to receive(:fetch_uri).and_return(uri) }
+
+      it "should return an api calling uri with the gems in the query" do
+        expect(subject.dependency_api_uri(gem_names).to_s).to eq(
+          "http://gem-api.com/api/v1/dependencies?gems=foo%2Cbar%2Cbundler%2Crubocop")
+      end
+    end
+
+    context "with no gem names" do
+      let(:gem_names) { [] }
+
+      before { allow(subject).to receive(:fetch_uri).and_return(uri) }
+
+      it "should return an api calling uri with no query" do
+        expect(subject.dependency_api_uri(gem_names).to_s).to eq(
+          "http://gem-api.com/api/v1/dependencies")
+      end
+    end
+  end
+
+  describe "#well_formed_dependency" do
+    let(:name)           { "foo" }
+    let(:requirement1)   { "req1" }
+    let(:requirement2)   { "req2" }
+    let(:gem_dependency) { double(:gem_dependency) }
+
+    before { allow(Gem::Dependency).to receive(:new).and_return(gem_dependency) }
+
+    it "should return a Gem::Dependency" do
+      expect(Gem::Dependency).to receive(:new).with("foo", "req1", "req2")
+      subject.well_formed_dependency(name, requirement1, requirement2)
+    end
+
+    context "when an ArgumentError occurs" do
+      before do
+        allow(Gem::Dependency).to receive(:new).with("foo", "req1", "req2") {
+          raise ArgumentError.new("Some error occurred")
+        }
+      end
+
+      it "should raise the original error" do
+        expect { subject.well_formed_dependency(name, requirement1, requirement2) }.to raise_error(
+          ArgumentError, "Some error occurred")
+      end
+    end
+
+    context "when there is an ill formed requirement" do
+      before do
+        allow(Gem::Dependency).to receive(:new).with("foo", "req1", "req2") {
+          raise ArgumentError.new("Ill-formed requirement [\"#<YAML::Syck::DefaultKey")
+        }
+        # Eliminate extra line break in rspec output due to `puts` in `#well_formed_dependency`
+        allow(subject).to receive(:puts) {}
+      end
+
+      it "should raise a Bundler::GemspecError with invalid gemspec message" do
+        expect { subject.well_formed_dependency(name, requirement1, requirement2) }.to raise_error(
+          Bundler::GemspecError, /Unfortunately, the gem foo has an invalid gemspec/)
+      end
+    end
+  end
+end