From ff1880c0bc33530eb3d2b2c695ff53a2f3905fc9 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Tue, 17 Oct 2023 12:02:56 -0700 Subject: [PATCH 1/3] (PUP-11974) Only warn when Ruby's verbose is enabled In ruby 2.7.x, the rb_file_exists_p and rb_dir_exists_p methods call rb_warning to log that the methods are deprecated[1][2] However, the rb_warning method is a noop if $VERBOSE is nil or false[3] To preserve the same behavior, only warn when $VERBOSE is truthy. Also include the class name and path in the warning so we can identify and fix the issue. [1] https://github.com/ruby/ruby/blob/1f4d4558484b370999954f3ede7e3aa3a3a01ef3/file.c#L1819 [2] https://github.com/ruby/ruby/blob/1f4d4558484b370999954f3ede7e3aa3a3a01ef3/dir.c#L3301 [3] https://github.com/ruby/ruby/blob/1f4d4558484b370999954f3ede7e3aa3a3a01ef3/error.c#L336-L338 --- lib/puppet/util/monkey_patches.rb | 4 ++-- spec/lib/puppet_spec/verbose.rb | 11 ++++++++++- spec/unit/agent_spec.rb | 11 ++--------- spec/unit/util/monkey_patches_spec.rb | 14 +++++++++----- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/lib/puppet/util/monkey_patches.rb b/lib/puppet/util/monkey_patches.rb index 430875ab073..f23cd102a29 100644 --- a/lib/puppet/util/monkey_patches.rb +++ b/lib/puppet/util/monkey_patches.rb @@ -33,7 +33,7 @@ def daemonize unless Dir.singleton_methods.include?(:exists?) class Dir def self.exists?(file_name) - warn('exists? is a deprecated name, use exist? instead') + warn("Dir.exists?('#{file_name}') is deprecated, use Dir.exist? instead") if $VERBOSE Dir.exist?(file_name) end end @@ -42,7 +42,7 @@ def self.exists?(file_name) unless File.singleton_methods.include?(:exists?) class File def self.exists?(file_name) - warn('exists? is a deprecated name, use exist? instead') + warn("File.exists?('#{file_name}') is deprecated, use File.exist? instead") if $VERBOSE File.exist?(file_name) end end diff --git a/spec/lib/puppet_spec/verbose.rb b/spec/lib/puppet_spec/verbose.rb index d9834f2d79c..2f7c26a9a29 100644 --- a/spec/lib/puppet_spec/verbose.rb +++ b/spec/lib/puppet_spec/verbose.rb @@ -1,4 +1,4 @@ -# Support code for running stuff with warnings disabled. +# Support code for running stuff with warnings disabled or enabled module Kernel def with_verbose_disabled verbose, $VERBOSE = $VERBOSE, nil @@ -6,4 +6,13 @@ def with_verbose_disabled $VERBOSE = verbose return result end + + def with_verbose_enabled + verbose, $VERBOSE = $VERBOSE, true + begin + yield + ensure + $VERBOSE = verbose + end + end end diff --git a/spec/unit/agent_spec.rb b/spec/unit/agent_spec.rb index 63a322d4743..3f51f9cd821 100644 --- a/spec/unit/agent_spec.rb +++ b/spec/unit/agent_spec.rb @@ -15,19 +15,12 @@ def stop end end -def without_warnings - flag = $VERBOSE - $VERBOSE = nil - yield - $VERBOSE = flag -end - describe Puppet::Agent do before do @agent = Puppet::Agent.new(AgentTestClient, false) # make Puppet::Application safe for stubbing; restore in an :after block; silence warnings for this. - without_warnings { Puppet::Application = Class.new(Puppet::Application) } + with_verbose_disabled { Puppet::Application = Class.new(Puppet::Application) } allow(Puppet::Application).to receive(:clear?).and_return(true) Puppet::Application.class_eval do class << self @@ -44,7 +37,7 @@ def controlled_run(&block) after do # restore Puppet::Application from stub-safe subclass, and silence warnings - without_warnings { Puppet::Application = Puppet::Application.superclass } + with_verbose_disabled { Puppet::Application = Puppet::Application.superclass } end it "should set its client class at initialization" do diff --git a/spec/unit/util/monkey_patches_spec.rb b/spec/unit/util/monkey_patches_spec.rb index 2e267fd1b52..68516161ec6 100644 --- a/spec/unit/util/monkey_patches_spec.rb +++ b/spec/unit/util/monkey_patches_spec.rb @@ -12,10 +12,12 @@ expect(Dir.exists?(__dir__)).to be true end - if RUBY_VERSION >= '3.2' + if RUBY_VERSION >= '3.2' it 'logs a warning message' do - expect(Dir).to receive(:warn).with('exists? is a deprecated name, use exist? instead') - Dir.exists?(__dir__) + expect(Dir).to receive(:warn).with("Dir.exists?('#{__dir__}') is deprecated, use Dir.exist? instead") + with_verbose_enabled do + Dir.exists?(__dir__) + end end end end @@ -33,8 +35,10 @@ if RUBY_VERSION >= '3.2' it 'logs a warning message' do - expect(File).to receive(:warn).with('exists? is a deprecated name, use exist? instead') - File.exists?(__FILE__) + expect(File).to receive(:warn).with("File.exists?('#{__FILE__}') is deprecated, use File.exist? instead") + with_verbose_enabled do + File.exists?(__FILE__) + end end end end From 9c4924758c4961aa9e6bd55b52c7cff4fa8bd9ad Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Tue, 17 Oct 2023 12:45:26 -0700 Subject: [PATCH 2/3] (maint) Use stub_const instead of disabling $VERBOSE Previously we were disabling $VERBOSE so that we could stub a constant during tests without generating warnings like: warning: already initialized constant ... Just use `stub_const`, which doesn't cause those warnings and it automatically restores the original value when the block completes. --- spec/unit/agent_spec.rb | 9 ++------- spec/unit/daemon_spec.rb | 23 +++-------------------- spec/unit/type/exec_spec.rb | 10 +--------- 3 files changed, 6 insertions(+), 36 deletions(-) diff --git a/spec/unit/agent_spec.rb b/spec/unit/agent_spec.rb index 3f51f9cd821..502f45fbc93 100644 --- a/spec/unit/agent_spec.rb +++ b/spec/unit/agent_spec.rb @@ -19,8 +19,8 @@ def stop before do @agent = Puppet::Agent.new(AgentTestClient, false) - # make Puppet::Application safe for stubbing; restore in an :after block; silence warnings for this. - with_verbose_disabled { Puppet::Application = Class.new(Puppet::Application) } + # make Puppet::Application safe for stubbing + stub_const('Puppet::Application', Class.new(Puppet::Application)) allow(Puppet::Application).to receive(:clear?).and_return(true) Puppet::Application.class_eval do class << self @@ -35,11 +35,6 @@ def controlled_run(&block) allow(Puppet::SSL::StateMachine).to receive(:new).and_return(machine) end - after do - # restore Puppet::Application from stub-safe subclass, and silence warnings - with_verbose_disabled { Puppet::Application = Puppet::Application.superclass } - end - it "should set its client class at initialization" do expect(Puppet::Agent.new("foo", false).client_class).to eq("foo") end diff --git a/spec/unit/daemon_spec.rb b/spec/unit/daemon_spec.rb index 575923d8479..93a5587f0b1 100644 --- a/spec/unit/daemon_spec.rb +++ b/spec/unit/daemon_spec.rb @@ -3,13 +3,6 @@ require 'puppet/agent' require 'puppet/configurer' -def without_warnings - flag = $VERBOSE - $VERBOSE = nil - yield - $VERBOSE = flag -end - describe Puppet::Daemon, :unless => Puppet::Util::Platform.windows? do include PuppetSpec::Files @@ -91,14 +84,8 @@ def run_loop(jobs) describe "when stopping" do before do allow(Puppet::Util::Log).to receive(:close_all) - # to make the global safe to mock, set it to a subclass of itself, - # then restore it in an after pass - without_warnings { Puppet::Application = Class.new(Puppet::Application) } - end - - after do - # restore from the superclass so we lose the stub garbage - without_warnings { Puppet::Application = Puppet::Application.superclass } + # to make the global safe to mock, set it to a subclass of itself + stub_const('Puppet::Application', Class.new(Puppet::Application)) end it 'should request a stop from Puppet::Application' do @@ -143,11 +130,7 @@ def run_loop(jobs) describe "when restarting" do before do - without_warnings { Puppet::Application = Class.new(Puppet::Application) } - end - - after do - without_warnings { Puppet::Application = Puppet::Application.superclass } + stub_const('Puppet::Application', Class.new(Puppet::Application)) end it 'should set Puppet::Application.restart!' do diff --git a/spec/unit/type/exec_spec.rb b/spec/unit/type/exec_spec.rb index 6c330920775..9608f176d17 100644 --- a/spec/unit/type/exec_spec.rb +++ b/spec/unit/type/exec_spec.rb @@ -266,15 +266,7 @@ def exec_stub(options = {}) describe "on platforms where path separator is not :" do before :each do - @old_verbosity = $VERBOSE - $VERBOSE = nil - @old_separator = File::PATH_SEPARATOR - File::PATH_SEPARATOR = 'q' - end - - after :each do - File::PATH_SEPARATOR = @old_separator - $VERBOSE = @old_verbosity + stub_const('File::PATH_SEPARATOR', 'q') end it "should use the path separator of the current platform" do From f4dc8e4d3be31ced3bb06a8a22636696e23f28bd Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Tue, 17 Oct 2023 12:51:09 -0700 Subject: [PATCH 3/3] (maint) Ensure $VERBOSE is restored If the block passed to `with_verbose_disabled` raised, then $VERBOSE wasn't restored to its original value. This method isn't used in puppet, but has been here a long time, so guard against the issue instead of removing the method. --- spec/lib/puppet_spec/verbose.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/lib/puppet_spec/verbose.rb b/spec/lib/puppet_spec/verbose.rb index 2f7c26a9a29..6e2b6550ddd 100644 --- a/spec/lib/puppet_spec/verbose.rb +++ b/spec/lib/puppet_spec/verbose.rb @@ -2,9 +2,11 @@ module Kernel def with_verbose_disabled verbose, $VERBOSE = $VERBOSE, nil - result = yield - $VERBOSE = verbose - return result + begin + yield + ensure + $VERBOSE = verbose + end end def with_verbose_enabled