Fixed #997 -- virtual defined types are no longer evaluated.

NOTE: This introduces a behaviour change, in that you previously
could realize a resource within a virtual defined resource, and now
you must realize the entire defined resource, rather than just
the contained resource.

6 files changed, 53 insertions, 42 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8a8ebdbef..ff65331cb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,9 @@
+    Fixed #997 -- virtual defined types are no longer evaluated.
+    NOTE: This introduces a behaviour change, in that you previously
+    could realize a resource within a virtual defined resource, and now
+    you must realize the entire defined resource, rather than just
+    the contained resource.
+
     Fixed #1030 - class and definition evaluation has been significantly
     refactored, fixing this problem and making the whole interplay
     between the classes, definitions, and nodes, and the Compile class much
diff --git a/spec/unit/parser/compiler.rb b/spec/unit/parser/compiler.rb
index d3039996f..6b821977d 100755
--- a/spec/unit/parser/compiler.rb
+++ b/spec/unit/parser/compiler.rb
@@ -169,7 +169,7 @@ describe Puppet::Parser::Compiler, " when compiling" do
     end
 
     it "should evaluate unevaluated resources" do
-        resource = stub 'notevaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => false
+        resource = stub 'notevaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => false, :virtual? => false
         @compiler.add_resource(@scope, resource)
 
         # We have to now mark the resource as evaluated
@@ -179,7 +179,7 @@ describe Puppet::Parser::Compiler, " when compiling" do
     end
 
     it "should not evaluate already-evaluated resources" do
-        resource = stub 'already_evaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => true
+        resource = stub 'already_evaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => true, :virtual? => false
         @compiler.add_resource(@scope, resource)
         resource.expects(:evaluate).never
         
@@ -187,10 +187,10 @@ describe Puppet::Parser::Compiler, " when compiling" do
     end
 
     it "should evaluate unevaluated resources created by evaluating other resources" do
-        resource = stub 'notevaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => false
+        resource = stub 'notevaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => false, :virtual? => false
         @compiler.add_resource(@scope, resource)
 
-        resource2 = stub 'created', :ref => "File[other]", :builtin? => false, :evaluated? => false
+        resource2 = stub 'created', :ref => "File[other]", :builtin? => false, :evaluated? => false, :virtual? => false
 
         # We have to now mark the resource as evaluated
         resource.expects(:evaluate).with { |*whatever| resource.stubs(:evaluated?).returns(true); @compiler.add_resource(@scope, resource2) }
@@ -244,6 +244,15 @@ describe Puppet::Parser::Compiler, " when compiling" do
         @compiler.add_resource(@scope, resource)
         @compiler.findresource("Yay", "foo").should equal(resource)
     end
+
+    it "should not evaluate virtual defined resources" do
+        resource = stub 'notevaluated', :ref => "File[testing]", :builtin? => false, :evaluated? => false, :virtual? => true
+        @compiler.add_resource(@scope, resource)
+
+        resource.expects(:evaluate).never
+        
+        @compiler.compile
+    end
 end
 
 describe Puppet::Parser::Compiler, " when evaluating collections" do
