Code review comments, additional testing

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

components/chef-workstation/lib/chef-workstation/cli.rb

@@ -155,14 +155,7 @@ def handle_perform_error(e)
       id = e.respond_to?(:id) ? e.id : e.class.to_s
       message = e.respond_to?(:message) ? e.message : e.to_s
       Telemetry.capture(:error, exception: { id: id, message: message })
-      # TODO: connection assignment below won't work, because the target host is internal the
-      #       action that failed. We can work around this for CW::Error-derived errors by accepting target host
-      #       in the constructor; but we still need to find a happy path for third-party errors
-      #       (train, runtime) - perhaps moving target host tracking and lookup to its own component
-      #
-      # #target = @cmd.nil? ? nil : @cmd.target_host
-      target_host = nil
-      wrapper = ChefWorkstation::WrappedError.new(e, target_host)
+      wrapper = ChefWorkstation::StandardErrorResolver.wrap_exception(e)
       capture_exception_backtrace(wrapper)
       # Now that our housekeeping is done, allow user-facing handling/formatting
       # in `run` to execute by re-raising

components/chef-workstation/lib/chef-workstation/command/base.rb

@@ -81,7 +81,7 @@ def run(params)
       # while providing visual feedback via the Terminal API.
       def connect_target(target_host, reporter = nil)
         if reporter.nil?
-          UI::Terminal.spinner(T.status.connecting, prefix: "[#{target_host.config[:host]}]") do |rep|
+          UI::Terminal.render_job(T.status.connecting, prefix: "[#{target_host.config[:host]}]") do |rep|
             target_host.connect!
             rep.success(T.status.connected)
           end
@@ -115,6 +115,18 @@ def usage
         self.class.usage
       end
 
+      # When running multiple jobs, exceptions are captured to the
+      # job to avoid interrupting other jobs in process.  This function
+      # collects them and raises a MultiJobFailure if failure has occurred;
+      # we do *not* differentiate between one failed jobs and multiple failed jobs
+      # - if you're in the 'multi-job' path (eg, multiple targets) we handle
+      # all errors the same to provide a consistent UX when running with mulitiple targets.
+      def handle_job_failures(jobs)
+        failed_jobs = jobs.select { |j| !j.exception.nil? }
+        return if failed_jobs.empty?
+        raise ChefWorkstation::MultiJobFailure.new(failed_jobs)
+      end
+
       private
 
       # TODO - does this all just belong in a HelpFormatter? Seems weird
@@ -191,18 +203,6 @@ def show_help_aliases
         end
       end
 
-      # When running multiple jobs, exceptions are captured to the
-      # job to avoid interrupting other jobs in process.  This function
-      # collects them and raises a MultiJobFailure if failure has occurred;
-      # we do *not* differentiate between one failed jobs and multiple failed jobs
-      # - if you're in the 'multi-job' path (eg, multiple targets) we handle
-      # all errors the same to provide a consistent experience.
-      def handle_job_failures(jobs)
-        failed_jobs = jobs.select { |j| !j.exception.nil? }
-        return if failed_jobs.empty?
-        raise ChefWorkstation::MultiJobFailure.new(failed_jobs)
-      end
-
       def subcommands
         # The base class behavior subcommands are actually the full list
         # of top-level commands - those are subcommands of 'chef'.

components/chef-workstation/lib/chef-workstation/error.rb

@@ -61,33 +61,42 @@ def initialize(jobs)
     end
   end
 
-
   # Provides mappings of common errors that we don't explicitly
   # handle, but can offer expanded help text around.
   class StandardErrorResolver
-    def self.unwrap_exception(wrapper)
+
+    def self.resolve_exception(exception)
       deps
       show_log = true
       show_stack = true
-      case wrapper.contained_exception
+      case exception
       when OpenSSL::SSL::SSLError
-        if wrapper.contained_exception.message =~ /SSL.*verify failed.*/
+        if exception.message =~ /SSL.*verify failed.*/
           id = "CHEFNET002"
           show_log = false
           show_stack = false
         end
       when SocketError then id = "CHEFNET001"; show_log = false; show_stack = false
       end
       if id.nil?
-        wrapper.contained_exception
+        exception
       else
-        e = ChefWorkstation::Error.new(id, wrapper.contained_exception.message)
+        e = ChefWorkstation::Error.new(id, exception.message)
         e.show_log = show_log
         e.show_stack = show_stack
         e
       end
     end
 
+    def self.wrap_exception(original, target_host = nil)
+      resolved_exception = resolve_exception(original)
+      WrappedError.new(resolved_exception, target_host)
+    end
+
+    def self.unwrap_exception(wrapper)
+      resolve_exception(wrapper.contained_exception)
+    end
+
     def self.deps
       # Avoid loading additional includes until they're needed
       require "socket"

components/chef-workstation/lib/chef-workstation/ui/error_printer.rb

@@ -50,21 +50,15 @@ def self.capture_multiple_failures(e)
       e.params << out_file # Tell the operator where to find this info
       File.open(out_file, "w") do |out|
         e.jobs.each do |j|
