Fix #4238 - if should match undef as ''

The comparisons operator (and more particularly == and !=) were not treating
the undef value as '', like case and selector did since #2818.

This patch makes sure comparison operator uses AST leaf matching.

Unfortunately, doing this introduces a behavior change compared to
the previous versions:
Numbers embedded in strings will now be matched as numbers in case and
selector statements instead of string matching.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

2 files changed, 69 insertions, 24 deletions

diff --git a/spec/unit/parser/ast/comparison_operator_spec.rb b/spec/unit/parser/ast/comparison_operator_spec.rb
index 724b6c6f7..931f936df 100755
--- a/spec/unit/parser/ast/comparison_operator_spec.rb
+++ b/spec/unit/parser/ast/comparison_operator_spec.rb
@@ -5,21 +5,22 @@ require File.dirname(__FILE__) + '/../../../spec_helper'
 describe Puppet::Parser::AST::ComparisonOperator do
   before :each do
     @scope = Puppet::Parser::Scope.new
-    @one = stub 'one', :safeevaluate => "1"
-    @two = stub 'two', :safeevaluate => "2"
+    @one = Puppet::Parser::AST::Leaf.new(:value => "1")
+    @two = Puppet::Parser::AST::Leaf.new(:value => "2")
+
+    @lval = Puppet::Parser::AST::Leaf.new(:value => "one")
+    @rval = Puppet::Parser::AST::Leaf.new(:value => "two")
   end
 
-  it "should evaluate both branches" do
-    lval = stub "lval"
-    lval.expects(:safeevaluate).with(@scope)
-    rval = stub "rval"
-    rval.expects(:safeevaluate).with(@scope)
+  it "should evaluate both values" do
+    @lval.expects(:safeevaluate).with(@scope)
+    @rval.expects(:safeevaluate).with(@scope)
 
-    operator = Puppet::Parser::AST::ComparisonOperator.new :lval => lval, :operator => "==", :rval => rval
+    operator = Puppet::Parser::AST::ComparisonOperator.new :lval => @lval, :operator => "==", :rval => @rval
     operator.evaluate(@scope)
   end
 
-  it "should convert arguments strings to numbers if they are" do
+  it "should convert the arguments to numbers if they are numbers in string" do
     Puppet::Parser::Scope.expects(:number?).with("1").returns(1)
     Puppet::Parser::Scope.expects(:number?).with("2").returns(2)
 
@@ -27,25 +28,48 @@ describe Puppet::Parser::AST::ComparisonOperator do
     operator.evaluate(@scope)
   end
 
-  %w{< > <= >= ==}.each do |oper|
+  %w{< > <= >=}.each do |oper|
     it "should use string comparison #{oper} if operands are strings" do
-      lval = stub 'one', :safeevaluate => "one"
-      rval = stub 'two', :safeevaluate => "two"
-      Puppet::Parser::Scope.stubs(:number?).with("one").returns(nil)
-      Puppet::Parser::Scope.stubs(:number?).with("two").returns(nil)
-
-      operator = Puppet::Parser::AST::ComparisonOperator.new :lval => lval, :operator => oper, :rval => rval
+      operator = Puppet::Parser::AST::ComparisonOperator.new :lval => @lval, :operator => oper, :rval => @rval
       operator.evaluate(@scope).should == "one".send(oper,"two")
     end
   end
 
-  it "should fail with arguments of different types" do
-    lval = stub 'one', :safeevaluate => "one"
-    rval = stub 'two', :safeevaluate => "2"
-    Puppet::Parser::Scope.stubs(:number?).with("one").returns(nil)
-    Puppet::Parser::Scope.stubs(:number?).with("2").returns(2)
+  describe "with string comparison" do
+    it "should use matching" do
+      @rval.expects(:evaluate_match).with("one", @scope)
 
-    operator = Puppet::Parser::AST::ComparisonOperator.new :lval => lval, :operator => ">", :rval => rval
+      operator = Puppet::Parser::AST::ComparisonOperator.new :lval => @lval, :operator => "==", :rval => @rval
+      operator.evaluate(@scope)
+    end
+
+    it "should return true for :undef to '' equality" do
+      astundef = Puppet::Parser::AST::Leaf.new(:value => :undef)
+      empty = Puppet::Parser::AST::Leaf.new(:value => '')
+
+      operator = Puppet::Parser::AST::ComparisonOperator.new :lval => astundef, :operator => "==", :rval => empty
+      operator.evaluate(@scope).should be_true
+    end
+
+    [true, false].each do |result|
+      it "should return #{(result).inspect} with '==' when matching return #{result.inspect}" do
+        @rval.expects(:evaluate_match).with("one", @scope).returns result
+
+        operator = Puppet::Parser::AST::ComparisonOperator.new :lval => @lval, :operator => "==", :rval => @rval
+        operator.evaluate(@scope).should == result
+      end
+
+      it "should return #{(!result).inspect} with '!=' when matching return #{result.inspect}" do
+        @rval.expects(:evaluate_match).with("one", @scope).returns result
+
+        operator = Puppet::Parser::AST::ComparisonOperator.new :lval => @lval, :operator => "!=", :rval => @rval
+        operator.evaluate(@scope).should == !result
+      end
+    end
+  end
+
+  it "should fail with arguments of different types" do
+    operator = Puppet::Parser::AST::ComparisonOperator.new :lval => @one, :operator => ">", :rval => @rval
     lambda { operator.evaluate(@scope) }.should raise_error(ArgumentError)
   end
 
@@ -81,8 +105,8 @@ describe Puppet::Parser::AST::ComparisonOperator do
   # see ticket #1759
   %w{< > <= >=}.each do |oper|
     it "should return the correct result of using '#{oper}' to compare 10 and 9" do
-      ten = stub 'one', :safeevaluate => "10"
-      nine = stub 'two', :safeevaluate => "9"
+      ten = Puppet::Parser::AST::Leaf.new(:value => "10")
+      nine = Puppet::Parser::AST::Leaf.new(:value => "9")
       operator = Puppet::Parser::AST::ComparisonOperator.new :lval => ten, :operator => oper, :rval => nine
 
       operator.evaluate(@scope).should == 10.send(oper,9)
diff --git a/spec/unit/parser/ast/leaf_spec.rb b/spec/unit/parser/ast/leaf_spec.rb
index 379cbfde7..d21cbf573 100755
--- a/spec/unit/parser/ast/leaf_spec.rb
+++ b/spec/unit/parser/ast/leaf_spec.rb
@@ -35,6 +35,27 @@ describe Puppet::Parser::AST::Leaf do
       @leaf.evaluate_match("value", @scope)
     end
 
+    it "should convert values to number" do
+      @leaf.stubs(:safeevaluate).with(@scope).returns(@value)
+      Puppet::Parser::Scope.expects(:number?).with(@value).returns(2)
+      Puppet::Parser::Scope.expects(:number?).with("23").returns(23)
+
+      @leaf.evaluate_match("23", @scope)
+    end
+
+    it "should compare 'numberized' values" do
+      @leaf.stubs(:safeevaluate).with(@scope).returns(@value)
+      two = stub_everything 'two'
+      one = stub_everything 'one'
+
+      Puppet::Parser::Scope.stubs(:number?).with(@value).returns(one)
+      Puppet::Parser::Scope.stubs(:number?).with("2").returns(two)
+
+      one.expects(:==).with(two)
+
+      @leaf.evaluate_match("2", @scope)
+    end
+
     it "should match undef if value is an empty string" do
       @leaf.stubs(:safeevaluate).with(@scope).returns("")