Moving the ast node tests to rspec (which I could have

*sworn* I did this weekend).  In the process, I fixed
a couple of bugs related to differentiating between
nodes and classes, and then cleaned up quite a few
error messages.

9 files changed, 299 insertions, 272 deletions

diff --git a/lib/puppet/parser/ast/definition.rb b/lib/puppet/parser/ast/definition.rb
index b4a90016a..2b7506446 100644
--- a/lib/puppet/parser/ast/definition.rb
+++ b/lib/puppet/parser/ast/definition.rb
@@ -25,9 +25,9 @@ class Puppet::Parser::AST::Definition < Puppet::Parser::AST::Branch
     # Create a resource that knows how to evaluate our actual code.
     def evaluate(scope)
         # Do nothing if the resource already exists; this provides the singleton nature classes need.
-        return if scope.catalog.resource(:class, self.classname)
+        return if scope.catalog.resource(self.class.name, self.classname)
 
-        resource = Puppet::Parser::Resource.new(:type => "class", :title => self.classname, :scope => scope, :source => scope.source)
+        resource = Puppet::Parser::Resource.new(:type => self.class.name, :title => self.classname, :scope => scope, :source => scope.source)
 
         scope.catalog.tag(*resource.tags)
 
diff --git a/lib/puppet/parser/ast/hostclass.rb b/lib/puppet/parser/ast/hostclass.rb
index f49016526..8d4d01660 100644
--- a/lib/puppet/parser/ast/hostclass.rb
+++ b/lib/puppet/parser/ast/hostclass.rb
@@ -20,7 +20,7 @@ class Puppet::Parser::AST::HostClass < Puppet::Parser::AST::Definition
 
     # Make sure our parent class has been evaluated, if we have one.
     def evaluate(scope)
-        if parentclass and ! scope.catalog.resource(:class, parentclass)
+        if parentclass and ! scope.catalog.resource(self.class.name, parentclass)
             resource = parentobj.evaluate(scope)
         end
 
@@ -39,7 +39,9 @@ class Puppet::Parser::AST::HostClass < Puppet::Parser::AST::Definition
 
         pnames = nil
         if pklass = self.parentobj
-            pklass.evaluate_code(resource)
+            parent_resource = resource.scope.compiler.catalog.resource(self.class.name, pklass.classname)
+            # This shouldn't evaluate if the class has already been evaluated.
+            pklass.evaluate_code(parent_resource)
 
             scope = parent_scope(scope, pklass)
             pnames = scope.namespaces
@@ -49,14 +51,15 @@ class Puppet::Parser::AST::HostClass < Puppet::Parser::AST::Definition
         # has its own scope.
         scope = subscope(scope, resource) unless resource.title == :main
 
+        # Add the parent scope namespaces to our own.
         if pnames
             pnames.each do |ns|
                 scope.add_namespace(ns)
             end
         end
 
-        # Set the class before we do anything else, so that it's set
-        # during the evaluation and can be inspected.
+        # Set the class before we evaluate the code, so that it's set during
+        # the evaluation and can be inspected.
         scope.compiler.class_set(self.classname, scope)
 
         # Now evaluate our code, yo.
diff --git a/lib/puppet/parser/ast/node.rb b/lib/puppet/parser/ast/node.rb
index 8cebac8a8..2bf6c1882 100644
--- a/lib/puppet/parser/ast/node.rb
+++ b/lib/puppet/parser/ast/node.rb
@@ -5,34 +5,6 @@ require 'puppet/parser/ast/hostclass'
 class Puppet::Parser::AST::Node < Puppet::Parser::AST::HostClass
     @name = :node
 
-    # Evaluate the code associated with our node definition.
-    def evaluate_code(resource)
-        scope = resource.scope
-
-        # We don't have to worry about the declarativeness of node parentage,
-        # because the entry point is always a single node definition.
-        if parent = self.parentobj
-            scope = parent.evaluate_code(resource)
-        end
-
-        scope = scope.newscope(
-            :resource => resource,
-            :keyword => @keyword,
-            :source => self,
-            :namespace => "" # nodes are always in ""
-        )
-
-        # Mark our node name as a class, too, but strip it of the domain
-        # name.  Make the mark before we evaluate the code, so that it is
-        # marked within the code itself.
-        scope.compiler.class_set(self.classname, scope)
-
-        # And then evaluate our code if we have any
-        @code.safeevaluate(scope) if self.code
-
-        return scope
-    end
-
     def initialize(options)
         @parentclass = nil
         super