-          # ErrorPrinter only instantiates with a WrappedError:
-          wrapped = ChefWorkstation::WrappedError.new(j.exception, j.target_host)
-          # This is silly, but necessary 'til we clean up (or do away with)
-          # wrapped errors because 'unwrap_exception' does further processing to resolve
-          # the error
-          unwrapped = ChefWorkstation::StandardErrorResolver.unwrap_exception(wrapped)
-          ep = ErrorPrinter.new(wrapped, unwrapped)
-          msg = ep.format_body().tr("\n", " ").gsub(/ {2,}/, " ")
+          wrapped = ChefWorkstation::StandardErrorResolver.wrap_exception(j.exception, j.target_host)
+          ep = ErrorPrinter.new(wrapped)
+          msg = ep.format_body().tr("\n", " ").gsub(/ {2,}/, " ").chomp.strip
           out.write("Host: #{j.target_host.hostname} ")
-          if unwrapped.respond_to? :id
-            out.write("Error: #{unwrapped.id}: ")
+          if ep.exception.respond_to? :id
+            out.write("Error: #{ep.exception.id}: ")
           else
             out.write(": ")
           end
-
           out.write("#{msg}\n")
         end
       end

components/chef-workstation/spec/unit/command/base_spec.rb

@@ -17,6 +17,7 @@
 require "spec_helper"
 require "chef-workstation/command/base"
 require "chef-workstation/commands_map"
+require "chef-workstation/error"
 
 RSpec.describe ChefWorkstation::Command::Base do
   let(:cmd_spec) { instance_double(ChefWorkstation::CommandsMap::CommandSpec, name: "cmd", subcommands: []) }
@@ -48,4 +49,28 @@
       end
     end
   end
+
+  describe "#handle_job_failures" do
+    let(:passing_job) { double("PassingJob", exception: nil) }
+    let(:failing_job_1) { double("FailingJob1", exception: "failed 1") }
+    let(:failing_job_2) { double("FailingJob2", exception: "failed 2") }
+
+    context "when all jobs pass" do
+      let(:jobs) { [ passing_job, passing_job] }
+      it "returns without raising any exception" do
+        subject.handle_job_failures(jobs)
+      end
+    end
+
+    context "when at least one job fails" do
+      let(:jobs) { [ failing_job_1, passing_job, failing_job_2 ] }
+      it "raises a MulitJobFailure containing the failed jobs" do
+        expect { subject.handle_job_failures(jobs) }.to raise_error(ChefWorkstation::MultiJobFailure) do |e|
+          expect(e.jobs).to eq [failing_job_1, failing_job_2]
+        end
+      end
+    end
+
+  end
+
 end

components/chef-workstation/spec/unit/fixtures/multi-error.out

@@ -0,0 +1,2 @@
+Host: host1 Error: CHEFUPL002: Uploading resource to target failed.
+Host: host2 : An unexpected error has occurred: Hello World

components/chef-workstation/spec/unit/ui/error_printer_spec.rb

@@ -48,8 +48,56 @@
     end
   end
 
-  context "#format_footer" do
+  context ".show_error" do
+    subject { ChefWorkstation::UI::ErrorPrinter }
+    context "when handling a MultiJobFailure" do
+      it "recognizes it and invokes capture_multiple_failures" do
+        underlying_error = ChefWorkstation::MultiJobFailure.new([])
+        error_to_process = ChefWorkstation::StandardErrorResolver.wrap_exception(underlying_error)
+        expect(subject).to receive(:capture_multiple_failures).with(underlying_error)
+        subject.show_error(error_to_process)
+
+      end
+    end
+
+    context "when an error occurs in error handling" do
+      it "processes the new failure with dump_unexpected_error" do
+        error_to_raise = StandardError.new("this will be raised")
+        error_to_process = ChefWorkstation::StandardErrorResolver.wrap_exception(StandardError.new("this is being shown"))
+        # Intercept a known call to raise an error
+        expect(ChefWorkstation::UI::Terminal).to receive(:output).and_raise error_to_raise
+        expect(subject).to receive(:dump_unexpected_error).with(error_to_raise)
+        subject.show_error(error_to_process)
+      end
+    end
+
+  end
 
+  context ".capture_multiple_failures" do
+    subject { ChefWorkstation::UI::ErrorPrinter }
+    let(:file_content_capture) { StringIO.new }
+    before do
+      allow(ChefWorkstation::Config).to receive(:error_output_path).and_return "/dev/null"
+      allow(File).to receive(:open).with("/dev/null", "w").and_yield(file_content_capture)
+    end
+
+    it "should write a properly formatted error file" do
+      # TODO - add support for test-only i18n content, so that we don't have
+      #        to rely on specific known error IDs that may change or be removed,
+      #        and arent' directly relevant to the test at hand.
+      job1 = double("Job", target_host: double("TargetHost", hostname: "host1"),
+                           exception: ChefWorkstation::Error.new("CHEFUPL002"))
+      job2 = double("Job", target_host: double("TargetHost", hostname: "host2"),
+                           exception: StandardError.new("Hello World"))
+
+      expected_content = File.read("spec/unit/fixtures/multi-error.out")
+      multifailure = ChefWorkstation::MultiJobFailure.new([job1, job2] )
+      subject.capture_multiple_failures(multifailure)
+      expect(file_content_capture.string).to eq expected_content
+    end
+  end
+
+  context "#format_footer" do
     let(:show_log) { true }
     let(:show_stack) { true }
     let(:formatter) do