Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(FACT-2798) Set color to true, fix Facter.log_exception #2105

Merged
merged 1 commit into from
Sep 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def create_api_call_file(test_vars, facter_querry)
step "Agent #{agent}: Verify that an error is outputted when custom fact file has an error" do
create_api_call_file(test_vars, "Facter.value('custom_fact_1')")
on(agent, "#{ruby_command(agent)} #{test_vars[:test_script_path]}") do |ruby_result|
assert_match(/Facter - error while resolving custom facts in .*file1.rb undefined local variable or method `nill'/,
assert_match(/Facter.*error while resolving custom facts in .*file1.rb undefined local variable or method `nill'/,
ruby_result.stdout)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def create_custom_fact_file(file_name, file_content, fact_dir, agent)

step "Agent #{agent}: Verify that an error is outputted when custom fact file has an error" do
on(agent, facter('custom_fact_4', environment: env), acceptable_exit_codes: [1]) do |facter_output|
assert_match(/ERROR Facter - error while resolving custom facts in .*file1.rb undefined local variable or method `nill'/,
assert_match(/ERROR Facter.*error while resolving custom facts in .*file1.rb undefined local variable or method `nill'/,
facter_output.stderr.chomp)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@

step "Facter: Logs that command of the first custom fact had timeout after setted time limit" do
on agent, facter('--custom-dir', custom_dir, 'foo --debug') do |facter_output|
assert_match(/DEBUG Facter::Core::Execution.* - Timeout encounter after 2s, killing process with pid:/,
assert_match(/DEBUG Facter::Core::Execution.*Timeout encounter after 2s, killing process with pid:/,
facter_output.stderr.chomp)
end
end

step "Facter: Logs that command of the second custom fact had timeout after befault time limit" do
on agent, facter('--custom-dir', custom_dir, 'custom_fact --debug') do |facter_output|
assert_match(/DEBUG Facter::Core::Execution.* - Timeout encounter after 1.5s, killing process with pid:/,
assert_match(/DEBUG Facter::Core::Execution.*Timeout encounter after 1.5s, killing process with pid:/,
facter_output.stderr.chomp)
end
end
Expand Down
2 changes: 1 addition & 1 deletion acceptance/tests/options/verbose.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
agents.each do |agent|
step "Agent #{agent}: retrieve verbose info from stderr using --verbose option" do
on(agent, facter('--verbose')) do
assert_match(/INFO .* executed with command line: --verbose/, stderr, "Expected stderr to contain verbose (INFO) statements")
assert_match(/INFO .*executed with command line: --verbose/, stderr, "Expected stderr to contain verbose (INFO) statements")
end
end
end
Expand Down
31 changes: 19 additions & 12 deletions lib/facter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -311,19 +311,13 @@ def to_user_output(cli_options, *args)
[fact_formatter.format(resolved_facts), status || 0]
end

def log_exception(exception, message = :default)
arr = []
if message == :default
arr << exception.message
elsif message
arr << message
end
if Options[:trace]
arr << 'backtrace:'
arr.concat(exception.backtrace)
end
def log_exception(exception, message = nil)
error_message = []

logger.error(arr.flatten.join("\n"))
error_message << message.to_s unless message.nil? || (message.is_a?(String) && message.empty?)

parse_exception(exception, error_message)
logger.error(error_message.flatten.join("\n"))
end

# Returns a list with the names of all solved facts
Expand Down Expand Up @@ -365,6 +359,19 @@ def warnonce(message)

private

def parse_exception(exception, error_message)
if exception.is_a?(Exception)
error_message << exception.message if error_message.empty?

if Options[:trace] && !exception.backtrace.nil?
error_message << 'backtrace:'
error_message.concat(exception.backtrace)
end
elsif error_message.empty?
error_message << exception.to_s
end
end

def logger
@logger ||= Log.new(self)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/facter/framework/core/options/option_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class OptionStore
@user_query = []
@block_list = {}
@fact_groups = {}
@color = false
@color = true
@timing = false

class << self
Expand Down
6 changes: 1 addition & 5 deletions lib/facter/framework/logging/logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ def warn(msg)
def error(msg, colorize = false)
@@has_errors = true

if msg.nil? || msg.empty?
empty_message_error(msg)
elsif @@message_callback
if @@message_callback
@@message_callback.call(:error, msg)
else
msg = colorize(msg, RED) if colorize || Options[:color]
Expand All @@ -111,8 +109,6 @@ def log_exception(exception)
private

def colorize(msg, color)
return msg if OsDetector.instance.identifier.eql?(:windows)

"#{color}#{msg}#{RESET}"
end

Expand Down
159 changes: 152 additions & 7 deletions spec/facter/facter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,89 @@ def mock_collection(method, os_name = nil, error = nil)
end

describe '#log_exception' do
shared_examples 'when exception param is an exception' do
it 'logs exception message' do
exception.set_backtrace(backtrace)

Facter.log_exception(exception, message)

expect(logger).to have_received(:error).with(expected_message)
end
end

shared_examples 'when exception param is not an exception' do
it 'logs exception message' do
Facter.log_exception(exception, message)

expect(logger).to have_received(:error).with(expected_message)
end
end

context 'when trace option is false' do
let(:backtrace) { 'prog.rb:2:in a' }

context 'when we have an exception and a message' do
let(:message) { 'Some error message' }
let(:exception) { FlushFakeError.new }
let(:expected_message) { 'Some error message' }

it_behaves_like 'when exception param is an exception'
end

context 'when we have an exception and an empty message' do
let(:message) { '' }
let(:exception) { FlushFakeError.new }
let(:expected_message) { 'FlushFakeError' }

it_behaves_like 'when exception param is an exception'
end

context 'when we have an exception and a nil message' do
let(:message) { nil }
let(:exception) { FlushFakeError.new }
let(:expected_message) { 'FlushFakeError' }

it_behaves_like 'when exception param is an exception'
end

context 'when we have an exception and no message' do
let(:exception) { FlushFakeError.new }
let(:expected_message) { 'FlushFakeError' }

it 'logs exception message' do
exception.set_backtrace(backtrace)

Facter.log_exception(exception)

expect(logger).to have_received(:error).with(expected_message)
end
end

context 'when exception and message are strings' do
let(:message) { 'message' }
let(:exception) { 'exception' }
let(:expected_message) { 'message' }

it_behaves_like 'when exception param is not an exception'
end

context 'when exception and message are nil' do
let(:message) { nil }
let(:exception) { nil }
let(:expected_message) { '' }

it_behaves_like 'when exception param is not an exception'
end

context 'when exception and message are hashes' do
let(:message) { { 'a': 1 } }
let(:exception) { { 'b': 2 } }
let(:expected_message) { '{:a=>1}' }

it_behaves_like 'when exception param is not an exception'
end
end

context 'when trace options is true' do
before do
Facter.trace(true)
Expand All @@ -393,16 +476,78 @@ def mock_collection(method, os_name = nil, error = nil)
Facter.trace(false)
end

let(:message) { 'Some error message' }
let(:exception) { FlushFakeError.new }
let(:expected_message) { "Some error message\nbacktrace:\nprog.rb:2:in `a'" }
let(:backtrace) { 'prog.rb:2:in a' }

it 'format exception to display backtrace' do
exception.set_backtrace("prog.rb:2:in `a'")
context 'when we have an exception and a message' do
let(:message) { 'Some error message' }
let(:exception) { FlushFakeError.new }
let(:expected_message) { "Some error message\nbacktrace:\nprog.rb:2:in a" }

Facter.log_exception(exception, message)
it_behaves_like 'when exception param is an exception'
end

expect(logger).to have_received(:error).with(expected_message)
context 'when we have an exception and an empty message' do
let(:message) { '' }
let(:exception) { FlushFakeError.new }
let(:expected_message) { "FlushFakeError\nbacktrace:\nprog.rb:2:in a" }

it_behaves_like 'when exception param is an exception'
end

context 'when we have an exception and a nil message' do
let(:message) { nil }
let(:exception) { FlushFakeError.new }
let(:expected_message) { "FlushFakeError\nbacktrace:\nprog.rb:2:in a" }

it_behaves_like 'when exception param is an exception'
end

context 'when we have an exception and no message' do
let(:exception) { FlushFakeError.new }
let(:expected_message) { "FlushFakeError\nbacktrace:\nprog.rb:2:in a" }

it 'logs exception message' do
exception.set_backtrace(backtrace)

Facter.log_exception(exception)

expect(logger).to have_received(:error).with(expected_message)
end
end

context 'when we have an exception with no backtrace' do
let(:exception) { FlushFakeError.new }
let(:expected_message) { 'FlushFakeError' }

it 'logs exception message' do
Facter.log_exception(exception)

expect(logger).to have_received(:error).with(expected_message)
end
end

context 'when exception and message are strings' do
let(:message) { 'message' }
let(:exception) { 'exception' }
let(:expected_message) { 'message' }

it_behaves_like 'when exception param is not an exception'
end

context 'when exception and message are nil' do
let(:message) { nil }
let(:exception) { nil }
let(:expected_message) { '' }

it_behaves_like 'when exception param is not an exception'
end

context 'when exception and message are hashes' do
let(:message) { { 'a': 1 } }
let(:exception) { { 'b': 2 } }
let(:expected_message) { '{:a=>1}' }

it_behaves_like 'when exception param is not an exception'
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion spec/facter/util/file_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

let(:path) { '/Users/admin/file.txt' }
let(:content) { 'file content' }
let(:error_message) { 'Facter::Util::FileHelper - File at: /Users/admin/file.txt is not accessible.' }
let(:error_message) do
"Facter::Util::FileHelper - #{Facter::CYAN}File at: /Users/admin/file.txt is not accessible.#{Facter::RESET}"
end
let(:array_content) { ['line 1', 'line 2', 'line 3'] }
let(:logger_double) { instance_spy(Logger) }

Expand Down
2 changes: 1 addition & 1 deletion spec/framework/core/options/option_store_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
verbose: false,
config: nil,
cache: true,
color: false,
color: true,
trace: false,
timing: false
)
Expand Down
Loading