@@ -43,13 +15,19 @@ class Puppet::Parser::AST::Node < Puppet::Parser::AST::HostClass
         end
     end
 
+    def namespace
+        ""
+    end
+
     # Make sure node scopes are marked as such.
     def subscope(*args)
         scope = super
         scope.nodescope = true
+        scope
     end
 
     private
+
     # Search for the object matching our parent class.
     def find_parentclass
         @parser.findnode(parentclass)
diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
index 26fdd3743..68c06e500 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -31,6 +31,7 @@ class Puppet::Parser::Compiler
 
     # Store a resource in our resource table.
     def add_resource(scope, resource)
+        # Note that this will fail if the resource is not unique.
         @catalog.add_resource(resource)
 
         # And in the resource graph.  At some point, this might supercede
@@ -48,10 +49,10 @@ class Puppet::Parser::Compiler
     # the scope in which it was evaluated, so that we can look it up later.
     def class_set(name, scope)
         if existing = @class_scopes[name]
-            if existing.nodescope? or scope.nodescope?
+            if existing.nodescope? != scope.nodescope?
                 raise Puppet::ParseError, "Cannot have classes, nodes, or definitions with the same name"
             else
-                raise Puppet::DevError, "Somehow evaluated the same class twice"
+                raise Puppet::DevError, "Somehow evaluated %s %s twice" % [ existing.nodescope? ? "node" : "class", name]
             end
         end
         @class_scopes[name] = scope
diff --git a/lib/puppet/parser/resource/reference.rb b/lib/puppet/parser/resource/reference.rb
index ea53b421a..c59748049 100644
--- a/lib/puppet/parser/resource/reference.rb
+++ b/lib/puppet/parser/resource/reference.rb
@@ -37,10 +37,14 @@ class Puppet::Parser::Resource::Reference < Puppet::ResourceReference
                 if self.title == :main
                     tmp = @scope.findclass("")
                 else
-                    tmp = @scope.findclass(self.title)
+                    unless tmp = @scope.findclass(self.title)
+                        fail Puppet::ParseError, "Could not find class '%s'" % self.title
+                    end
                 end
             when "Node": # look for node definitions
-                tmp = @scope.parser.nodes[self.title]
+                unless tmp = @scope.parser.nodes[self.title]
+                    fail Puppet::ParseError, "Could not find node '%s'" % self.title
+                end
             else # normal definitions
                 # We have to swap these variables around so the errors are right.
                 tmp = @scope.finddefine(self.type)
diff --git a/spec/unit/parser/ast/definition.rb b/spec/unit/parser/ast/definition.rb
index ba80894e8..f236e23b7 100755
--- a/spec/unit/parser/ast/definition.rb
+++ b/spec/unit/parser/ast/definition.rb
@@ -44,4 +44,152 @@ describe Puppet::Parser::AST::Definition, "when evaluating" do
 #    it "should not copy the resource's title as the name if 'name' is one of the resource parameters"
 #
 #    it "should evaluate the associated code with the new scope"
