From 607b68493e3c1ada5b5437be54b2fc82e48ab96a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Thu, 27 Apr 2023 14:06:26 -1000 Subject: [PATCH 1/2] Rework the tests to check actual values Rather that testing that when called twice the function return the same or different values, test the actual returned value so that we are sure the behavior does not change unexpectedly. --- spec/functions/seeded_rand_spec.rb | 44 ++++-------------------------- 1 file changed, 5 insertions(+), 39 deletions(-) diff --git a/spec/functions/seeded_rand_spec.rb b/spec/functions/seeded_rand_spec.rb index 9ee3b867b..50d1b15ff 100644 --- a/spec/functions/seeded_rand_spec.rb +++ b/spec/functions/seeded_rand_spec.rb @@ -17,44 +17,10 @@ it { is_expected.to run.with_params(1, []).and_raise_error(ArgumentError, %r{second argument must be a string}) } it { is_expected.to run.with_params(1, {}).and_raise_error(ArgumentError, %r{second argument must be a string}) } - it 'provides a random number strictly less than the given max' do - expect(seeded_rand(3, 'seed')).to satisfy { |n| n.to_i < 3 } # rubocop:disable Lint/AmbiguousBlockAssociation : Cannot parenthesize without break code or violating other Rubocop rules - end - - it 'provides a random number greater or equal to zero' do - expect(seeded_rand(3, 'seed')).to satisfy { |n| n.to_i >= 0 } # rubocop:disable Lint/AmbiguousBlockAssociation : Cannot parenthesize without break code or violating other Rubocop rules - end - - it "provides the same 'random' value on subsequent calls for the same host" do - expect(seeded_rand(10, 'seed')).to eql(seeded_rand(10, 'seed')) - end - - it 'allows seed to control the random value on a single host' do - first_random = seeded_rand(1000, 'seed1') - second_different_random = seeded_rand(1000, 'seed2') - - expect(first_random).not_to eql(second_different_random) - end - - it 'does not return different values for different hosts' do - val1 = seeded_rand(1000, 'foo', host: 'first.host.com') - val2 = seeded_rand(1000, 'foo', host: 'second.host.com') - - expect(val1).to eql(val2) - end - - def seeded_rand(max, seed, args = {}) - host = args[:host] || '127.0.0.1' - - # workaround not being able to use let(:facts) because some tests need - # multiple different hostnames in one context - allow(scope).to receive(:lookupvar).with('::fqdn', {}).and_return(host) - - scope.function_seeded_rand([max, seed]) - end - - context 'with UTF8 and double byte characters' do - it { is_expected.to run.with_params(1000, 'ǿňè') } - it { is_expected.to run.with_params(1000, '文字列') } + context 'produce predictible and reproducible results' do + it { is_expected.to run.with_params(20, 'foo').and_return(1) } + it { is_expected.to run.with_params(100, 'bar').and_return(35) } + it { is_expected.to run.with_params(1000, 'ǿňè').and_return(247) } + it { is_expected.to run.with_params(1000, '文字列').and_return(67) } end end From 205037164fd7445582aa289271efc390dfa99de1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Thu, 27 Apr 2023 14:12:17 -1000 Subject: [PATCH 2/2] Rewrite seeded_rand() as a Puppet 4.x function The 3.x function rely on is_integer() which is deprecated. Rewrite it using the more modern puppet 4.x function to rely on data types for better parameters validation. --- lib/puppet/functions/seeded_rand.rb | 22 ++++++++++++++++ lib/puppet/parser/functions/seeded_rand.rb | 30 ---------------------- spec/functions/seeded_rand_spec.rb | 23 ++++++++--------- 3 files changed, 33 insertions(+), 42 deletions(-) create mode 100644 lib/puppet/functions/seeded_rand.rb delete mode 100644 lib/puppet/parser/functions/seeded_rand.rb diff --git a/lib/puppet/functions/seeded_rand.rb b/lib/puppet/functions/seeded_rand.rb new file mode 100644 index 000000000..f5960709b --- /dev/null +++ b/lib/puppet/functions/seeded_rand.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# @summary +# Generates a random whole number greater than or equal to 0 and less than max, using the value of seed for repeatable randomness. +Puppet::Functions.create_function(:seeded_rand) do + # @param max The maximum value. + # @param seed The seed used for repeatable randomness. + # + # @return [Integer] + # A random number greater than or equal to 0 and less than max + dispatch :seeded_rand do + param 'Integer[1]', :max + param 'String', :seed + end + + def seeded_rand(max, seed) + require 'digest/md5' + + seed = Digest::MD5.hexdigest(seed).hex + Puppet::Util.deterministic_rand_int(seed, max) + end +end diff --git a/lib/puppet/parser/functions/seeded_rand.rb b/lib/puppet/parser/functions/seeded_rand.rb deleted file mode 100644 index f9919f8d1..000000000 --- a/lib/puppet/parser/functions/seeded_rand.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -# -# seeded_rand.rb -# -Puppet::Parser::Functions.newfunction(:seeded_rand, arity: 2, type: :rvalue, doc: <<-DOC - @summary - Generates a random whole number greater than or equal to 0 and less than MAX, using the value of SEED for repeatable randomness. - - @return - random number greater than or equal to 0 and less than MAX - - @example **Usage:** - seeded_rand(MAX, SEED). - MAX must be a positive integer; SEED is any string. - - Generates a random whole number greater than or equal to 0 and less - than MAX, using the value of SEED for repeatable randomness. If SEED - starts with "$fqdn:", this is behaves the same as `fqdn_rand`. -DOC -) do |args| - require 'digest/md5' - - raise(ArgumentError, 'seeded_rand(): first argument must be a positive integer') unless function_is_integer([args[0]]) && args[0].to_i > 0 - raise(ArgumentError, 'seeded_rand(): second argument must be a string') unless args[1].is_a? String - - max = args[0].to_i - seed = Digest::MD5.hexdigest(args[1]).hex - Puppet::Util.deterministic_rand_int(seed, max) -end diff --git a/spec/functions/seeded_rand_spec.rb b/spec/functions/seeded_rand_spec.rb index 50d1b15ff..206765b70 100644 --- a/spec/functions/seeded_rand_spec.rb +++ b/spec/functions/seeded_rand_spec.rb @@ -4,18 +4,17 @@ describe 'seeded_rand' do it { is_expected.not_to eq(nil) } - it { is_expected.to run.with_params.and_raise_error(ArgumentError, %r{wrong number of arguments}i) } - it { is_expected.to run.with_params(1).and_raise_error(ArgumentError, %r{wrong number of arguments}i) } - it { is_expected.to run.with_params(0, '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params(1.5, '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params(-10, '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params('-10', '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params('string', '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params([], '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params({}, '').and_raise_error(ArgumentError, %r{first argument must be a positive integer}) } - it { is_expected.to run.with_params(1, 1).and_raise_error(ArgumentError, %r{second argument must be a string}) } - it { is_expected.to run.with_params(1, []).and_raise_error(ArgumentError, %r{second argument must be a string}) } - it { is_expected.to run.with_params(1, {}).and_raise_error(ArgumentError, %r{second argument must be a string}) } + it { is_expected.to run.with_params.and_raise_error(ArgumentError, %r{'seeded_rand' expects 2 arguments, got none}i) } + it { is_expected.to run.with_params(1).and_raise_error(ArgumentError, %r{'seeded_rand' expects 2 arguments, got 1}i) } + it { is_expected.to run.with_params(0, '').and_raise_error(ArgumentError, %r{parameter 'max' expects an Integer\[1\] value, got Integer\[0, 0\]}) } + it { is_expected.to run.with_params(1.5, '').and_raise_error(ArgumentError, %r{parameter 'max' expects an Integer value, got Float}) } + it { is_expected.to run.with_params(-10, '').and_raise_error(ArgumentError, %r{parameter 'max' expects an Integer\[1\] value, got Integer\[-10, -10\]}) } + it { is_expected.to run.with_params('string', '').and_raise_error(ArgumentError, %r{parameter 'max' expects an Integer value, got String}) } + it { is_expected.to run.with_params([], '').and_raise_error(ArgumentError, %r{parameter 'max' expects an Integer value, got Array}) } + it { is_expected.to run.with_params({}, '').and_raise_error(ArgumentError, %r{parameter 'max' expects an Integer value, got Hash}) } + it { is_expected.to run.with_params(1, 1).and_raise_error(ArgumentError, %r{parameter 'seed' expects a String value, got Integer}) } + it { is_expected.to run.with_params(1, []).and_raise_error(ArgumentError, %r{parameter 'seed' expects a String value, got Array}) } + it { is_expected.to run.with_params(1, {}).and_raise_error(ArgumentError, %r{parameter 'seed' expects a String value, got Hash}) } context 'produce predictible and reproducible results' do it { is_expected.to run.with_params(20, 'foo').and_return(1) }