Merge pull request #649 from DavidS/modules-3737

(MODULES-3737) refactor validate_legacy and tests

Author: hunner

Gemfile

@@ -21,7 +21,7 @@ group :development, :unit_tests do
   gem 'puppet_facts'
   gem 'puppet-blacksmith', '>= 3.4.0'
   gem 'puppetlabs_spec_helper', '>= 1.2.1'
-  gem 'rspec-puppet', '>= 2.3.2'
+  gem 'rspec-puppet', '>= 2.3.2', :git => 'https://github.com/rodjek/rspec-puppet.git', :branch => 'fb27c533e2664057fba4b73d0bd902a946abfce0'
   gem 'rspec-puppet-facts'
   gem 'simplecov'
   gem 'parallel_tests'

lib/puppet/functions/validate_legacy.rb

@@ -2,48 +2,49 @@
   # The function checks a value against both the target_type (new) and the previous_validation function (old).
 
   dispatch :validate_legacy do
+    scope_param
     param 'Type', :target_type
-    param 'String', :previous_validation
-    param 'NotUndef', :value
-    optional_param 'Any', :args
+    param 'String', :function_name
+    param 'Any', :value
+    optional_repeated_param 'Any', :args
   end
+
   dispatch :validate_legacy_s do
     scope_param
     param 'String', :type_string
-    param 'String', :previous_validation
-    param 'NotUndef', :value
+    param 'String', :function_name
+    param 'Any', :value
     optional_repeated_param 'Any', :args
   end
 
   def validate_legacy_s(scope, type_string, *args)
     t = Puppet::Pops::Types::TypeParser.new.parse(type_string, scope)
-    validate_legacy(t, *args)
+    validate_legacy(scope, t, *args)
   end
 
-  def validate_legacy(target_type, previous_validation, value, *prev_args)
+  def validate_legacy(scope, target_type, function_name, value, *prev_args)
     if assert_type(target_type, value)
-      if previous_validation(previous_validation, value, *prev_args)
+      if previous_validation(scope, function_name, value, *prev_args)
         # Silently passes
       else
-        Puppet.warn("Accepting previously invalid value for target_type '#{target_type}'")
+        Puppet.notice("Accepting previously invalid value for target type '#{target_type}'")
       end
     else
       caller_infos = caller.first.split(":")
       inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(value)
-      message = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch(previous_validation, target_type, inferred_type)
-      error_msg = "#{message} : #{caller_infos[0]} : #{caller_infos[1]}"
-      if previous_validation(previous_validation, value, *prev_args)
-        Puppet.warn(error_msg)
+      error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch("validate_legacy(#{function_name}) [#{caller_infos[0]}:#{caller_infos[1]}]", target_type, inferred_type)
+      if previous_validation(scope, function_name, value, *prev_args)
+        call_function('deprecation', 'validate_legacy', error_msg)
       else
         call_function('fail', error_msg)
       end
     end
   end
 
-  def previous_validation(previous_validation, value, *prev_args)
+  def previous_validation(scope, function_name, value, *prev_args)
     # Call the previous validation function and catch any errors. Return true if no errors are thrown.
     begin
-      call_function(previous_validation, value, *prev_args)
+      scope.send("function_#{function_name}".to_s, [value, *prev_args])
       true
     rescue Puppet::ParseError
       false

spec/classes/validate_legacy_spec.rb

@@ -5,31 +5,26 @@
     it "should not fail" do
       scope.expects(:lookupvar).with('module_name').returns('foo')
       scope.expects(:lookupvar).with('caller_module_name').returns('foo')
-      expect {
-        subject.call []
-      }.not_to raise_error
+
+      is_expected.to run.with_params()
     end
   end
 
-  context "with an explicit failure message" do
-    it "prints the failure message on error" do
+  context "when called from private class" do
+    before :each do
       scope.expects(:lookupvar).with('module_name').returns('foo')
       scope.expects(:lookupvar).with('caller_module_name').returns('bar')
-      expect {
-        subject.call ['failure message!']
-      }.to raise_error Puppet::ParseError, /failure message!/
     end
-  end
 
-  context "when called from private class" do
     it "should fail with a class error message" do
-      scope.expects(:lookupvar).with('module_name').returns('foo')
-      scope.expects(:lookupvar).with('caller_module_name').returns('bar')
       scope.source.expects(:name).returns('foo::baz')
       scope.source.expects(:type).returns('hostclass')