+
+    def test_initialize
+        parser = mkparser
+
+        # Create a new definition
+        klass = parser.newdefine "yayness",
+            :arguments => [["owner", stringobj("nobody")], %w{mode}],
+            :code => AST::ASTArray.new(
+                :children => [resourcedef("file", "/tmp/$name",
+                        "owner" => varref("owner"), "mode" => varref("mode"))]
+            )
+
+        # Test validattr? a couple different ways
+        [:owner, "owner", :schedule, "schedule"].each do |var|
+            assert(klass.validattr?(var), "%s was not considered valid" % var.inspect)
+        end
+
+        [:random, "random"].each do |var|
+            assert(! klass.validattr?(var), "%s was considered valid" % var.inspect)
+        end
+
+    end
+
+    def test_evaluate
+        parser = mkparser
+        config = mkcompiler
+        config.send(:evaluate_main)
+        scope = config.topscope
+        klass = parser.newdefine "yayness",
+            :arguments => [["owner", stringobj("nobody")], %w{mode}],
+            :code => AST::ASTArray.new(
+                :children => [resourcedef("file", "/tmp/$name",
+                        "owner" => varref("owner"), "mode" => varref("mode"))]
+            )
+
+        resource = Puppet::Parser::Resource.new(
+            :title => "first",
+            :type => "yayness",
+            :exported => false,
+            :virtual => false,
+            :scope => scope,
+            :source => scope.source
+        )
+        resource.send(:set_parameter, "name", "first")
+        resource.send(:set_parameter, "mode", "755")
+
+        resource.stubs(:title)
+        assert_nothing_raised do
+            klass.evaluate_code(resource)
+        end
+
+        firstobj = config.findresource("File[/tmp/first]")
+        assert(firstobj, "Did not create /tmp/first obj")
+
+        assert_equal("File", firstobj.type)
+        assert_equal("/tmp/first", firstobj.title)
+        assert_equal("nobody", firstobj[:owner])
+        assert_equal("755", firstobj[:mode])
+
+        # Make sure we can't evaluate it with the same args
+        assert_raise(Puppet::ParseError) do
+            klass.evaluate_code(resource)
+        end
+
+        # Now create another with different args
+        resource2 = Puppet::Parser::Resource.new(
+            :title => "second",
+            :type => "yayness",
+            :exported => false,
+            :virtual => false,
+            :scope => scope,
+            :source => scope.source
+        )
+        resource2.send(:set_parameter, "name", "second")
+        resource2.send(:set_parameter, "mode", "755")
+        resource2.send(:set_parameter, "owner", "daemon")
+
+        assert_nothing_raised do
+            klass.evaluate_code(resource2)
+        end
+
+        secondobj = config.findresource("File[/tmp/second]")
+        assert(secondobj, "Did not create /tmp/second obj")
+
+        assert_equal("File", secondobj.type)
+        assert_equal("/tmp/second", secondobj.title)
+        assert_equal("daemon", secondobj[:owner])
+        assert_equal("755", secondobj[:mode])
+    end
+
+    # #539 - definitions should support both names and titles
+    def test_names_and_titles
+        parser = mkparser
+        scope = mkscope :parser => parser
+
+        [
+            {:name => "one", :title => "two"},
+            {:title => "mytitle"}
+        ].each_with_index do |hash, i|
+            # Create a definition that uses both name and title.  Put this
+            # inside the loop so the subscope expectations work.
+            klass = parser.newdefine "yayness%s" % i
+
+            resource = Puppet::Parser::Resource.new(
+                :title => hash[:title],
+                :type => "yayness%s" % i,
+                :exported => false,
+                :virtual => false,
+                :scope => scope,
+                :source => scope.source
+            )
+
+            subscope = klass.subscope(scope, resource)
+
+            klass.expects(:subscope).returns(subscope)
+
+            if hash[:name]
+                resource.stubs(:to_hash).returns({:name => hash[:name]})
+            end
+
+            assert_nothing_raised("Could not evaluate definition with %s" % hash.inspect) do
+                klass.evaluate_code(resource)
+            end
+
+            name = hash[:name] || hash[:title]
+            title = hash[:title]
+
+            assert_equal(name, subscope.lookupvar("name"),
+                "Name did not get set correctly")
+            assert_equal(title, subscope.lookupvar("title"),
+                "title did not get set correctly")
+
+            [:name, :title].each do |param|
+                val = resource.send(param)
+                assert(subscope.tags.include?(val),
+                    "Scope was not tagged with %s '%s'" % [param, val])
+            end
+        end
+    end
+
+    # Testing the root cause of #615.  We should be using the fqname for the type, instead
+    # of just the short name.
+    def test_fully_qualified_types
+        parser = mkparser
+        klass = parser.newclass("one::two")
+
+        assert_equal("one::two", klass.classname, "Class did not get fully qualified class name")
+    end
 end
