From ba8904c71357ddda0e18d95f3184307cd52be710 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Wed, 5 Jun 2019 18:58:08 -0700 Subject: [PATCH 1/5] Check for the existence of the ivar before accessing it. Makes the code run nearly warning free, reducing the output of `rspec -w spec` by 42 lines. That has to mean something, right? Signed-off-by: Ryan Davis --- lib/mixlib/log.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/mixlib/log.rb b/lib/mixlib/log.rb index 491bbc8..18cdfd3 100644 --- a/lib/mixlib/log.rb +++ b/lib/mixlib/log.rb @@ -190,6 +190,7 @@ def loggers_to_close # via public API. In order to reduce amount of impact and # handle only File type log devices I had to use this method # to get access to it. + next unless logger.instance_variable_defined?(:"@logdev") next unless (logdev = logger.instance_variable_get(:"@logdev")) loggers_to_close << logger if logdev.filename end From 9b2d51adfff3d6dda5e8fc0ca1d5c0eb40e7a393 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Wed, 5 Jun 2019 19:01:13 -0700 Subject: [PATCH 2/5] Minor spec cleanup, using the block form of expect. Signed-off-by: Ryan Davis --- spec/mixlib/log_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/mixlib/log_spec.rb b/spec/mixlib/log_spec.rb index 5e0f270..bbf290b 100644 --- a/spec/mixlib/log_spec.rb +++ b/spec/mixlib/log_spec.rb @@ -134,17 +134,17 @@ def #{method_name}(message) end it "should raise an ArgumentError if you try and set the level to something strange using the binding form" do - expect(lambda { Logit.level = :the_roots }).to raise_error(ArgumentError) + expect { Logit.level = :the_roots }.to raise_error(ArgumentError) end it "should raise an ArgumentError if you try and set the level to something strange using the method form" do - expect(lambda { Logit.level(:the_roots) }).to raise_error(ArgumentError) + expect { Logit.level(:the_roots) }.to raise_error(ArgumentError) end it "should pass other method calls directly to logger" do Logit.level = :debug expect(Logit).to be_debug - expect(lambda { Logit.debug("Gimme some sugar!") }).to_not raise_error + expect { Logit.debug("Gimme some sugar!") }.to_not raise_error end it "should pass add method calls directly to logger" do @@ -152,7 +152,7 @@ def #{method_name}(message) Logit.init(logdev) Logit.level = :debug expect(Logit).to be_debug - expect(lambda { Logit.add(Logger::DEBUG, "Gimme some sugar!") }).to_not raise_error + expect { Logit.add(Logger::DEBUG, "Gimme some sugar!") }.to_not raise_error expect(logdev.string).to match(/Gimme some sugar/) end From d0dc22161e5d6da10581855b9a61fa277f291801 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Wed, 5 Jun 2019 19:46:59 -0700 Subject: [PATCH 3/5] Use $stdout instead of STDOUT for the default logdev. This makes it testable using minitest/rspec IO assertions/expectations. STDOUT/STDERR should only be used to restore $stdout/$stderr if something goes haywire. The globals should be used for everyday use. Also bolstered an IO test to prevent the output from going to the rspec result output. Signed-off-by: Ryan Davis --- lib/mixlib/log.rb | 2 +- lib/mixlib/log/logger.rb | 2 +- spec/mixlib/log_spec.rb | 13 ++++++++++--- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/mixlib/log.rb b/lib/mixlib/log.rb index 18cdfd3..599ac57 100644 --- a/lib/mixlib/log.rb +++ b/lib/mixlib/log.rb @@ -170,7 +170,7 @@ def method_missing(method_symbol, *args, &block) def logger_for(*opts) if opts.empty? - Mixlib::Log::Logger.new(STDOUT) + Mixlib::Log::Logger.new($stdout) elsif LEVELS.keys.inject(true) { |quacks, level| quacks && opts.first.respond_to?(level) } opts.first else diff --git a/lib/mixlib/log/logger.rb b/lib/mixlib/log/logger.rb index f92d4a2..f227f23 100644 --- a/lib/mixlib/log/logger.rb +++ b/lib/mixlib/log/logger.rb @@ -21,7 +21,7 @@ def trace?; @level <= TRACE; end # # +logdev+:: # The log device. This is a filename (String) or IO object (typically - # +STDOUT+, +STDERR+, or an open file). + # +$stdout+, +$stderr+, or an open file). # +shift_age+:: # Number of old log files to keep, *or* frequency of rotation (+daily+, # +weekly+ or +monthly+). diff --git a/spec/mixlib/log_spec.rb b/spec/mixlib/log_spec.rb index bbf290b..b58c4e2 100644 --- a/spec/mixlib/log_spec.rb +++ b/spec/mixlib/log_spec.rb @@ -142,9 +142,14 @@ def #{method_name}(message) end it "should pass other method calls directly to logger" do - Logit.level = :debug - expect(Logit).to be_debug - expect { Logit.debug("Gimme some sugar!") }.to_not raise_error + expect do + # this needs to be inside of the block because the level setting + # is causing the init, which grabs $stderr before rspec replaces + # it for output testing. + Logit.level = :debug + expect(Logit).to be_debug + Logit.debug("Gimme some sugar!") + end.to output(/DEBUG: Gimme some sugar!/).to_stdout end it "should pass add method calls directly to logger" do @@ -158,6 +163,7 @@ def #{method_name}(message) it "should default to STDOUT if init is called with no arguments" do logger_mock = Struct.new(:formatter, :level).new + # intentionally STDOUT to avoid unfailable test expect(Logger).to receive(:new).with(STDOUT).and_return(logger_mock) Logit.init end @@ -203,6 +209,7 @@ def #{method_name}(message) end it "should return nil from its logging methods" do + # intentionally STDOUT to avoid unfailable test expect(Logger).to receive(:new).with(STDOUT) { double("a-quiet-logger").as_null_object } Logit.init From c04defd313576d316497d38d63bf913cbc2fb013 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Wed, 5 Jun 2019 19:49:07 -0700 Subject: [PATCH 4/5] Mode #logger a lazy initializer. Otherwise the tests were resetting twice per. Signed-off-by: Ryan Davis --- lib/mixlib/log.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mixlib/log.rb b/lib/mixlib/log.rb index 599ac57..2d2653d 100644 --- a/lib/mixlib/log.rb +++ b/lib/mixlib/log.rb @@ -46,7 +46,7 @@ def loggers # and creates a new one if it doesn't yet exist ## def logger - @logger || init + @logger ||= init end # Sets the log device to +new_log_device+. Any additional loggers From 616f57e849331ee349ad5fda431ec86b4e3e4b65 Mon Sep 17 00:00:00 2001 From: Ryan Davis Date: Wed, 5 Jun 2019 22:00:30 -0700 Subject: [PATCH 5/5] Fixed the last of the warnings. Also removed a line that didn't have the effect intended. Those were class-ivars, not ivars. Signed-off-by: Ryan Davis --- lib/mixlib/log.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/mixlib/log.rb b/lib/mixlib/log.rb index 2d2653d..e57ffcd 100644 --- a/lib/mixlib/log.rb +++ b/lib/mixlib/log.rb @@ -27,11 +27,12 @@ module Mixlib module Log include Logging - @logger, @loggers = nil def reset! + @logger ||= nil + @loggers ||= [] close! - @logger, @loggers = nil, nil + @logger = @loggers = nil @metadata = {} end