Merge pull request #2669 from joshcooper/rubocop_5_RSpec

(FACT-3428) Resolve RSpec cops

.rubocop.yml

@@ -53,6 +53,12 @@ Naming/VariableNumber:
     - 'lib/facter/resolvers/windows/ffi/networking_ffi.rb'
     - 'lib/facter/util/facts/windows_release_finder.rb'
 
+RSpec/MultipleMemoizedHelpers:
+  Enabled: false
+
+RSpec/StubbedMock:
+  Enabled: false
+
 Style/IfUnlessModifier:
   Enabled: false
 
@@ -67,3 +73,43 @@ Style/GlobalStdStream:
     - 'lib/facter/framework/logging/logger.rb'
     - 'spec/framework/core/fact/internal/internal_fact_manager_spec.rb'
     - 'spec/framework/logging/logger_spec.rb'
+
+RSpec/DescribedClass:
+  EnforcedStyle: explicit
+
+RSpec/ExampleLength:
+  Enabled: false
+
+# It is sometimes better to expect().to receive().and_return
+# or to receive different messages.
+RSpec/MessageSpies:
+  Enabled: false
+
+RSpec/MultipleExpectations:
+  Max: 3
+
+RSpec/MultipleMemoizedHelpers:
+  Enabled: false
+
+RSpec/NestedGroups:
+  Enabled: 6
+
+RSpec/SubjectStub:
+  Exclude:
+    - 'spec/custom_facts/core/aggregate_spec.rb'
+    - 'spec/custom_facts/core/resolvable_spec.rb'
+    - 'spec/custom_facts/util/fact_spec.rb'
+    - 'spec/custom_facts/util/resolution_spec.rb'
+
+# Prefer instance_double/instance_spy over double/spy because only methods
+# defined on the underlying object can be stubbed. FFI and some Windows
+# code can't be verified because of the way we fake out those classes.
+RSpec/VerifiedDoubles:
+  Enabled: true
+  Exclude:
+    - 'spec/custom_facts/**/*'
+    - 'spec/facter/resolvers/aix/hardware_spec.rb'
+    - 'spec/facter/resolvers/*/ffi_helper_spec.rb'
+    - 'spec/facter/resolvers/windows/*'
+    - 'spec/facter/util/windows/network_utils_spec.rb'
+    - 'spec/facter/util/windows/win32ole_spec.rb'

.rubocop_todo.yml

@@ -6,95 +6,12 @@
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 3
-# This cop supports safe autocorrection (--autocorrect).
-Performance/RegexpMatch:
-  Exclude:
-    - 'install.rb'
-
-# Offense count: 1490
-# This cop supports unsafe autocorrection (--autocorrect-all).
-# Configuration parameters: SkipBlocks, EnforcedStyle.
-# SupportedStyles: described_class, explicit
-RSpec/DescribedClass:
-  Enabled: false
-
-# Offense count: 192
-# Configuration parameters: CountAsOne.
-RSpec/ExampleLength:
-  Max: 34
-
-# Offense count: 1
-# This cop supports safe autocorrection (--autocorrect).
-# Configuration parameters: CustomTransform, IgnoredWords, DisallowedExamples.
-# DisallowedExamples: works
-RSpec/ExampleWording:
-  Exclude:
-    - 'spec/custom_facts/core/execution/windows_spec.rb'
-
 # Offense count: 70
 # Configuration parameters: Include, CustomTransform, IgnoreMethods, SpecSuffixOnly.
 # Include: **/*_spec*rb*, **/spec/**/*
 RSpec/FilePath:
   Enabled: false
 
