From 96e9225c17b7361ee8b81b5bd0b622d17665adcd Mon Sep 17 00:00:00 2001 From: "Marc A. Paradise" Date: Wed, 2 May 2018 12:32:14 -0400 Subject: [PATCH] Code review comments, additional testing Signed-off-by: Marc A. Paradise --- .../lib/chef-workstation/cli.rb | 9 +--- .../lib/chef-workstation/command/base.rb | 26 +++++----- .../lib/chef-workstation/error.rb | 21 +++++--- .../lib/chef-workstation/ui/error_printer.rb | 18 +++---- .../lib/chef-workstation/ui/terminal.rb | 3 +- .../spec/unit/command/base_spec.rb | 25 ++++++++++ .../spec/unit/fixtures/multi-error.out | 2 + .../spec/unit/ui/error_printer_spec.rb | 50 ++++++++++++++++++- 8 files changed, 113 insertions(+), 41 deletions(-) create mode 100644 components/chef-workstation/spec/unit/fixtures/multi-error.out diff --git a/components/chef-workstation/lib/chef-workstation/cli.rb b/components/chef-workstation/lib/chef-workstation/cli.rb index cb499cb440..dfcfc01181 100644 --- a/components/chef-workstation/lib/chef-workstation/cli.rb +++ b/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 diff --git a/components/chef-workstation/lib/chef-workstation/command/base.rb b/components/chef-workstation/lib/chef-workstation/command/base.rb index 37c76fce40..4d515b6480 100644 --- a/components/chef-workstation/lib/chef-workstation/command/base.rb +++ b/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.render_action(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'. diff --git a/components/chef-workstation/lib/chef-workstation/error.rb b/components/chef-workstation/lib/chef-workstation/error.rb index 817d0a20f2..a32a48e44c 100644 --- a/components/chef-workstation/lib/chef-workstation/error.rb +++ b/components/chef-workstation/lib/chef-workstation/error.rb @@ -61,17 +61,17 @@ 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 @@ -79,15 +79,24 @@ def self.unwrap_exception(wrapper) 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" diff --git a/components/chef-workstation/lib/chef-workstation/ui/error_printer.rb b/components/chef-workstation/lib/chef-workstation/ui/error_printer.rb index cf40dc04bc..7af10b5ba0 100644 --- a/components/chef-workstation/lib/chef-workstation/ui/error_printer.rb +++ b/components/chef-workstation/lib/chef-workstation/ui/error_printer.rb @@ -33,7 +33,7 @@ def t DEFAULT_ERROR_NO = "CHEFINT001" def self.show_error(e) - # Name is misleading - it's unwrapping but also doing furtehr + # Name is misleading - it's unwrapping but also doing further # error resolution for common errors: unwrapped = ChefWorkstation::StandardErrorResolver.unwrap_exception(e) if unwrapped.class == ChefWorkstation::MultiJobFailure @@ -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 diff --git a/components/chef-workstation/lib/chef-workstation/ui/terminal.rb b/components/chef-workstation/lib/chef-workstation/ui/terminal.rb index 02cc89a96b..065e4f1c31 100644 --- a/components/chef-workstation/lib/chef-workstation/ui/terminal.rb +++ b/components/chef-workstation/lib/chef-workstation/ui/terminal.rb @@ -56,7 +56,8 @@ def render_parallel_jobs(header, actions, prefix: "") multispinner.auto_spin end - # TODO this should also accept a job. + # TODO update this to accept a job instead of a block, for consistency of usage + # between render_job and render_parallel def render_job(msg, prefix: "", &block) klass = ChefWorkstation::UI.const_get(ChefWorkstation::Config.dev.spinner) spinner = klass.new("[:spinner] :prefix :status", output: @location) diff --git a/components/chef-workstation/spec/unit/command/base_spec.rb b/components/chef-workstation/spec/unit/command/base_spec.rb index ec47bfa48d..fab16db696 100644 --- a/components/chef-workstation/spec/unit/command/base_spec.rb +++ b/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 diff --git a/components/chef-workstation/spec/unit/fixtures/multi-error.out b/components/chef-workstation/spec/unit/fixtures/multi-error.out new file mode 100644 index 0000000000..693c9e069b --- /dev/null +++ b/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 diff --git a/components/chef-workstation/spec/unit/ui/error_printer_spec.rb b/components/chef-workstation/spec/unit/ui/error_printer_spec.rb index 8a7f913d90..e8276cd55f 100644 --- a/components/chef-workstation/spec/unit/ui/error_printer_spec.rb +++ b/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