From 25d03d43224d18de07df7de2eef51270359a646d Mon Sep 17 00:00:00 2001 From: Thibault Jouan Date: Sat, 11 Oct 2014 17:34:19 +0000 Subject: [PATCH] Improve CLI error reporting --- features/cli_error_reporting.feature | 20 ++++++++ lib/producer/core.rb | 4 +- lib/producer/core/cli.rb | 5 +- lib/producer/core/error_formatter.rb | 53 ++++++++++++++++++++++ lib/producer/core/errors.rb | 2 + lib/producer/core/recipe/file_evaluator.rb | 9 +++- spec/producer/core/cli_spec.rb | 6 +-- spec/producer/core/error_formatter_spec.rb | 50 ++++++++++++++++++++ 8 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 features/cli_error_reporting.feature create mode 100644 lib/producer/core/error_formatter.rb create mode 100644 spec/producer/core/error_formatter_spec.rb diff --git a/features/cli_error_reporting.feature b/features/cli_error_reporting.feature new file mode 100644 index 0000000..50a8bb6 --- /dev/null +++ b/features/cli_error_reporting.feature @@ -0,0 +1,20 @@ +Feature: CLI error reporting + + Background: + Given a recipe with: + """ + task(:trigger_error) { fail 'some error' } + """ + + Scenario: reports recipe errors + When I execute the recipe + Then the exit status must be 70 + And the output must match /\ARuntimeError: some error\n/ + + Scenario: reports errors with a backtrace + When I execute the recipe + Then the output must match /^\s+recipe\.rb:\d+:in / + + Scenario: prepends recipe file path in the backtrace + When I execute the recipe + Then the output must match /^\s+recipe\.rb \(recipe\)\n\s+recipe\.rb:/ diff --git a/lib/producer/core.rb b/lib/producer/core.rb index 75735de..2679e6b 100644 --- a/lib/producer/core.rb +++ b/lib/producer/core.rb @@ -32,10 +32,12 @@ require 'producer/core/tests/has_file' require 'producer/core/tests/shell_command_status' require 'producer/core/tests/yaml_eq' +require 'producer/core/errors' + require 'producer/core/cli' require 'producer/core/condition' require 'producer/core/env' -require 'producer/core/errors' +require 'producer/core/error_formatter' require 'producer/core/logger_formatter' require 'producer/core/prompter' require 'producer/core/recipe' diff --git a/lib/producer/core/cli.rb b/lib/producer/core/cli.rb index 52d1993..d2c2daa 100644 --- a/lib/producer/core/cli.rb +++ b/lib/producer/core/cli.rb @@ -17,8 +17,9 @@ module Producer rescue ArgumentError => e stderr.puts e.message exit EX_USAGE - rescue RuntimeError => e - stderr.puts "#{e.class.name.split('::').last}: #{e.message}" + rescue Exception => e + ef = ErrorFormatter.new(force_cause: [RecipeEvaluationError]) + stderr.puts ef.format e exit EX_SOFTWARE end end diff --git a/lib/producer/core/error_formatter.rb b/lib/producer/core/error_formatter.rb new file mode 100644 index 0000000..97f17a9 --- /dev/null +++ b/lib/producer/core/error_formatter.rb @@ -0,0 +1,53 @@ +module Producer + module Core + class ErrorFormatter + def initialize(debug: false, force_cause: []) + @debug = debug + @force_cause = force_cause + end + + def debug? + !!@debug + end + + def format(exception) + lines = format_exception exception + + if debug? && exception.cause + lines << '' + lines << 'cause:' + lines << format_exception(exception.cause, filter: false) + end + + lines.join("\n") + end + + private + + def format_exception(exception, filter: true) + [ + format_message(exception), + *format_backtrace(exception.backtrace, filter: filter) + ] + end + + def format_message(exception) + exception = exception.cause if @force_cause.include? exception.class + "#{exception.class.name.split('::').last}: #{exception.message}" + end + + def format_backtrace(backtrace, filter: true) + backtrace = filter_backtrace backtrace if filter + indent_backtrace backtrace + end + + def filter_backtrace(backtrace) + backtrace.reject { |l| l =~ /\/producer-\w+\/(?:bin|lib)\// } + end + + def indent_backtrace(backtrace) + backtrace.map { |e| ' %s' % e } + end + end + end +end diff --git a/lib/producer/core/errors.rb b/lib/producer/core/errors.rb index e97e867..49d0687 100644 --- a/lib/producer/core/errors.rb +++ b/lib/producer/core/errors.rb @@ -2,8 +2,10 @@ module Producer module Core Error = Class.new(StandardError) RuntimeError = Class.new(RuntimeError) + ArgumentError = Class.new(Error) ConditionNotMetError = Class.new(Error) + RecipeEvaluationError = Class.new(RuntimeError) RemoteCommandExecutionError = Class.new(RuntimeError) RegistryKeyError = Class.new(RuntimeError) end diff --git a/lib/producer/core/recipe/file_evaluator.rb b/lib/producer/core/recipe/file_evaluator.rb index fb280c0..6898a4a 100644 --- a/lib/producer/core/recipe/file_evaluator.rb +++ b/lib/producer/core/recipe/file_evaluator.rb @@ -5,7 +5,14 @@ module Producer class << self def evaluate(file_path, env) content = File.read(file_path) - Recipe.new(env).tap { |o| o.instance_eval content, file_path } + begin + Recipe.new(env).tap { |o| o.instance_eval content, file_path } + rescue Exception => e + fail RecipeEvaluationError, e.message, [ + '%s (recipe)' % file_path, + *e.backtrace + ] + end end end end diff --git a/spec/producer/core/cli_spec.rb b/spec/producer/core/cli_spec.rb index c4fa56e..a3fd902 100644 --- a/spec/producer/core/cli_spec.rb +++ b/spec/producer/core/cli_spec.rb @@ -44,7 +44,7 @@ module Producer::Core end end - context 'when a runtime error is raised' do + context 'when an error is raised' do let(:recipe_file) { fixture_path_for 'recipes/raise.rb' } it 'exits with a return status of 70' do @@ -52,9 +52,9 @@ module Producer::Core .to raise_error(SystemExit) { |e| expect(e.status).to eq 70 } end - it 'prints exception name and message and the error stream' do + it 'prints a report to the error stream' do expect { trap_exit { run! } } - .to output("RemoteCommandExecutionError: false\n").to_stderr + .to output(/\ARemoteCommandExecutionError: false$/).to_stderr end end end diff --git a/spec/producer/core/error_formatter_spec.rb b/spec/producer/core/error_formatter_spec.rb new file mode 100644 index 0000000..fd4bc86 --- /dev/null +++ b/spec/producer/core/error_formatter_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +module Producer + module Core + describe ErrorFormatter do + let(:debug) { false } + let(:force_cause) { [] } + let(:options) { { debug: debug, force_cause: force_cause } } + subject(:formatter) { described_class.new(options) } + + describe '#debug?' do + it 'returns false' do + expect(formatter.debug?).to be false + end + + context 'when debug is enabled' do + let(:debug) { true } + + it 'returns true' do + expect(formatter.debug?).to be true + end + end + end + + describe '#format' do + let(:message) { 'some exception' } + let(:exception) { Exception.new(message) } + + before { exception.set_backtrace %w[back trace] } + + it 'formats the message' do + expect(formatter.format exception) + .to match /^Exception: some exception$/ + end + + it 'indents the backtrace' do + expect(formatter.format exception).to match /^\s+back$/ + end + + context 'filtering' do + before { exception.set_backtrace %w[back trace /producer-core/lib/] } + + it 'excludes producer code from the backtrace' do + expect(formatter.format exception).not_to include 'producer-core' + end + end + end + end + end +end