-      expect {
-        subject.call []
-      }.to raise_error Puppet::ParseError, /Class foo::baz is private/
+
+      is_expected.to run.with_params().and_raise_error(Puppet::ParseError, /Class foo::baz is private/)
+    end
+
+    context "with an explicit failure message" do
+      it { is_expected.to run.with_params('failure message!').and_raise_error(Puppet::ParseError, /failure message!/) }
     end
   end
 
@@ -39,9 +34,8 @@
       scope.expects(:lookupvar).with('caller_module_name').returns('bar')
       scope.source.expects(:name).returns('foo::baz')
       scope.source.expects(:type).returns('definition')
-      expect {
-        subject.call []
-      }.to raise_error Puppet::ParseError, /Definition foo::baz is private/
+
+      is_expected.to run.with_params().and_raise_error(Puppet::ParseError, /Definition foo::baz is private/)
     end
   end
 end

spec/fixtures/test/manifests/validate_legacy.pp

@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+if Puppet.version.to_f >= 4.0
+  describe 'validate_legacy' do
+    it { is_expected.not_to eq(nil) }
+    it { is_expected.to run.with_params.and_raise_error(ArgumentError) }
+
+    describe 'when passing the type assertion and passing the previous validation' do
+      before do
+        scope.expects(:function_validate_foo).with([5]).once
+        Puppet.expects(:notice).never
+      end
+      it 'passes without notice' do
+        is_expected.to run.with_params('Integer', 'validate_foo', 5)
+      end
+    end
+
+    describe 'when passing the type assertion and failing the previous validation' do
+      before do
+        scope.expects(:function_validate_foo).with([5]).raises(Puppet::ParseError, 'foo').once
+        Puppet.expects(:notice).with(includes('Accepting previously invalid value for target type'))
+      end
+      it 'passes with a notice about newly accepted value' do
+        is_expected.to run.with_params('Integer', 'validate_foo', 5)
+      end
+    end
+
+    describe 'when failing the type assertion and passing the previous validation' do
+      before do
+        scope.expects(:function_validate_foo).with(['5']).once
+        subject.func.expects(:call_function).with('deprecation', 'validate_legacy', includes('expected an Integer value')).once
+      end
+      it 'passes with a deprecation message' do
+        is_expected.to run.with_params('Integer', 'validate_foo', '5')
+      end
+    end
+
+    describe 'when failing the type assertion and failing the previous validation' do
+      before do
+        scope.expects(:function_validate_foo).with(['5']).raises(Puppet::ParseError, 'foo').once
+        subject.func.expects(:call_function).with('fail', includes('expected an Integer value')).once
+      end
+      it 'fails with a helpful message' do
+        is_expected.to run.with_params('Integer', 'validate_foo', '5')
+      end
+    end
+
+    describe 'when passing in undef' do
+      before do
+        scope.expects(:function_validate_foo).with([:undef]).once
+        Puppet.expects(:notice).never
+      end
+      it 'works' do
+        is_expected.to run.with_params('Optional[Integer]', 'validate_foo', :undef)
+      end
+    end
+
+    describe 'when passing in multiple arguments' do
+      before do
+        scope.expects(:function_validate_foo).with([:undef, 1, 'foo']).once
+        Puppet.expects(:notice).never
+      end
+      it 'passes with a deprecation message' do
+        is_expected.to run.with_params('Optional[Integer]', 'validate_foo', :undef, 1, 'foo')
+      end
+    end
+  end
+end

spec/functions/assert_private_spec.rb

@@ -28,22 +28,6 @@
     end
 
     describe 'invalid inputs' do
-      it {
-        pending('should implement stricter type checking')
-        is_expected.to run.with_params([], '').and_raise_error(Puppet::ParseError, /is not a String/)
-      }
-      it {
-        pending('should implement stricter type checking')
-        is_expected.to run.with_params('', {}).and_raise_error(Puppet::ParseError, /is not an Array/)
-      }
-      it {
-        pending('should implement stricter type checking')
-        is_expected.to run.with_params('', '', []).and_raise_error(Puppet::ParseError, /is not a String/)
-      }
-      it {
-        pending('should implement stricter type checking')
-        is_expected.to run.with_params(nil, nil).and_raise_error(Puppet::ParseError, /is not a String/)
-      }
       it { is_expected.to run.with_params('', []).and_raise_error(Puppet::ParseError, /does not match/) }
       it { is_expected.to run.with_params('one', 'two').and_raise_error(Puppet::ParseError, /does not match/) }
       it { is_expected.to run.with_params('', 'two').and_raise_error(Puppet::ParseError, /does not match/) }