Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#12345) hiera enabled function for stdlib #32

Closed
wants to merge 5 commits into from
Closed

(#12345) hiera enabled function for stdlib #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d54fa78
(#12345) hiera enabled function for stdlib
Jan 31, 2012
a78cdf2
(#12345) hiera enabled function for stdlib
Jan 31, 2012
15c7137
(#12345) fix spelling typo in hiera_enabled spec test
Feb 1, 2012
773cb47
(#12345) added inline doc to hiera_enabled function
Feb 1, 2012
85ea966
(#12345) using spec_helper
Feb 1, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/puppet/feature/hiera.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
require 'puppet/util/feature'
Puppet.features.add(:hiera, :libs => %{hiera})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be :hiera or :hiera? It doesn't seem to match up to spec/unit/puppet/parser/functions/hiera_enabled_spec.rb:L13 where you use Puppet.features.stubs(:hiera?).returns false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be any test coverage for this feature, I only see that we stub it. I'd like to see an expectation that the feature is added to Puppet. This could be as simple as setting an expectation like Puppet.features.expects(:add).with(:hiera, :libs => 'hiera')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something actionable that I need to address?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry for not being direct.

It actually seems strange that Puppet.features.add(:hiera, :libs => %{hiera}) isn't contained in anything. What's self when this code is evaluated?

This should definitely have some coverage.

16 changes: 16 additions & 0 deletions lib/puppet/parser/functions/hiera_enabled.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module Puppet::Parser::Functions
newfunction(:hiera_enabled, :type => :rvalue ) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see an inline doc string here so puppet doc -r function works well for the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me an example?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the Doc lines here --> https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/lib/puppet/parser/functions/abs.rb#L6-8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this with commit 773cb47

# begin
# require 'hiera'
# true if Puppet::Parser::Functions.function 'hiera'
# rescue LoadError => e
# false
# end
#true if Puppet.features.hiera?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments?

if Puppet.features.hiera? and Puppet::Parser::Functions.function(:hiera)
true
else
false
end
end
end
32 changes: 32 additions & 0 deletions spec/unit/puppet/parser/functions/hiera_enabled_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
require 'puppet'
require 'mocha'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the spec_helper take care of this? If not, could you add these two requires to the spec_helper please?


RSpec.configure do |config|
config.mock_with :mocha
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely be in the spec_helper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec_helper?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Badboy :) https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/spec/spec_helper.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This is done with commit 85ea966


describe Puppet::Parser::Functions.function(:hiera_enabled) do
before :each do
@scope = Puppet::Parser::Scope.new
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend avoiding instance variables and instead use let to get a memoized helper method like this:

More information at let and let!

describe 'Puppet::Parser::Functions.function(:hiera_enabled)' do
  let :scope do
    Puppet::Parser::Scope.new
  end

  it "should fail if hiera is not installed" do
    Puppet.features.expects(:hiera?).returns false
    Puppet::Parser::Functions.stubs(:function).with(:hiera).returns false
    scope.function_hiera_enabled([]).should be_false
  end
end

end
it "should fail if hiera is not installed" do
Puppet.features.stubs(:hiera?).returns false
Puppet::Parser::Functions.stubs(:function).with(:hiera).returns false
@scope.function_hiera_enabled([]).should be_false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that all the .stubs should be .expects - setup an expectation that the function call (or features call) IS made so that if that call is ever NOT made then you know something broke. As it stands now, we're saying "IF you call this function, just return false" when in reality we want to say "YES, this function SHOULD be called, AND it should return FALSE".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/.stubs/.expects and the first test now fails and I'm not sure why

"/opt/puppet/share/puppet/modules/stdlib/spec" F...

Failures:

  1. function_hiera_enabled should fail if hiera is not installed
    Failure/Error: Puppet::Parser::Functions.expects(:function).with(:hiera).returns false
    Mocha::ExpectationError:
    not all expectations were satisfied
    unsatisfied expectations:
    • expected exactly once, not yet invoked: Puppet::Parser::Functions.function(:hiera)
      satisfied expectations:
    • allowed any number of times, not yet invoked: Signal.trap(any_parameters)
    • expected exactly once, invoked once: #Puppet::Util::Feature:0xb7c8190c.hiera?(any_parameters)

    ./spec/unit/puppet/parser/functions/hiera_enabled_spec.rb:15

Finished in 0.00799 seconds
4 examples, 1 failure

Failed examples:

rspec ./spec/unit/puppet/parser/functions/hiera_enabled_spec.rb:13 # function_hiera_enabled should fail if hiera is not installed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this happens it's because the thing you're stubbing never actually receives a message. Switching the stub to an expectation means it will fail if it's never sent a message.

When this happens to me, it's almost always because the thing I thought I was calling isn't actually the thing being called.

Does this help?

end
it "should fail if hiera is not installed but the function, hiera(), is available" do
Puppet.features.stubs(:hiera?).returns false
Puppet::Parser::Functions.stubs(:function).with(:hiera).returns true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These stubs shouldn't actually return true but instead a String. It's important to match up the return value of a mocked object with what would actually be returned in code, otherwise we risk testing behaviors that will never happen in the real world.

A quick way to find out what would actually be returned from a real object rather than a mocked object is to drop into an interactive debugger right before setting up your mock. For example:

Does Puppet::Parser::Functions.function(:template) return true or some other object?

% rdebug -- rspec spec/unit/puppet/parser/functions/hiera_enabled_spec.rb
[4, 13] in /Users/jeff/.rvm/gems/ruby-1.8.7-p334@puppet/bin/rspec
   4  #
   5  # The application 'rspec-core' is installed as part of a gem, and
   6  # this file is here to facilitate running it.
   7  #
   8  
=> 9  require 'rubygems'
   10  
   11  version = ">= 0"
   12  
   13  if ARGV.first =~ /^_(.*)_$/ and Gem::Version.correct? $1 then
/Users/jeff/.rvm/gems/ruby-1.8.7-p334@puppet/bin/rspec:9
require 'rubygems'
(rdb:1) c
"/Users/jeff/vms/puppet/modules/stdlib/spec"
...[18, 26] in /Users/jeff/vms/puppet/modules/stdlib/spec/unit/puppet/parser/functions/hiera_enabled_spec.rb
   18      @scope.function_hiera_enabled([]).should be_false
   19    end
   20    it "should succeed if hiera is installed and the function, hiera(), is available" do
   21      Puppet.features.expects(:hiera?).returns true
   22      debugger
=> 23      Puppet::Parser::Functions.expects(:function).with(:hiera).returns true
   24      @scope.function_hiera_enabled([]).should be_true
   25    end
   26  end
/Users/jeff/vms/puppet/modules/stdlib/spec/unit/puppet/parser/functions/hiera_enabled_spec.rb:23
Puppet::Parser::Functions.expects(:function).with(:hiera).returns true

(rdb:1) Puppet::Parser::Functions.function(:template)
"function_template"

Ah, it looks like a valid function returns a string rather than true. This tells me the mocked object isn't behaving like the real object since the mock is returning true instead of the string.

@scope.function_hiera_enabled([]).should be_false
end
it "should fail if hiera is installed but the function, hiera(), is not available" do
Puppet.features.stubs(:hiera?).returns true
Puppet::Parser::Functions.stubs(:function).with(:hiera).returns false
@scope.function_hiera_enabled([]).should be_false
end
it "should succeed is hiera is installed and the function, hiera(), is available" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/is/if/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote 'is' twice on that line, --> "should succeed IF hiera is installed..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah!

committed 15c7137 to fix that

Puppet.features.stubs(:hiera?).returns true
Puppet::Parser::Functions.stubs(:function).with(:hiera).returns true
@scope.function_hiera_enabled([]).should be_true
end
end