diff --git a/spec/unit/parser/ast/node.rb b/spec/unit/parser/ast/node.rb
new file mode 100755
index 000000000..340630194
--- /dev/null
+++ b/spec/unit/parser/ast/node.rb
@@ -0,0 +1,127 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+module ASTNodeTesting
+    def setup
+        @node = Puppet::Node.new "testnode"
+        @parser = Puppet::Parser::Parser.new :environment => "development"
+        @scope_resource = stub 'scope_resource', :builtin? => true
+        @compiler = Puppet::Parser::Compiler.new(@node, @parser)
+
+        @scope = @compiler.topscope
+    end
+end
+
+describe Puppet::Parser::AST::Node, "when evaluating" do
+    include ASTNodeTesting
+
+    before do
+        @top = @parser.newnode("top").shift
+        @middle = @parser.newnode("middle", :parent => "top").shift
+    end
+
+    it "should create a resource that references itself" do
+        @top.evaluate(@scope)
+
+        @compiler.catalog.resource(:node, "top").should be_an_instance_of(Puppet::Parser::Resource)
+    end
+
+    it "should evaluate the parent class if one exists" do
+        @middle.evaluate(@scope)
+
+        @compiler.catalog.resource(:node, "top").should be_an_instance_of(Puppet::Parser::Resource)
+    end
+
+    it "should fail to evaluate if a parent class is defined but cannot be found" do
+        othertop = @parser.newnode("something", :parent => "yay").shift
+        lambda { othertop.evaluate(@scope) }.should raise_error(Puppet::ParseError)
+    end
+
+    it "should not create a new resource if one already exists" do
+        @compiler.catalog.expects(:resource).with(:node, "top").returns("something")
+        @compiler.catalog.expects(:add_resource).never
+        @top.evaluate(@scope)
+    end
+
+    it "should not create a new parent resource if one already exists and it has a parent class" do
+        @top.evaluate(@scope)
+
+        top_resource = @compiler.catalog.resource(:node, "top")
+
+        @middle.evaluate(@scope)
+
+        @compiler.catalog.resource(:node, "top").should equal(top_resource)
+    end
+
+    # #795 - tag before evaluation.
+    it "should tag the catalog with the resource tags when it is evaluated" do
+        @middle.evaluate(@scope)
+
+        @compiler.catalog.should be_tagged("middle")
+    end
+
+    it "should tag the catalog with the parent class tags when it is evaluated" do
+        @middle.evaluate(@scope)
+
+        @compiler.catalog.should be_tagged("top")
+    end
+end
+
+describe Puppet::Parser::AST::Node, "when evaluating code" do
+    include ASTNodeTesting
+
+    before do
+        @top_resource = stub "top_resource"
+        @top = @parser.newnode("top", :code => @top_resource).shift
+
+        @middle_resource = stub "middle_resource"
+        @middle = @parser.newnode("middle", :parent => "top", :code => @middle_resource).shift
+    end
+
+    it "should evaluate the code referred to by the class" do
+        @top_resource.expects(:safeevaluate)
+
+        resource = @top.evaluate(@scope)
+
+        @top.evaluate_code(resource)
+    end
+
+    it "should evaluate the parent class's code if it has a parent" do
+        @top_resource.expects(:safeevaluate)
+        @middle_resource.expects(:safeevaluate)
+
+        resource = @middle.evaluate(@scope)
+
+        @middle.evaluate_code(resource)
+    end
+
+    it "should not evaluate the parent class's code if the parent has already been evaluated" do
+        @top_resource.stubs(:safeevaluate)
+        resource = @top.evaluate(@scope)
+        @top.evaluate_code(resource)
+
+        @top_resource.expects(:safeevaluate).never
+        @middle_resource.stubs(:safeevaluate)
+        resource = @middle.evaluate(@scope)
+        @middle.evaluate_code(resource)
+    end
+
+    it "should use the parent class's scope as its parent scope" do
+        @top_resource.stubs(:safeevaluate)
+        @middle_resource.stubs(:safeevaluate)
+        resource = @middle.evaluate(@scope)
+        @middle.evaluate_code(resource)
+
+        @compiler.class_scope(@middle).parent.should equal(@compiler.class_scope(@top))
+    end
+
+    it "should add the parent class's namespace to its namespace search path" do
+        @top_resource.stubs(:safeevaluate)
+        @middle_resource.stubs(:safeevaluate)
+        resource = @middle.evaluate(@scope)
+        @middle.evaluate_code(resource)
+
+        @compiler.class_scope(@middle).namespaces.should be_include(@top.namespace)
+    end
+end
diff --git a/test/language/ast/definition.rb b/test/language/ast/definition.rb
deleted file mode 100755
index 1585c5b1d..000000000
--- a/test/language/ast/definition.rb
+++ /dev/null
@@ -1,166 +0,0 @@
-#!/usr/bin/env ruby
-#
-#  Created by Luke A. Kanies on 2006-02-20.
-#  Copyright (c) 2006. All rights reserved.
-
-require File.dirname(__FILE__) + '/../../lib/puppettest'
-
-require 'puppettest'
-require 'mocha'
-require 'puppettest/parsertesting'
-require 'puppettest/resourcetesting'
-
-class TestASTDefinition < Test::Unit::TestCase
-	include PuppetTest
-	include PuppetTest::ParserTesting
-    include PuppetTest::ResourceTesting
-	AST = Puppet::Parser::AST
-
-    def test_initialize
-        parser = mkparser
-
-        # Create a new definition
-        klass = parser.newdefine "yayness",
-            :arguments => [["owner", stringobj("nobody")], %w{mode}],
-            :code => AST::ASTArray.new(
-                :children => [resourcedef("file", "/tmp/$name",
-                        "owner" => varref("owner"), "mode" => varref("mode"))]
-            )
-
-        # Test validattr? a couple different ways
-        [:owner, "owner", :schedule, "schedule"].each do |var|
-            assert(klass.validattr?(var), "%s was not considered valid" % var.inspect)
-        end
-
-        [:random, "random"].each do |var|
-            assert(! klass.validattr?(var), "%s was considered valid" % var.inspect)
-        end
-
-    end
-
-    def test_evaluate
-        parser = mkparser
-        config = mkcompiler
-        config.send(:evaluate_main)
-        scope = config.topscope
-        klass = parser.newdefine "yayness",
-            :arguments => [["owner", stringobj("nobody")], %w{mode}],
-            :code => AST::ASTArray.new(
-                :children => [resourcedef("file", "/tmp/$name",
-                        "owner" => varref("owner"), "mode" => varref("mode"))]
-            )
-
-        resource = Puppet::Parser::Resource.new(
-            :title => "first",
-            :type => "yayness",
-            :exported => false,
-            :virtual => false,
-            :scope => scope,
-            :source => scope.source
-        )
-        resource.send(:set_parameter, "name", "first")
-        resource.send(:set_parameter, "mode", "755")
-
-        resource.stubs(:title)
-        assert_nothing_raised do
-            klass.evaluate_code(resource)
-        end
-
-        firstobj = config.findresource("File[/tmp/first]")
-        assert(firstobj, "Did not create /tmp/first obj")
-
-        assert_equal("File", firstobj.type)
-        assert_equal("/tmp/first", firstobj.title)
-        assert_equal("nobody", firstobj[:owner])
-        assert_equal("755", firstobj[:mode])
-
-        # Make sure we can't evaluate it with the same args
-        assert_raise(Puppet::ParseError) do
-            klass.evaluate_code(resource)
-        end
-
-        # Now create another with different args
-        resource2 = Puppet::Parser::Resource.new(
-            :title => "second",
-            :type => "yayness",
-            :exported => false,
-            :virtual => false,
-            :scope => scope,
-            :source => scope.source
-        )
-        resource2.send(:set_parameter, "name", "second")
-        resource2.send(:set_parameter, "mode", "755")
-        resource2.send(:set_parameter, "owner", "daemon")
-
-        assert_nothing_raised do
-            klass.evaluate_code(resource2)
-        end
-
-        secondobj = config.findresource("File[/tmp/second]")
-        assert(secondobj, "Did not create /tmp/second obj")
-
-        assert_equal("File", secondobj.type)
-        assert_equal("/tmp/second", secondobj.title)
-        assert_equal("daemon", secondobj[:owner])
-        assert_equal("755", secondobj[:mode])
-    end
-
-    # #539 - definitions should support both names and titles
-    def test_names_and_titles
-        parser = mkparser
-        scope = mkscope :parser => parser
-
-        [
-            {:name => "one", :title => "two"},
-            {:title => "mytitle"}
-        ].each_with_index do |hash, i|
-            # Create a definition that uses both name and title.  Put this
-            # inside the loop so the subscope expectations work.
-            klass = parser.newdefine "yayness%s" % i
-
-            resource = Puppet::Parser::Resource.new(
-                :title => hash[:title],
-                :type => "yayness%s" % i,
-                :exported => false,
-                :virtual => false,
-                :scope => scope,
-                :source => scope.source
-            )
-
-            subscope = klass.subscope(scope, resource)
-
-            klass.expects(:subscope).returns(subscope)
-
-            if hash[:name]
-                resource.stubs(:to_hash).returns({:name => hash[:name]})
-            end
-
-            assert_nothing_raised("Could not evaluate definition with %s" % hash.inspect) do
-                klass.evaluate_code(resource)
-            end
-
-            name = hash[:name] || hash[:title]
-            title = hash[:title]
-
-            assert_equal(name, subscope.lookupvar("name"),
-                "Name did not get set correctly")
-            assert_equal(title, subscope.lookupvar("title"),
-                "title did not get set correctly")
-
-            [:name, :title].each do |param|
-                val = resource.send(param)
-                assert(subscope.tags.include?(val),
-                    "Scope was not tagged with %s '%s'" % [param, val])
-            end
-        end
-    end
-
-    # Testing the root cause of #615.  We should be using the fqname for the type, instead
-    # of just the short name.
-    def test_fully_qualified_types
-        parser = mkparser
-        klass = parser.newclass("one::two")
-
-        assert_equal("one::two", klass.classname, "Class did not get fully qualified class name")
-    end
-end
diff --git a/test/language/ast/node.rb b/test/language/ast/node.rb
deleted file mode 100755
index a90f05adf..000000000
--- a/test/language/ast/node.rb
+++ /dev/null
@@ -1,68 +0,0 @@
-#!/usr/bin/env ruby
-#
-#  Created by Luke A. Kanies on 2008-02-09.
-#  Copyright (c) 2008. All rights reserved.
-
-require File.dirname(__FILE__) + '/../../lib/puppettest'
-
-require 'puppettest'
-require 'mocha'
-require 'puppettest/parsertesting'
-require 'puppettest/resourcetesting'
-
-class TestASTNode < Test::Unit::TestCase
-	include PuppetTest
-	include PuppetTest::ParserTesting
-    include PuppetTest::ResourceTesting
-	AST = Puppet::Parser::AST
-
-    def test_node
-        scope = mkscope
-        parser = scope.compiler.parser
-
-        # Define a base node
-        basenode = parser.newnode "basenode", :code => AST::ASTArray.new(:children => [
-            resourcedef("file", "/tmp/base", "owner" => "root")
-        ])
-
-        # Now define a subnode
-        nodes = parser.newnode ["mynode", "othernode"],
-            :code => AST::ASTArray.new(:children => [
-                resourcedef("file", "/tmp/mynode", "owner" => "root"),
-                resourcedef("file", "/tmp/basenode", "owner" => "daemon")
-        ])
-
-        assert_instance_of(Array, nodes)
-
-        # Make sure we can find them all.
-        %w{mynode othernode}.each do |node|
-            assert(parser.nodes[node], "Could not find %s" % node)
-        end
-        mynode = parser.nodes["mynode"]
-
-        # Now try evaluating the node
-        assert_nothing_raised do
-            mynode.evaluate_code scope.resource
-        end
-
-        # Make sure that we can find each of the files
-        myfile = scope.findresource "File[/tmp/mynode]"
-        assert(myfile, "Could not find file from node")
-        assert_equal("root", myfile[:owner])
-
-        basefile = scope.findresource "File[/tmp/basenode]"
-        assert(basefile, "Could not find file from base node")
-        assert_equal("daemon", basefile[:owner])
-
-        # Now make sure we can evaluate nodes with parents
-        child = parser.newnode(%w{child}, :parent => "basenode").shift
-
-        newscope = mkscope :parser => parser
-        assert_nothing_raised do
-            child.evaluate_code newscope.resource
-        end
-
-        assert(newscope.findresource("File[/tmp/base]"),
-            "Could not find base resource")
-    end
-end