-# Offense count: 15
-# Configuration parameters: AssignmentOnly.
-RSpec/InstanceVariable:
-  Exclude:
-    - 'spec/custom_facts/core/execution/fact_manager_spec.rb'
-    - 'spec/custom_facts/util/collection_spec.rb'
-    - 'spec/custom_facts/util/confine_spec.rb'
-
-# Offense count: 8
-RSpec/LeakyConstantDeclaration:
-  Exclude:
-    - 'spec/custom_facts/core/resolvable_spec.rb'
-    - 'spec/custom_facts/core/suitable_spec.rb'
-    - 'spec/custom_facts/util/collection_spec.rb'
-    - 'spec/facter/resolvers/macosx/mountpoints_spec.rb'
-    - 'spec/facter/util/windows/network_utils_spec.rb'
-
-# Offense count: 51
-# Configuration parameters: EnforcedStyle.
-# SupportedStyles: have_received, receive
-RSpec/MessageSpies:
-  Enabled: false
-
-# Offense count: 13
-RSpec/MultipleExpectations:
-  Max: 3
-
-# Offense count: 166
-# Configuration parameters: AllowSubject.
-RSpec/MultipleMemoizedHelpers:
-  Max: 16
-
-# Offense count: 241
-# Configuration parameters: AllowedGroups.
-RSpec/NestedGroups:
-  Max: 6
-
-# Offense count: 3
-RSpec/StubbedMock:
-  Exclude:
-    - 'spec/custom_facts/core/execution/fact_manager_spec.rb'
-    - 'spec/custom_facts/util/confine_spec.rb'
-    - 'spec/custom_facts/util/parser_spec.rb'
-
-# Offense count: 6
-RSpec/SubjectStub:
-  Exclude:
-    - 'spec/custom_facts/core/aggregate_spec.rb'
-    - 'spec/custom_facts/core/resolvable_spec.rb'
-    - 'spec/custom_facts/util/fact_spec.rb'
-    - 'spec/custom_facts/util/resolution_spec.rb'
-
-# Offense count: 114
-# Configuration parameters: IgnoreNameless, IgnoreSymbolicNames.
-RSpec/VerifiedDoubles:
-  Enabled: false
-
 # Need to remove logger class variables
 Style/ClassVars:
   Exclude:

install.rb

@@ -40,7 +40,7 @@ class Installer
 
   # Returns true if OS is windows (copied from facter/util/config.rb)
   def windows?
-    (defined?(RbConfig) ? RbConfig : Config)::CONFIG['host_os'] =~ /mswin|win32|dos|mingw|cygwin/i
+    (defined?(RbConfig) ? RbConfig : Config)::CONFIG['host_os'].match?(/mswin|win32|dos|mingw|cygwin/i)
   end
 
   def glob(list)
@@ -153,7 +153,7 @@ def prepare_installation
     # /System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/bin
     # which is not generally where people expect executables to be installed
     # These settings are appropriate defaults for all OS X versions.
-    RbConfig::CONFIG['bindir'] = '/usr/bin' if RUBY_PLATFORM =~ /^universal-darwin[\d.]+$/
+    RbConfig::CONFIG['bindir'] = '/usr/bin' if RUBY_PLATFORM.match?(/^universal-darwin[\d.]+$/)
 
     # if InstallOptions.configdir
     #   configdir = InstallOptions.configdir
@@ -173,10 +173,10 @@ def prepare_installation
     else
       sitelibdir = RbConfig::CONFIG['sitelibdir']
       if sitelibdir.nil?
-        sitelibdir = $LOAD_PATH.find { |x| x =~ /site_ruby/ }
+        sitelibdir = $LOAD_PATH.find { |x| x.match?(/site_ruby/) }
         if sitelibdir.nil?
           sitelibdir = File.join(libdir, 'site_ruby')
-        elsif sitelibdir !~ Regexp.quote(version)
+        elsif !sitelibdir.match?(Regexp.quote(version))
           sitelibdir = File.join(sitelibdir, version)
         end
       end
@@ -232,7 +232,7 @@ def install_binfile(from, op_file, target)
       File.open(tmp_file.path, 'w') do |op|
         op.puts "#!#{ruby}"
         contents = ip.readlines