diff --git a/test/data/snippets/collection_within_virtual_definitions.pp b/test/data/snippets/collection_within_virtual_definitions.pp
new file mode 100644
index 000000000..3c21468b0
--- /dev/null
+++ b/test/data/snippets/collection_within_virtual_definitions.pp
@@ -0,0 +1,20 @@
+define test($name) {
+    file {"/tmp/collection_within_virtual_definitions1_$name.txt":
+        content => "File name $name\n"
+    }
+    Test2 <||>
+}
+
+define test2() {
+    file {"/tmp/collection_within_virtual_definitions2_$name.txt":
+        content => "This is a test\n"
+    }
+}
+
+node default {
+    @test {"foo":
+        name => "foo"
+    }
+    @test2 {"foo2": }
+    Test <||>
+}
diff --git a/test/data/snippets/realize_defined_types.pp b/test/data/snippets/realize_defined_types.pp
deleted file mode 100644
index a4b562258..000000000
--- a/test/data/snippets/realize_defined_types.pp
+++ /dev/null
@@ -1,13 +0,0 @@
-define testing {
-    file { "/tmp/realize_defined_test1": ensure => file }
-}
-@testing { yay: }
-
-define deeper {
-    file { "/tmp/realize_defined_test2": ensure => file }
-}
-
-@deeper { boo: }
-
-realize Testing[yay]
-realize File["/tmp/realize_defined_test2"]
diff --git a/test/language/scope.rb b/test/language/scope.rb
index 9c0e583e4..c96581a23 100755
--- a/test/language/scope.rb
+++ b/test/language/scope.rb
@@ -328,10 +328,9 @@ class TestScope < Test::Unit::TestCase
             "undef considered true")
     end
 
-    if defined? ActiveRecord
     # Verify that we recursively mark as exported the results of collectable
     # components.
-    def test_exportedcomponents
+    def test_virtual_definitions_do_not_get_evaluated
         config = mkcompiler
         parser = config.parser
 
@@ -348,7 +347,7 @@ class TestScope < Test::Unit::TestCase
             :children => [nameobj("arg")]
         )
 
-        # Create a top-level component
+        # Create a top-level define
         parser.newdefine "one", :arguments => [%w{arg}],
             :code => AST::ASTArray.new(
                 :children => [
@@ -356,27 +355,11 @@ class TestScope < Test::Unit::TestCase
                 ]
             )
 
-        # And a component that calls it
-        parser.newdefine "two", :arguments => [%w{arg}],
-            :code => AST::ASTArray.new(
-                :children => [
-                    resourcedef("one", "ptest", {"arg" => varref("arg")})
-                ]
-            )
-
-        # And then a third component that calls the second
-        parser.newdefine "three", :arguments => [%w{arg}],
-            :code => AST::ASTArray.new(
-                :children => [
-                    resourcedef("two", "yay", {"arg" => varref("arg")})
-                ]
-            )
-
-        # lastly, create an object that calls our third component
-        obj = resourcedef("three", "boo", {"arg" => "parentfoo"})
+        # create a resource that calls our third define
+        obj = resourcedef("one", "boo", {"arg" => "parentfoo"})
 
-        # And mark it as exported
-        obj.exported = true
+        # And mark it as virtual
+        obj.virtual = true
 
         # And then evaluate it
         obj.evaluate config.topscope
@@ -385,12 +368,13 @@ class TestScope < Test::Unit::TestCase
         config.send(:evaluate_generators)
 
         %w{File}.each do |type|
-            objects = config.resources.find_all { |r| r.type == type and r.exported }
+            objects = config.resources.find_all { |r| r.type == type and r.virtual }
 
-            assert(!objects.empty?, "Did not get an exported %s" % type)
+            assert(objects.empty?, "Virtual define got evaluated")
         end
     end
 
+    if defined? ActiveRecord
     # Verify that we can both store and collect an object in the same
     # run, whether it's in the same scope as a collection or a different
     # scope.
diff --git a/test/language/snippets.rb b/test/language/snippets.rb
index def3d4cf9..982ddfec4 100755
--- a/test/language/snippets.rb
+++ b/test/language/snippets.rb
@@ -424,6 +424,11 @@ class TestSnippets < Test::Unit::TestCase
         assert_file("/tmp/realize_defined_test2")
     end
 
+    def snippet_collection_within_virtual_definitions
+        assert_file("/tmp/collection_within_virtual_definitions1_foo.txt")
+        assert_file("/tmp/collection_within_virtual_definitions2_foo2.txt")
+    end
+
     def snippet_fqparents
         assert_file("/tmp/fqparent1", "Did not make file from parent class")
         assert_file("/tmp/fqparent2", "Did not make file from subclass")