(#12345) hiera enabled function for stdlib #32

ghoneycutt · 2012-01-31T23:37:56Z

adds ability to test if hiera is enabled in your environment

hunner · 2012-01-31T23:45:11Z

lib/puppet/parser/functions/hiera_enabled.rb

+    #    rescue LoadError => e
+    #      false
+    #    end
+    #true if Puppet.features.hiera?


removing extraneous code that was commented out

ghoneycutt · 2012-01-31T23:48:22Z

Whoops! Thank Hunter, I removed those lines and added another commit.

glarizza · 2012-02-01T00:01:27Z

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

+  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


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".

s/.stubs/.expects and the first test now fails and I'm not sure why
"/opt/puppet/share/puppet/modules/stdlib/spec" F... Failures: 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

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?

jeffmccune · 2012-02-08T18:53:57Z

Git Hygiene

Gramatical Mood

A few comments about good commit messages:

(#12345) hiera enabled function for stdlib    
adds ability to test if hiera is enabled in your environment

The first line should be imperative present tense. This is important because the output of git log is quite jarring to read if the grammatical mood is inconsistent. "hiera enabled function for stdlib" is not imperative, perhaps reword the commit to something like "(#12345) Add hiera_enabled() function to check if hiera is enabled" The 50 character recommendation on the first line is a soft limit.

Rebasing

It's important to rebase commits before merging into the integration branch because once it's merged we can't re-write history. It's perfectly fine to re-write history in a topic branch though.

I notice these two commits have the same summary line and the second appears to undo the first. This is a perfect place to squash them together and reword the commit message using the git rebase -i master command.

a78cdf2 (#12345) hiera enabled function for stdlib 8 days ago Garrett Honeycutt
d54fa78 (#12345) hiera enabled function for stdlib 8 days ago Garrett Honeycutt

This commit should be squashed into the commit where the typo was first introduced. It'll be like it never happened. You can use git blame -w 15c7137~ spec/unit/puppet/parser/functions/hiera_enabled_spec.rb to find the commit where the line you're fixing up was most recently modified. In this case 15c7137 should be squashed into d54fa78.

15c7137 (#12345) fix spelling typo in hiera_enabled spec test 8 days ago Garrett Honeycut

The spec helper fixup in commit 85ea966 should be squashed into d54fa78 which is the commit that introduced the RSpec.configure in the wrong place. The commit should be reworded to imperative present tense.

85ea966 (#12345) using spec_helper 8 days ago Garrett Honeycutt

Describe the behavior without the patch and with the patch

It's important to describe in the commit message the behavior of the code without a commit applied, with a commit applied and why the commit should be applied. Without this information people reading the commit log have much less information about why the change was introduced into the codebase.

None of these commit messages describe the behavior with the patch, without the patch or why the patch should be applied:

My recommendation is to git rebase them together as much as possible and then reword the commit to describe why it's important that stdlib should have this function and what the consequence are if it is not merged.

commit 85ea966
Author: Garrett Honeycutt garrett@puppetlabs.com
Date: Tue Jan 31 17:40:18 2012 -0800

(#12345) using spec_helper

commit 773cb47
Author: Garrett Honeycutt garrett@puppetlabs.com
Date: Tue Jan 31 17:27:01 2012 -0800

(#12345) added inline doc to hiera_enabled function

commit 15c7137
Author: Garrett Honeycutt garrett@puppetlabs.com
Date: Tue Jan 31 17:21:04 2012 -0800

(#12345) fix spelling typo in hiera_enabled spec test

commit a78cdf2
Author: Garrett Honeycutt garrett@puppetlabs.com
Date: Tue Jan 31 15:46:33 2012 -0800

(#12345) hiera enabled function for stdlib

removing extraneous code that was commented out

commit d54fa78
Author: Garrett Honeycutt garrett@puppetlabs.com
Date: Tue Jan 31 15:36:37 2012 -0800

(#12345) hiera enabled function for stdlib

adds ability to test if hiera is enabled in your environment

jeffmccune · 2012-02-08T19:02:03Z

Spec Tests

Regarding the tests themselves, I'm not confident they're testing the things you intend to test. I mentioned this the last time I reviewed this.

Switching all of your stubs to expects yields these two failures. This tells me that the things you're stubbing aren't ever called, which tells me the things you're stubbing probably aren't the things you mean to stub or are simply unnecessary.

% git diff
diff --git a/spec/unit/puppet/parser/functions/hiera_enabled_spec.rb b/spec/unit/puppet/parser/functions/hiera_enabled_spec.rb
index 958a399..fa31575 100644
--- a/spec/unit/puppet/parser/functions/hiera_enabled_spec.rb
+++ b/spec/unit/puppet/parser/functions/hiera_enabled_spec.rb
@@ -5,23 +5,23 @@ describe Puppet::Parser::Functions.function(:hiera_enabled) do
     @scope = Puppet::Parser::Scope.new
   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
+    Puppet.features.expects(:hiera?).returns false
+    Puppet::Parser::Functions.expects(:function).with(:hiera).returns false
     @scope.function_hiera_enabled([]).should be_false
   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
+    Puppet.features.expects(:hiera?).returns false
+    Puppet::Parser::Functions.expects(:function).with(:hiera).returns true
     @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
+    Puppet.features.expects(:hiera?).returns true
+    Puppet::Parser::Functions.expects(:function).with(:hiera).returns false
     @scope.function_hiera_enabled([]).should be_false
   end
   it "should succeed if hiera is installed and the function, hiera(), is available" do
-    Puppet.features.stubs(:hiera?).returns true
-    Puppet::Parser::Functions.stubs(:function).with(:hiera).returns true
+    Puppet.features.expects(:hiera?).returns true
+    Puppet::Parser::Functions.expects(:function).with(:hiera).returns true
     @scope.function_hiera_enabled([]).should be_true
   end
 end

% 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"
FF..

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:0x101819a78>.hiera?(any_parameters)
     # ./spec/unit/puppet/parser/functions/hiera_enabled_spec.rb:9

  2) function_hiera_enabled should fail if hiera is not installed but the function, hiera(), is available
     Failure/Error: Puppet::Parser::Functions.expects(:function).with(:hiera).returns true
     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:0x101819a78>.hiera?(any_parameters)
     # ./spec/unit/puppet/parser/functions/hiera_enabled_spec.rb:14

Finished in 0.00633 seconds
4 examples, 2 failures

jeffmccune · 2012-02-08T19:14:38Z

Unaddressed Review Comments

@ghoneycutt These comments were made in the first review and remain unaddressed:

@scope should avoided and let(:scope) used instead

jeffmccune · 2012-02-08T19:27:44Z

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

+  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


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.

jeffmccune · 2012-02-08T19:31:38Z

@ghoneycutt Please ping code-reviewers once you've addressed this round of comments. I'm going to switch back to my other milestone deadlines and won't poll this pull request to see when it's ready to merge.

jeffmccune · 2012-03-07T01:41:14Z

@ghoneycutt Just wanted to follow up on this, we haven't cut a 2.3.x release for stdlib yet, but once we do this will likely be the version we ship in PE 2.5. If you'd like this new function to be included in PE 2.5 please try and address the comments and rebase the topic branch off of 2.3.x rather than master.

jeffmccune · 2012-03-29T18:08:59Z

Bump?

zaphod42 · 2013-02-18T14:15:03Z

@jeffmccune This PR is pretty old now, and since puppet 3 always includes hiera, I don't see a reason for putting this in. I suggest that we just close it down and reject the associated ticket.

jeffmccune · 2013-02-19T17:06:45Z

I'm going to go ahead and close this pull request for the time being. Please re-open this pull request once the next actions are addressed, new information is available, or you have a question related to this pull request. We've become aware of difficulties re-opening pull requests, in the event you cannot please mention jeffmccune or adrienthebo with an @ sign in front and we'll re-open this pull request.

Closing the pull request doesn't mean we don't consider this change valuable, just that there are things that need to be addressed before it can be merged. If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net.

(#12345) hiera enabled function for stdlib

d54fa78

adds ability to test if hiera is enabled in your environment

hunner reviewed Jan 31, 2012
View reviewed changes

lib/puppet/parser/functions/hiera_enabled.rb

# rescue LoadError => e

# false

# end

#true if Puppet.features.hiera?

Copy link

Contributor

hunner Jan 31, 2012

Choose a reason for hiding this comment

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

Comments?

(#12345) hiera enabled function for stdlib

a78cdf2

removing extraneous code that was commented out

glarizza reviewed Feb 1, 2012
View reviewed changes

Garrett Honeycutt added 3 commits January 31, 2012 17:21

(#12345) fix spelling typo in hiera_enabled spec test

15c7137

(#12345) added inline doc to hiera_enabled function

773cb47

(#12345) using spec_helper

85ea966

jeffmccune reviewed Feb 8, 2012
View reviewed changes

jeffmccune closed this Feb 19, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#12345) hiera enabled function for stdlib #32

(#12345) hiera enabled function for stdlib #32

ghoneycutt commented Jan 31, 2012

hunner Jan 31, 2012

ghoneycutt commented Jan 31, 2012

glarizza Feb 1, 2012

ghoneycutt Feb 1, 2012

jeffmccune Feb 1, 2012

jeffmccune commented Feb 8, 2012

jeffmccune commented Feb 8, 2012

jeffmccune commented Feb 8, 2012

jeffmccune Feb 8, 2012

jeffmccune commented Feb 8, 2012

jeffmccune commented Mar 7, 2012

jeffmccune commented Mar 29, 2012

zaphod42 commented Feb 18, 2013

jeffmccune commented Feb 19, 2013

(#12345) hiera enabled function for stdlib #32

(#12345) hiera enabled function for stdlib #32

Conversation

ghoneycutt commented Jan 31, 2012

hunner Jan 31, 2012

Choose a reason for hiding this comment

ghoneycutt commented Jan 31, 2012

glarizza Feb 1, 2012

Choose a reason for hiding this comment

ghoneycutt Feb 1, 2012

Choose a reason for hiding this comment

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

jeffmccune Feb 1, 2012

Choose a reason for hiding this comment

jeffmccune commented Feb 8, 2012

Git Hygiene

Gramatical Mood

Rebasing

Describe the behavior without the patch and with the patch

jeffmccune commented Feb 8, 2012

Spec Tests

jeffmccune commented Feb 8, 2012

Unaddressed Review Comments

jeffmccune Feb 8, 2012

Choose a reason for hiding this comment

jeffmccune commented Feb 8, 2012

jeffmccune commented Mar 7, 2012

jeffmccune commented Mar 29, 2012

zaphod42 commented Feb 18, 2013

jeffmccune commented Feb 19, 2013