-        contents.shift if contents[0] =~ /^#!/
+        contents.shift if contents[0].match?(/^#!/)
         op.write contents.join
       end
     end

spec/custom_facts/core/execution/fact_manager_spec.rb

@@ -4,6 +4,8 @@
   subject(:executor) { Facter::Core::Execution::Posix.new }
 
   describe '#with_env' do
+    let(:sentinel_var) { :resolution_test_foo.to_s }
+
     it "executes the caller's block with the specified env vars" do
       test_env = { 'LANG' => 'C', 'LC_ALL' => 'C', 'FOO' => 'BAR' }
       executor.with_env test_env do
@@ -56,23 +58,21 @@
     end
 
     it "is not affected by a 'return' statement in the yield block" do
-      @sentinel_var = :resolution_test_foo.to_s
-
       # the intent of this test case is to test a yield block that contains a return statement.  However, it's illegal
       # to use a return statement outside of a method, so we need to create one here to give scope to the 'return'
       def handy_method
-        ENV[@sentinel_var] = 'foo'
-        new_env = { @sentinel_var => 'bar' }
+        ENV[sentinel_var] = 'foo'
+        new_env = { sentinel_var => 'bar' }
 
         executor.with_env new_env do
-          ENV[@sentinel_var] = 'bar'
+          ENV[sentinel_var] = 'bar'
           return
         end
       end
 
       handy_method
 
-      expect(ENV[@sentinel_var]).to eq 'foo'
+      expect(ENV[sentinel_var]).to eq 'foo'
     end
   end
 

spec/custom_facts/core/execution/windows_spec.rb

@@ -82,7 +82,7 @@
         expect(executor.which('foo.exe')).to eq 'C:\Windows\foo.exe'
       end
 
-      it "won't check all paths if one is executable" do
+      it 'does not check all paths if one is executable' do
         allow(File).to receive(:executable?).with('C:\Windows\system32\foo.exe').and_return false
         allow(File).to receive(:executable?).with('C:\Windows\foo.exe').and_return true
 

spec/custom_facts/util/collection_spec.rb

@@ -125,20 +125,21 @@ def load(collection)
   end
 
   describe 'when retrieving facts' do
-    before do
-      @fact = collection.add('YayNess')
-    end
+    let(:fact) { collection.add('YayNess') }
+
+    # initialize
+    before { fact }
 
     it 'returns the fact instance specified by the name' do
-      expect(collection.fact('YayNess')).to equal(@fact)
+      expect(collection.fact('YayNess')).to equal(fact)
     end
 
     it 'is case-insensitive' do
-      expect(collection.fact('yayness')).to equal(@fact)
+      expect(collection.fact('yayness')).to equal(fact)
     end
 
     it 'treats strings and symbols equivalently' do
-      expect(collection.fact(:yayness)).to equal(@fact)
+      expect(collection.fact(:yayness)).to equal(fact)
     end
 
     it 'uses its loader to try to load the fact if no fact can be found' do

spec/custom_facts/util/confine_spec.rb

@@ -27,14 +27,15 @@
   end
 
   describe 'when evaluating' do
+    let(:fact) { double 'fact' }
+
     def confined(fact_value, *confines)
-      allow(@fact).to receive(:value).and_return fact_value
+      allow(fact).to receive(:value).and_return fact_value
       LegacyFacter::Util::Confine.new('yay', *confines).true?
     end
 
     before do
-      @fact = double 'fact'
-      allow(Facter).to receive(:[]).and_return @fact
+      allow(Facter).to receive(:[]).and_return fact
     end
 
     it 'returns false if the fact does not exist' do
@@ -44,9 +45,9 @@ def confined(fact_value, *confines)
     end
 
     it 'uses the returned fact to get the value' do
-      allow(Facter).to receive(:[]).with('yay').and_return @fact
+      allow(Facter).to receive(:[]).with('yay').and_return fact
 
-      expect(@fact).to receive(:value).and_return nil
+      expect(fact).to receive(:value).and_return nil
 
       LegacyFacter::Util::Confine.new('yay', 'test').true?
     end
@@ -120,13 +121,13 @@ def confined(fact_value, *confines)
     end
 
     it 'accepts and evaluate a block argument against the fact' do
-      allow(@fact).to receive(:value).and_return 'foo'
+      allow(fact).to receive(:value).and_return 'foo'
       confine = LegacyFacter::Util::Confine.new(:yay) { |f| f == 'foo' }
       expect(confine.true?).to be true
     end
 
     it 'returns false if the block raises a StandardError when checking a fact' do
-      allow(@fact).to receive(:value).and_return 'foo'
+      allow(fact).to receive(:value).and_return 'foo'
       confine = LegacyFacter::Util::Confine.new(:yay) { |_f| raise StandardError }
       expect(confine.true?).to be false
     end

spec/facter/facts/macosx/memory/swap/capacity_spec.rb

@@ -3,7 +3,7 @@
 describe Facts::Macosx::Memory::Swap::Capacity do
   describe '#call_the_resolver' do
     it 'returns a fact' do
-      expected_fact = double(Facter::ResolvedFact, name: 'memory.swap.capacity', value: 1024)
+      expected_fact = instance_double(Facter::ResolvedFact, name: 'memory.swap.capacity', value: 1024)
 
       allow(Facter::Resolvers::Macosx::SwapMemory).to receive(:resolve).with(:capacity).and_return(1024)
       allow(Facter::ResolvedFact).to receive(:new).with('memory.swap.capacity', 1024).and_return(expected_fact)

spec/facter/facts/macosx/memory/swap/used_bytes_spec.rb

@@ -3,7 +3,7 @@
 describe Facts::Macosx::Memory::Swap::UsedBytes do
   describe '#call_the_resolver' do
     it 'returns a fact' do
-      expected_fact = double(Facter::ResolvedFact, name: 'memory.swap.used_bytes', value: 1024)
+      expected_fact = instance_double(Facter::ResolvedFact, name: 'memory.swap.used_bytes', value: 1024)
 
       allow(Facter::Resolvers::Macosx::SwapMemory).to receive(:resolve).with(:used_bytes).and_return(1024)
       allow(Facter::ResolvedFact).to receive(:new).with('memory.swap.used_bytes', 1024).and_return(expected_fact)

spec/facter/facts/macosx/memory/system/capacity_spec.rb

@@ -3,7 +3,7 @@
 describe Facts::Macosx::Memory::System::Capacity do
   describe '#call_the_resolver' do
     it 'returns a fact' do
-      expected_fact = double(Facter::ResolvedFact, name: 'memory.system.capacity', value: '15.53%')
+      expected_fact = instance_double(Facter::ResolvedFact, name: 'memory.system.capacity', value: '15.53%')
 
       allow(Facter::Resolvers::Macosx::SystemMemory).to receive(:resolve).with(:capacity).and_return('15.53%')
       allow(Facter::ResolvedFact).to receive(:new).with('memory.system.capacity', '15.53%').and_return(expected_fact)

spec/facter/facts/macosx/memory/system/used_bytes_spec.rb

@@ -3,7 +3,7 @@
 describe Facts::Macosx::Memory::System::UsedBytes do
   describe '#call_the_resolver' do
     it 'returns a fact' do
-      expected_fact = double(Facter::ResolvedFact, name: 'memory.system.used_bytes', value: 1024)
+      expected_fact = instance_double(Facter::ResolvedFact, name: 'memory.system.used_bytes', value: 1024)
 
       allow(Facter::Resolvers::Macosx::SystemMemory).to receive(:resolve).with(:used_bytes).and_return(1024)
       allow(Facter::ResolvedFact).to receive(:new).with('memory.system.used_bytes', 1024).and_return(expected_fact)

spec/facter/facts/windows/dmi/product/serial_number_spec.rb

@@ -5,8 +5,8 @@
     subject(:fact) { Facts::Windows::Dmi::Product::SerialNumber.new }
 
     let(:value) { 'VMware-42 1a 0d 03 0a b7 98 28-78 98 5e 85 a0 ad 18 47' }
-    let(:expected_resolved_fact) { double(Facter::ResolvedFact, name: 'dmi.product.serial_number', value: value) }
-    let(:resolved_legacy_fact) { double(Facter::ResolvedFact, name: 'serialnumber', value: value, type: :legacy) }
+    let(:expected_resolved_fact) { instance_double(Facter::ResolvedFact, name: 'dmi.product.serial_number', value: value) }
+    let(:resolved_legacy_fact) { instance_double(Facter::ResolvedFact, name: 'serialnumber', value: value, type: :legacy) }
 
     before do
       allow(Facter::Resolvers::DMIBios).to receive(:resolve).with(:serial_number).and_return(value)

spec/facter/facts/windows/hypervisors/hyperv_spec.rb

@@ -4,7 +4,7 @@
   describe '#call_the_resolver' do
     context 'when is not HyperV hypervisor' do
       it 'returns nil' do
-        expected_fact = double(Facter::ResolvedFact, name: 'hypervisors.hyperv', value: nil)
+        expected_fact = instance_double(Facter::ResolvedFact, name: 'hypervisors.hyperv', value: nil)
         allow(Facter::Resolvers::Windows::Virtualization).to receive(:resolve).with(:virtual).and_return('value')
         allow(Facter::Resolvers::DMIBios).to receive(:resolve).with(:manufacturer).and_return('value')
         allow(Facter::ResolvedFact).to receive(:new).with('hypervisors.hyperv', nil).and_return(expected_fact)
@@ -16,7 +16,7 @@
 
     context 'when is HyperV hypervisor and CpuidSource resolver returns the required output' do
       it 'returns a fact' do
-        expected_fact = double(Facter::ResolvedFact, name: 'hypervisors.hyperv', value: {})
+        expected_fact = instance_double(Facter::ResolvedFact, name: 'hypervisors.hyperv', value: {})
         allow(Facter::Resolvers::Windows::Virtualization).to receive(:resolve).with(:virtual).and_return('hyperv')
         allow(Facter::ResolvedFact).to receive(:new).with('hypervisors.hyperv', {}).and_return(expected_fact)
 
@@ -27,7 +27,7 @@
 
     context 'when is HyperV hypervisor and DmiBios resolver returns the required output' do
       it 'returns a fact' do
-        expected_fact = double(Facter::ResolvedFact, name: 'hypervisors.hyperv', value: {})
+        expected_fact = instance_double(Facter::ResolvedFact, name: 'hypervisors.hyperv', value: {})
         allow(Facter::Resolvers::Windows::Virtualization).to receive(:resolve).with(:virtual).and_return('value')
         allow(Facter::Resolvers::DMIBios).to receive(:resolve).with(:manufacturer).and_return('Microsoft Enterprise')
         allow(Facter::ResolvedFact).to receive(:new).with('hypervisors.hyperv', {}).and_return(expected_fact)