(#6978) Enforce the calling convention of option hooks.

We require that hooks take exactly three arguments; now we enforce that in the
DSL, to ensure we give good, and early, errors to users who do the wrong
thing.

Paired-With: Max Martin <max@puppetlabs.com>

2 files changed, 51 insertions, 17 deletions

diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 51aa837dc..4be6a1c55 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -182,7 +182,7 @@ describe Puppet::Interface::Action do
       it "should call action before decorators" do
         face.action(:baz) do
           option "--baz" do
-            before_action do
+            before_action do |action, args, options|
               report(:action_option)
             end
           end
@@ -196,7 +196,7 @@ describe Puppet::Interface::Action do
       it "should call action after decorators" do
         face.action(:baz) do
           option "--baz" do
-            after_action do
+            after_action do |action, args, options|
               report(:action_option)
             end
           end
@@ -209,7 +209,7 @@ describe Puppet::Interface::Action do
 
       it "should call local before decorators" do
         face.option "--foo FOO" do
-          before_action do
+          before_action do |action, args, options|
             report(:before)
           end
         end
@@ -218,7 +218,9 @@ describe Puppet::Interface::Action do
       end
 
       it "should call local after decorators" do
-        face.option "--foo FOO" do after_action do report(:after) end end
+        face.option "--foo FOO" do
+          after_action do |action, args, options| report(:after) end
+        end
         face.expects(:report).with(:after)
         face.bar({:foo => 12})
       end
@@ -226,7 +228,9 @@ describe Puppet::Interface::Action do
       context "with inactive decorators" do
         it "should not invoke a decorator if the options are empty" do
           face.option "--foo FOO" do
-            before_action do report :before_action end
+            before_action do |action, args, options|
+              report :before_action
+            end
           end
           face.expects(:report).never # I am testing the negative.
           face.bar
@@ -234,8 +238,12 @@ describe Puppet::Interface::Action do
 
         context "with some decorators only" do
           before :each do
-            face.option "--foo" do before_action do report :foo end end
-            face.option "--bar" do before_action do report :bar end end
+            face.option "--foo" do
+              before_action do |action, args, options| report :foo end
+            end
+            face.option "--bar" do
+              before_action do |action, args, options| report :bar end
+            end
           end
 
           it "should work with the foo option" do
@@ -274,7 +282,7 @@ describe Puppet::Interface::Action do
       context "with a child decorator" do
         subject do
           child.option "--foo FOO" do
-            before_action do
+            before_action do |action, args, options|
               report(:child_before)
             end
           end
@@ -294,7 +302,7 @@ describe Puppet::Interface::Action do
       context "with a parent decorator" do
         subject do
           parent.option "--foo FOO" do
-            before_action do
+            before_action do |action, args, options|
               report(:parent_before)
             end
           end
@@ -314,12 +322,12 @@ describe Puppet::Interface::Action do
       context "with child and parent decorators" do
         subject do
           parent.option "--foo FOO" do
-            before_action { report(:parent_before) }
-            after_action  { report(:parent_after)  }
+            before_action { |action, args, options| report(:parent_before) }
+            after_action  { |action, args, options| report(:parent_after)  }
           end
           child.option "--bar BAR" do
-            before_action { report(:child_before) }
-            after_action  { report(:child_after)  }
+            before_action { |action, args, options| report(:child_before) }
+            after_action  { |action, args, options| report(:child_after)  }
           end
 
           child.expects(:report).with(:child_before)
diff --git a/spec/unit/interface/option_builder_spec.rb b/spec/unit/interface/option_builder_spec.rb
index 0bcbed82c..b32b316f6 100755
--- a/spec/unit/interface/option_builder_spec.rb
+++ b/spec/unit/interface/option_builder_spec.rb
@@ -25,10 +25,36 @@ describe Puppet::Interface::OptionBuilder do
     option.desc.should == text
   end
 
-  it "should support a before_action hook" do
-    option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
-      before_action do :whatever end
+  context "before_action hook" do
+    it "should support a before_action hook" do
+      option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
+        before_action do |a,b,c| :whatever end
+      end
+      option.before_action.should be_an_instance_of UnboundMethod
+    end
+
+    it "should fail if the hook block takes too few arguments" do
+      expect do
+        Puppet::Interface::OptionBuilder.build(face, "--foo") do
+          before_action do |one, two| true end
+        end
+      end.to raise_error ArgumentError, /takes three arguments/
+    end
+
+    it "should fail if the hook block takes too many arguments" do
+      expect do
+        Puppet::Interface::OptionBuilder.build(face, "--foo") do
+          before_action do |one, two, three, four| true end
+        end
+      end.to raise_error ArgumentError, /takes three arguments/
+    end
+
+    it "should fail if the hook block takes a variable number of arguments" do
+      expect do
+        Puppet::Interface::OptionBuilder.build(face, "--foo") do
+          before_action do |*blah| true end
+        end
+      end.to raise_error ArgumentError, /takes three arguments/
     end
-    option.before_action.should be_an_instance_of UnboundMethod
   end
 end