Converting the file 'source' property to a parameter.

This makes a lot of sense because source was always
more of a metaparameter than a property -- it affected
the 'should' values of other properties, but it shouldn't
have done any other work.

It will hopefully make everything else much cleaner.

This is such a large commit mostly because of the need
to fix a lot of tests.

Signed-off-by: Luke Kanies <luke@madstop.com>

14 files changed, 248 insertions, 484 deletions

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index 40bb32ff6..f82438903 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -487,15 +487,6 @@ class Puppet::Parameter
         @value = munge(value)
     end
 
-    def inspect
-        s = "Parameter(%s = %s" % [self.name, self.value || "nil"]
-        if defined? @resource
-            s += ", @resource = %s)" % @resource
-        else
-            s += ")"
-        end
-    end
-
     # Retrieve the resource's provider.  Some types don't have providers, in which
     # case we return the resource object itself.
     def provider
diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 96f6ff1bf..b8dbd6e77 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -188,16 +188,6 @@ class Puppet::Property < Puppet::Parameter
         end
     end
 
-    def inspect
-        str = "Property('%s', " % self.name
-
-        if defined? @should and @should
-            str += "@should = '%s')" % @should.join(", ")
-        else
-            str += "@should = nil)"
-        end
-    end
-
     # Determine whether the property is in-sync or not.  If @should is
     # not defined or is set to a non-true value, then we do not have
     # a valid value for it and thus consider the property to be in-sync
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 6038aa50b..dfb909f3f 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -240,6 +240,9 @@ module Puppet
             CREATORS.each do |param|
                 count += 1 if self.should(param)
             end
+            if @parameters.include?(:source)
+                count += 1
+            end
             if count > 1
                 self.fail "You cannot specify more than one of %s" % CREATORS.collect { |p| p.to_s}.join(", ")
             end
@@ -293,6 +296,12 @@ module Puppet
             return asuser
         end
 
+        # Does the file currently exist?  Just checks for whether
+        # we have a stat
+        def exist?
+            stat ? true : false
+        end
+
         # We have to do some extra finishing, to retrieve our bucket if
         # there is one.
         def finish
@@ -478,6 +487,7 @@ module Puppet
             end
 
             return self.class.create(options)
+            #return catalog.create_implicit_resource(:file, options)
         end
 
         # Files handle paths specially, because they just lengthen their
@@ -576,6 +586,7 @@ module Puppet
 
             total = self[:source].collect do |source|
                 next unless result = perform_recursion(source)
+                return if top = result.find { |r| r.relative_path == "." } and top.ftype != "directory"
                 result.each { |data| data.source = "%s/%s" % [source, data.relative_path] }
                 break result if result and ! result.empty? and sourceselect == :first
                 result
@@ -593,14 +604,14 @@ module Puppet
 
             total.each do |meta|
                 if meta.relative_path == "."
-                    property(:source).metadata = meta
+                    parameter(:source).metadata = meta
                     next
                 end
                 children[meta.relative_path] ||= newchild(meta.relative_path)
                 children[meta.relative_path][:source] = meta.source
                 children[meta.relative_path][:checksum] = :md5 if meta.ftype == "file"
 
-                children[meta.relative_path].property(:source).metadata = meta
+                children[meta.relative_path].parameter(:source).metadata = meta
             end
 
             # If we're purging resources, then delete any resource that isn't on the
@@ -681,22 +692,10 @@ module Puppet
 
         # a wrapper method to make sure the file exists before doing anything
         def retrieve
-            unless stat = self.stat(true)
-
-                propertyvalues = properties().inject({}) { |hash, property|
-                                     hash[property] = :absent
-                                     hash
-                                  }
-
-                # If the file doesn't exist but we have a source, then call
-                # set our 'should' values based on the source file.
-                if @parameters.include?(:source)
-                    @parameters[:source].copy_source_values
-                end
-                return propertyvalues
+            if source = parameter(:source)
+                source.copy_source_values
             end
-
-            currentpropvalues()
+            super
         end
 
         # Set the checksum, from another property.  There are multiple
@@ -715,6 +714,28 @@ module Puppet
             end
         end
 
+        # Should this thing be a normal file?  This is a relatively complex
+        # way of determining whether we're trying to create a normal file,
+        # and it's here so that the logic isn't visible in the content property.
+        def should_be_file?
+            return true if self[:ensure] == :file
+
+            # I.e., it's set to something like "directory"
+            return false if e = self[:ensure] and e != :present
+
+            # The user doesn't really care, apparently
+            if self[:ensure] == :present
+                return true unless s = stat
+                return true if s.ftype == "file"
+                return false
+            end
+
+            # If we've gotten here, then :ensure isn't set
+            return true if self[:content]
+            return true if stat and stat.ftype == "file"
+            return false
+        end
+
         # Stat our file.  Depending on the value of the 'links' attribute, we
         # use either 'stat' or 'lstat', and we expect the properties to use the
         # resulting stat object accordingly (mostly by testing the 'ftype'
diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index 1eb1423aa..169ba9078 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -1,6 +1,9 @@
+require 'puppet/util/checksums'
+
 module Puppet
     Puppet::Type.type(:file).newproperty(:content) do
         include Puppet::Util::Diff
+        include Puppet::Util::Checksums
 
         desc "Specify the contents of a file as a string.  Newlines, tabs, and
             spaces can be specified using the escaped syntax (e.g., \\n for a
@@ -23,7 +26,11 @@ module Puppet
             `PuppetTemplating templating`:trac:."
 
         def change_to_s(currentvalue, newvalue)
-            newvalue = "{md5}" + Digest::MD5.hexdigest(newvalue)
+            if source = resource.parameter(:source)
+                newvalue = source.metadata.checksum
+            else
+                newvalue = "{md5}" + Digest::MD5.hexdigest(newvalue)
+            end
             if currentvalue == :absent
                 return "created file with contents %s" % newvalue
             else
@@ -31,18 +38,40 @@ module Puppet
                 return "changed file contents from %s to %s" % [currentvalue, newvalue]
             end
         end
+        
+        def content
+            self.should || (s = resource.parameter(:source) and s.content)
+        end
 
         # Override this method to provide diffs if asked for.
         # Also, fix #872: when content is used, and replace is true, the file
         # should be insync when it exists
         def insync?(is)
-            if ! @resource.replace? and File.exists?(@resource[:path])
+            if resource.should_be_file?
+                return false if is == :absent
+            else
+                return true
+            end
+
+            return true if ! @resource.replace?
+
+            if self.should
+                return super
+            elsif source = resource.parameter(:source)
+                unless sum_method = sumtype(source.checksum)
+                    fail "Could not extract checksum type from source checksum '%s'" % source.checksum
+                end
+
+                newsum = "{%s}" % sum_method + send(sum_method, is)
+                result = (newsum == source.checksum)
+            else
+                # We've got no content specified, and no source from which to
+                # get content.
                 return true
             end
 
-            result = super
-            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path])
-                string_file_diff(@resource[:path], self.should)
+            if ! result and Puppet[:show_diff]
+                string_file_diff(@resource[:path], content)
             end
             return result
         end
@@ -50,31 +79,31 @@ module Puppet
         def retrieve
             return :absent unless stat = @resource.stat
 
-            return self.should if stat.ftype == "link" and @resource[:links] == :ignore
-
             # Don't even try to manage the content on directories
             return nil if stat.ftype == "directory"
 
             begin
-                currentvalue = File.read(@resource[:path])
-                return currentvalue
+                return File.read(@resource[:path])
             rescue => detail
-                raise Puppet::Error, "Could not read %s: %s" %
-                    [@resource.title, detail]
+                raise Puppet::Error, "Could not read %s: %s" % [@resource.title, detail]
             end
         end
 
         # Make sure we're also managing the checksum property.
         def should=(value)
             super
-            @resource.newattr(:checksum) unless @resource.property(:checksum)
+            @resource.newattr(:checksum) unless @resource.parameter(:checksum)
         end
 
         # Just write our content out to disk.
         def sync
             return_event = @resource.stat ? :file_changed : :file_created
             
-            @resource.write(self.should, :content)
+            # We're safe not testing for the 'source' if there's no 'should'
+            # because we wouldn't have gotten this far if there weren't at least
+            # one valid value somewhere.
+            content = self.should || resource.parameter(:source).content
+            @resource.write(content, :content)
 
             return return_event
         end
diff --git a/lib/puppet/type/file/ensure.rb b/lib/puppet/type/file/ensure.rb
index a9ddc2dba..ea89a7d83 100755
--- a/lib/puppet/type/file/ensure.rb
+++ b/lib/puppet/type/file/ensure.rb
@@ -111,10 +111,8 @@ module Puppet
         end
 
         def change_to_s(currentvalue, newvalue)
-            if property = (@resource.property(:content) || @resource.property(:source)) and ! property.insync?(currentvalue)
-                currentvalue = property.retrieve
-                
-                return property.change_to_s(property.retrieve, property.should)
+            if property = @resource.property(:content) and ! property.insync?(currentvalue)
+                return property.change_to_s(currentvalue, property.should)
             else
                 super(currentvalue, newvalue)
             end
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 04a6931f9..ba5fcbd0e 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -8,7 +8,7 @@ module Puppet
     # the file down.  If the remote file is a dir or a link or whatever, then
     # this state, during retrieval, modifies the appropriate other states
     # so that things get taken care of appropriately.
-    Puppet.type(:file).newproperty(:source) do
+    Puppet.type(:file).newparam(:source) do
         include Puppet::Util::Diff
 
         attr_accessor :source, :local
@@ -63,27 +63,24 @@ module Puppet
             to ``follow`` if any remote sources are links.
             "
 
-        uncheckable
-        
-        validate do |source|
-            begin
-                uri = URI.parse(URI.escape(source))
-            rescue => detail
-                self.fail "Could not understand source %s: %s" % [source, detail.to_s]
-            end
+        validate do |sources|
+            sources = [sources] unless sources.is_a?(Array)
+            sources.each do |source|
+                begin
+                    uri = URI.parse(URI.escape(source))
+                rescue => detail
+                    self.fail "Could not understand source %s: %s" % [source, detail.to_s]
+                end
 
-            unless uri.scheme.nil? or %w{file puppet}.include?(uri.scheme)
-                self.fail "Cannot use URLs of type '%s' as source for fileserving" % [uri.scheme]
+                unless uri.scheme.nil? or %w{file puppet}.include?(uri.scheme)
+                    self.fail "Cannot use URLs of type '%s' as source for fileserving" % [uri.scheme]
+                end
             end
         end
             
-        munge do |source|
-            # if source.is_a? Symbol
-            #     return source
-            # end
-
-            # Remove any trailing slashes
-            source.sub(/\/$/, '')
+        munge do |sources|
+            sources = [sources] unless sources.is_a?(Array)
+            sources.collect { |source| source.sub(/\/$/, '') }
         end
         
         def change_to_s(currentvalue, newvalue)
@@ -124,6 +121,7 @@ module Puppet
             # if a value has not already been provided.
             [:owner, :mode, :group, :checksum].each do |param|
                 next if param == :owner and Puppet::Util::SUIDManager.uid != 0
+                next if param == :checksum and metadata.ftype == "directory"
                 unless value = @resource[param] and value != :absent
                     @resource[param] = metadata.send(param)
                 end
@@ -143,34 +141,8 @@ module Puppet
             @content = nil
         end
 
-        # Use the remote metadata to see if we're in sync.
-        # LAK:NOTE This method should still get refactored.
-        def insync?(currentvalue)
-            # the only thing this actual state can do is copy files around.  Therefore,
-            # only pay attention if the remote is a file.
-            return true unless metadata.ftype == "file" 
-            
-            # The file is not in sync if it doesn't even exist.
-            return false unless resource.stat
-            
-            # The file is considered in sync if it exists and 'replace' is false.
-            return true unless resource.replace?
-
-            # Now, we just check to see if the checksums are the same
-            parentchecksum = @resource.property(:checksum).retrieve
-            result = (!parentchecksum.nil? and (parentchecksum == metadata.checksum))
-
-            # Diff the contents if they ask it.  This is quite annoying -- we need to do this in
-            # 'insync?' because they might be in noop mode, but we don't want to do the file
-            # retrieval twice, so we cache the value.
-            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path])
-                string_file_diff(@resource[:path], content)
-            end
-            return result
-        end
-
         def pinparams
-            [:mode, :type, :owner, :group]
+            [:mode, :type, :owner, :group, :content]
         end
 
         def found?
@@ -183,8 +155,8 @@ module Puppet
         attr_writer :metadata
         def metadata
             unless defined?(@metadata) and @metadata
-                return @metadata = nil unless should
-                should.each do |source|
+                return @metadata = nil unless value
+                value.each do |source|
                     begin
                         if data = Puppet::FileServing::Metadata.find(source)
                             @metadata = data
@@ -195,43 +167,20 @@ module Puppet
                         fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail]
                     end
                 end
-                fail "Could not retrieve information from source(s) %s" % @should.join(", ") unless @metadata
+                fail "Could not retrieve information from source(s) %s" % value.join(", ") unless @metadata
             end
             return @metadata
         end
 
-        # Just call out to our copy method.  Hopefully we'll refactor 'source' to
-        # be a parameter soon, in which case 'retrieve' is unnecessary.
-        def retrieve
-            copy_source_values
-        end
-        
-        # Return the whole array, rather than the first item.
-        def should
-            @should
-        end
-        
         # Make sure we're also checking the checksum
-        def should=(value)
+        def value=(value)
             super
 
             checks = (pinparams + [:ensure])
             checks.delete(:checksum)
             
-            @resource[:check] = checks
-            @resource[:checksum] = :md5 unless @resource.property(:checksum)
-        end
-
-        def sync
-            exists = FileTest.exist?(@resource[:path])
-
-            @resource.write(content, :source, @metadata.checksum)
-
-            if exists
-                return :file_changed
-            else
-                return :file_created
-            end
+            resource[:check] = checks
+            resource[:checksum] = :md5 unless resource.property(:checksum)
         end
     end
 end
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index c9e0d19c6..5e32332d3 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -175,10 +175,39 @@ describe Puppet::Type.type(:file) do
 
             # And make sure it's changed
             File.read(dest).should == "bar"
+        end
+
+        it "should be able to copy individual files even if recurse has been specified" do
+            source = tmpfile("source")
+            dest = tmpfile("dest")
+
+            File.open(source, "w") { |f| f.print "foo" }
+
+            file = Puppet::Type::File.create(:name => dest, :source => source, :recurse => true)
 
+            catalog = Puppet::Node::Catalog.new
+            catalog.add_resource file
+            catalog.apply
+
+            File.read(dest).should == "foo"
         end
     end
 
+    it "should be able to create files when 'content' is specified but 'ensure' is not" do
+        dest = tmpfile("files_with_content")
+
+        file = Puppet.type(:file).create(
+            :name => dest,
+            :content => "this is some content, yo"
+        )
+
+        catalog = Puppet::Node::Catalog.new
+        catalog.add_resource file
+        catalog.apply
+
+        File.read(dest).should == "this is some content, yo"
+    end
+
     it "should create files with content if both 'content' and 'ensure' are set" do
         dest = tmpfile("files_with_content")
 
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 533f19556..8c1eef177 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -14,25 +14,59 @@ describe Puppet::Type.type(:file) do
         @file.catalog = @catalog
     end
 
-    describe "when used with content and replace=>false" do
-        before do
-            @file[:content] = "foo"
-            @file[:replace] = false
-        end
+    it "should have a method for determining if the file is present" do
+        @file.must respond_to(:exist?)
+    end
 
-        it "should be insync if the file exists and the content is different" do
-            File.open(@path, "w") do |f| f.puts "bar" end
-            @file.property(:content).insync?("bar").should be_true
-        end
+    it "should be considered existent if it can be stat'ed" do
+        @file.expects(:stat).returns mock('stat')
+        @file.must be_exist
+    end
 
-        it "should be insync if the file exists and the content is right" do
-            File.open(@path, "w") do |f| f.puts "foo" end
-            @file.property(:content).insync?("foo").should be_true
-        end
+    it "should be considered nonexistent if it can not be stat'ed" do
+        @file.expects(:stat).returns nil
+        @file.must_not be_exist
+    end
 
-        it "should not be insync if the file does not exist" do
-            @file.property(:content).insync?(:nil).should be_false
-        end
+    it "should have a method for determining if the file should be a normal file" do
+        @file.must respond_to(:should_be_file?)
+    end
+
+    it "should be a file if :ensure is set to :file" do
+        @file[:ensure] = :file
+        @file.must be_should_be_file
+    end
+
+    it "should be a file if :ensure is set to :present and the file exists as a normal file" do
+        @file.stubs(:stat).returns(mock('stat', :ftype => "file"))
+        @file[:ensure] = :present
+        @file.must be_should_be_file
+    end
+
+    it "should not be a file if :ensure is set to something other than :file" do
+        @file[:ensure] = :directory
+        @file.must_not be_should_be_file
+    end
+
+    it "should not be a file if :ensure is set to :present and the file exists but is not a normal file" do
+        @file.stubs(:stat).returns(mock('stat', :ftype => "directory"))
+        @file[:ensure] = :present
+        @file.must_not be_should_be_file
+    end
+
+    it "should be a file if :ensure is not set and :content is" do
+        @file[:content] = "foo"
+        @file.must be_should_be_file
+    end
+
+    it "should be a file if neither :ensure nor :content is set but the file exists as a normal file" do
+        @file.stubs(:stat).returns(mock("stat", :ftype => "file"))
+        @file.must be_should_be_file
+    end
+
+    it "should not be a file if neither :ensure nor :content is set but the file exists but not as a normal file" do
+        @file.stubs(:stat).returns(mock("stat", :ftype => "directory"))
+        @file.must_not be_should_be_file
     end
 
     describe "when managing links" do
@@ -149,7 +183,7 @@ describe Puppet::Type.type(:file) do
     describe "when flushing" do
         it "should flush all properties that respond to :flush" do
             @resource = Puppet.type(:file).create(:path => "/foo/bar", :source => "/bar/foo")
-            @resource.property(:source).expects(:flush)
+            @resource.parameter(:source).expects(:flush)
             @resource.flush
         end
 
@@ -318,9 +352,11 @@ describe Puppet::Type.type(:file) do
 
             @first = Puppet::FileServing::Metadata.new("/my", :relative_path => "first")
             @second = Puppet::FileServing::Metadata.new("/my", :relative_path => "second")
+            @first.stubs(:ftype).returns "directory"
+            @second.stubs(:ftype).returns "directory"
 
-            @property = stub 'property', :metadata= => nil
-            @resource = stub 'file', :[]= => nil, :property => @property
+            @parameter = stub 'property', :metadata= => nil
+            @resource = stub 'file', :[]= => nil, :parameter => @parameter
         end
 
         it "should pass its source to the :perform_recursion method" do
@@ -330,6 +366,14 @@ describe Puppet::Type.type(:file) do
             @file.recurse_remote({})
         end
 
+        it "should not recurse when the remote file is not a directory" do
+            data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => ".")
+            data.stubs(:ftype).returns "file"
+            @file.expects(:perform_recursion).with("puppet://foo/bar").returns [data]
+            @file.expects(:newchild).never
+            @file.recurse_remote({})
+        end
+
         it "should set the source of each returned file to the searched-for URI plus the found relative path" do
             @first.expects(:source=).with File.join("puppet://foo/bar", @first.relative_path)
             @file.expects(:perform_recursion).returns [@first]
@@ -368,9 +412,9 @@ describe Puppet::Type.type(:file) do
 
         it "should store the metadata in the source property for each resource so the source does not have to requery the metadata" do
             @file.stubs(:perform_recursion).returns [@first]
-            @resource.expects(:property).with(:source).returns @property
+            @resource.expects(:parameter).with(:source).returns @parameter
             
-            @property.expects(:metadata=).with(@first)
+            @parameter.expects(:metadata=).with(@first)
 
             @file.recurse_remote("first" => @resource)
         end
@@ -388,7 +432,7 @@ describe Puppet::Type.type(:file) do
             @first.stubs(:relative_path).returns "."
             @file.stubs(:perform_recursion).returns [@first]
 
-            @file.property(:source).expects(:metadata=).with @first
+            @file.parameter(:source).expects(:metadata=).with @first
             
             @file.recurse_remote("first" => @resource)
         end
@@ -403,7 +447,7 @@ describe Puppet::Type.type(:file) do
 
                 @resource.expects(:[]=).with(:ensure, :absent)
 
-                @file.expects(:newchild).returns stub('secondfile', :[]= => nil, :property => @property)
+                @file.expects(:newchild).returns stub('secondfile', :[]= => nil, :parameter => @parameter)
 
                 @file.recurse_remote("first" => @resource)
             end
@@ -582,8 +626,14 @@ describe Puppet::Type.type(:file) do
                 end
             end
 
+            it "should fail if it has no catalog" do
+                file = @file.class.create(:path => "/foo/bar", :owner => "root", :group => "wheel")
+                lambda { file.newchild("foo/bar").should raise_error(ArgumentError)
+            end
+
             it "should copy all of the parent resource's 'should' values that were set at initialization" do
                 file = @file.class.create(:path => "/foo/bar", :owner => "root", :group => "wheel")
+                @catalog.add_resource(file)
                 file.class.expects(:create).with { |options| options[:owner] == "root" and options[:group] == "wheel" }
                 file.newchild("my/path")
             end
@@ -596,13 +646,18 @@ describe Puppet::Type.type(:file) do
             it "should not copy values to the child which were set by the source" do
                 @file[:source] = "/foo/bar"
                 metadata = stub 'metadata', :owner => "root", :group => "root", :mode => 0755, :ftype => "file", :checksum => "{md5}whatever"
-                @file.property(:source).stubs(:metadata).returns metadata
+                @file.parameter(:source).stubs(:metadata).returns metadata
 
-                @file.property(:source).copy_source_values
+                @file.parameter(:source).copy_source_values
 
                 @file.class.expects(:create).with { |params| params[:group].nil? }
                 @file.newchild("my/path")
             end
+
+            it "should return nil if the specified child resource already exists in the catalog" do
+                @catalog.expects(:resource).with(:file, File.join(@file[:path], "foo/bar")).returns mock("resource")
+                @file.newchild("foo/bar").should be_nil
+            end
         end
     end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
index 2bb19eecc..2dcc1a557 100755
--- a/spec/unit/type/file/source.rb
+++ b/spec/unit/type/file/source.rb
@@ -9,22 +9,22 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         @resource = stub 'resource', :[]= => nil, :property => nil
     end
 
-    it "should be a subclass of Property" do
-        source.superclass.must == Puppet::Property
+    it "should be a subclass of Parameter" do
+        source.superclass.must == Puppet::Parameter
     end
 
     describe "when initializing" do
-        it "should fail if the 'should' values are not URLs" do
+        it "should fail if the set values are not URLs" do
             s = source.new(:resource => @resource)
             URI.expects(:parse).with('foo').raises RuntimeError
 
-            lambda { s.should = %w{foo} }.must raise_error(Puppet::Error)
+            lambda { s.value = %w{foo} }.must raise_error(Puppet::Error)
         end
 
         it "should fail if the URI is not a local file, file URI, or puppet URI" do
             s = source.new(:resource => @resource)
 
-            lambda { s.should = %w{http://foo/bar} }.must raise_error(Puppet::Error)
+            lambda { s.value = %w{http://foo/bar} }.must raise_error(Puppet::Error)
         end
     end
 
@@ -53,14 +53,14 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         end
 
         it "should collect its metadata using the Metadata class if it is not already set" do
-            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            @source = source.new(:resource => @resource, :value => "/foo/bar")
             Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns @metadata
             @source.metadata
         end
 
         it "should use the metadata from the first found source" do
             metadata = stub 'metadata', :source= => nil
-            @source = source.new(:resource => @resource, :should => ["/foo/bar", "/fee/booz"])
+            @source = source.new(:resource => @resource, :value => ["/foo/bar", "/fee/booz"])
             Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns nil
             Puppet::FileServing::Metadata.expects(:find).with("/fee/booz").returns metadata
             @source.metadata.should equal(metadata)
@@ -68,7 +68,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
         it "should store the found source as the metadata's source" do
             metadata = mock 'metadata'
-            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            @source = source.new(:resource => @resource, :value => "/foo/bar")
             Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns metadata
 
             metadata.expects(:source=).with("/foo/bar")
@@ -76,7 +76,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         end
 
         it "should fail intelligently if an exception is encountered while querying for metadata" do
-            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            @source = source.new(:resource => @resource, :value => "/foo/bar")
             Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").raises RuntimeError
 
             @source.expects(:fail).raises ArgumentError
@@ -84,7 +84,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         end
 
         it "should fail if no specified sources can be found" do
-            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            @source = source.new(:resource => @resource, :value => "/foo/bar")
             Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns nil
 
             @source.expects(:fail).raises RuntimeError
@@ -200,15 +200,6 @@ describe Puppet::Type.type(:file).attrclass(:source) do
         end
     end
 
-    describe "when retrieving the property state" do
-        it "should copy all metadata to the resource" do
-            @source = source.new(:resource => @resource)
-            @source.expects(:copy_source_values)
-
-            @source.retrieve
-        end
-    end
-
     describe "when flushing" do
         it "should set its metadata to nil" do
             @source = source.new(:resource => @resource)
@@ -224,86 +215,6 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             @source.instance_variable_get("@content").should be_nil
         end
     end
-    
-    describe "when testing whether the local file is in sync" do
-        before do
-            @source = source.new(:resource => @resource)
-        end
-
-        it "should be considered in sync if the remote file is a directory" do
-            metadata = mock 'data', :ftype => "directory"
-            @source.expects(:metadata).returns metadata
-
-            @source.must be_insync("some content")
-        end
-
-        it "should be considered in sync if the remote file is a symlink" do
-            metadata = mock 'data', :ftype => "link"
-            @source.expects(:metadata).returns metadata
-
-            @source.must be_insync("some content")
-        end
-
-        describe "and the remote file is a normal file" do
-            before do
-                @metadata = mock 'data', :ftype => "file"
-                @source.expects(:metadata).returns @metadata
-            end
-
-            it "should be not considered in sync if the file does not exist" do
-                @resource.expects(:stat).returns nil
-                @source.should_not be_insync("some content")
-            end
-
-            it "should be considered in sync if :replace is false and the file exists" do
-                @resource.expects(:stat).returns mock('stat')
-                @resource.expects(:replace?).returns false
-                @source.must be_insync("some content")
-            end
-
-            it "should be not considered in sync if :replace is false and the file does not exist" do
-                @resource.expects(:stat).returns nil
-                @resource.stubs(:replace?).returns false
-                @source.should_not be_insync("some content")
-            end
-
-            it "should not be considered in sync if the local file's contents are not the same as the remote file's contents"
-
-            it "should be considered in sync if the local file's content matches the remote file's contents"
-        end
-    end
-
-    def test_insync
-        source = tempfile()
-        dest = tempfile()
-        
-        file = Puppet::Type.type(:file).create :path => dest, :source => source, :title => "copier"
-        
-        property = file.property(:source)
-        assert(property, "did not get source property")
-        
-        # with a directory
-        Dir.mkdir(source)
-        currentvalues = file.retrieve
-        assert(property.insync?(currentvalues[property]), "source property not in sync with directory as source")
-        Dir.rmdir(source)
-        
-        # with a file
-        File.open(source, "w") { |f| f.puts "yay" }
-        currentvalues = file.retrieve
-        p currentvalues[property]
-        assert(!property.insync?(currentvalues[property]), "source property was in sync when file was missing")
-        
-        # With a different file
-        File.open(dest, "w") { |f| f.puts "foo" }
-        currentvalues = file.retrieve
-        assert(!property.insync?(currentvalues[property]), "source property was in sync with different file")
-        
-        # with matching files
-        File.open(dest, "w") { |f| f.puts "yay" }
-        currentvalues = file.retrieve
-        assert(property.insync?(currentvalues[property]), "source property was not in sync with matching file")
-    end
 
     it "should have a method for returning the content" do
         source.new(:resource => @resource).must respond_to(:content)
@@ -340,33 +251,4 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             lambda { @source.content }.should raise_error(RuntimeError)
         end
     end
-
-    describe "when changing the content" do
-        before do
-            @source = source.new(:resource => @resource)
-            @source.stubs(:content).returns "foobar"
-
-            @metadata = stub 'metadata', :checksum => 123
-            @source.metadata = @metadata
-            @resource.stubs(:[]).with(:path).returns "/boo"
-        end
-
-        it "should use the file's :write method to write the content" do
-            @resource.expects(:write).with("foobar", :source, 123)
-
-            @source.sync
-        end
-
-        it "should return :file_changed if the file already existed" do
-            @resource.stubs(:write)
-            FileTest.expects(:exist?).with("/boo").returns true
-            @source.sync.should == :file_changed
-        end
-
-        it "should return :file_created if the file already existed" do
-            @resource.stubs(:write)
-            FileTest.expects(:exist?).with("/boo").returns false
-            @source.sync.should == :file_created
-        end
-    end
 end
diff --git a/test/other/overrides.rb b/test/other/overrides.rb
index e0e3fdf5f..b38cb09e8 100755
--- a/test/other/overrides.rb
+++ b/test/other/overrides.rb
@@ -23,33 +23,27 @@ class TestOverrides < Test::Unit::TestCase
         basedir = File.join(tmpdir(), "overridetesting")
         mksubdirs(basedir, 1)
 
-        baseobj = nil
         basefile = File.join(basedir, "file")
-        assert_nothing_raised("Could not create base obj") {
-            baseobj = Puppet.type(:file).create(
-                :title => "base",
-                :path => basedir,
-                :recurse => true,
-                :mode => "755"
-            )
-        }
+        baseobj = Puppet.type(:file).create(
+            :title => "base",
+            :path => basedir,
+            :recurse => true,
+            :mode => "755"
+        )
 
-        subobj = nil
         subdir = File.join(basedir, "0")
         subfile = File.join(subdir, "file")
-        assert_nothing_raised("Could not create sub obj") {
-            subobj = Puppet.type(:file).create(
-                :title => "sub",
-                :path => subdir,
-                :recurse => true,
-                :mode => "644"
-            )
-        }
+        subobj = Puppet.type(:file).create(
+            :title => "sub",
+            :path => subdir,
+            :recurse => true,
+            :mode => "644"
+        )
 
         assert_apply(baseobj, subobj)
 
-        assert(File.stat(basefile).mode & 007777 == 0755)
-        assert(File.stat(subfile).mode & 007777 == 0644)
+        assert_equal(0755, File.stat(basefile).mode & 007777, "Did not set base mode")
+        assert_equal(0644, File.stat(subfile).mode & 007777, "Did not set overridden mode")
     end
 
     def test_deepoverride
diff --git a/test/ral/type/exec.rb b/test/ral/type/exec.rb
index 8c7c3f69b..5ccfc2475 100755
--- a/test/ral/type/exec.rb
+++ b/test/ral/type/exec.rb
@@ -442,7 +442,7 @@ class TestExec < Test::Unit::TestCase
 
         Puppet.type(:exec).checks.each do |check|
             klass = Puppet.type(:exec).paramclass(check)
-            next if klass.values.include? :false
+            next if klass.value_collection.values.include? :false
             assert_raise(Puppet::Error, "Check '%s' did not fail on false" % check) do
                 exec[check] = false
             end
diff --git a/test/ral/type/file.rb b/test/ral/type/file.rb
index c0c644dc0..348970fd7 100755
--- a/test/ral/type/file.rb
+++ b/test/ral/type/file.rb
@@ -639,46 +639,6 @@ class TestFile < Test::Unit::TestCase
         assert_equal(subobj, edge.target, "file did not require its parent dir")
     end
 
-    def test_content
-        file = tempfile()
-        str = "This is some content"
-
-        obj = nil
-        assert_nothing_raised {
-            obj = Puppet.type(:file).create(
-                :name => file,
-                :content => str
-            )
-        }
-
-        assert(!obj.insync?(obj.retrieve), "Object is incorrectly in sync")
-
-        assert_events([:file_created], obj)
-
-        currentvalues = obj.retrieve
-
-        assert(obj.insync?(currentvalues), "Object is not in sync")
-
-        text = File.read(file)
-
-        assert_equal(str, text, "Content did not copy correctly")
-
-        newstr = "Another string, yo"
-
-        obj[:content] = newstr
-
-        assert(!obj.insync?(obj.retrieve), "Object is incorrectly in sync")
-
-        assert_events([:file_changed], obj)
-
-        text = File.read(file)
-
-        assert_equal(newstr, text, "Content did not copy correctly")
-
-        currentvalues = obj.retrieve
-        assert(obj.insync?(currentvalues), "Object is not in sync")
-    end
-
     # Unfortunately, I know this fails
     def disabled_test_recursivemkdir
         path = tempfile()
@@ -946,6 +906,7 @@ class TestFile < Test::Unit::TestCase
         currentvalues = file.retrieve
         assert(file.insync?(currentvalues), "Symlink not considered 'present'")
         File.unlink(path)
+        file.flush
 
         # Now set some content, and make sure it works
         file[:content] = "yayness"
diff --git a/test/ral/type/file/target.rb b/test/ral/type/file/target.rb
index b6c84df99..4fd5894d7 100755
--- a/test/ral/type/file/target.rb
+++ b/test/ral/type/file/target.rb
@@ -42,54 +42,6 @@ class TestFileTarget < Test::Unit::TestCase
         assert_events([], file)
         assert_events([], file)
     end
-    
-    def test_linkrecurse
-        dest = tempfile()
-        link = @file.create :path => tempfile(), :recurse => true, :ensure => dest
-        catalog = mk_catalog(link)
-        
-        ret = nil
-        
-        # Start with nothing, just to make sure we get nothing back
-        assert_nothing_raised { ret = link.linkrecurse(true) }
-        assert_nil(ret, "got a return when the dest doesn't exist")
-        
-        # then with a directory with only one file
-        Dir.mkdir(dest)
-        one = File.join(dest, "one")
-        File.open(one, "w") { |f| f.puts "" }
-        link[:ensure] = dest
-        assert_nothing_raised { ret = link.linkrecurse(true) }
-        
-        assert_equal(:directory, link.should(:ensure), "ensure was not set to directory")
-        assert_equal([File.join(link.title, "one")], ret.collect { |f| f.title },
-            "Did not get linked file")
-        oneobj = catalog.resource(:file, File.join(link.title, "one"))
-        assert_equal(one, oneobj.should(:target), "target was not set correctly")
-        
-        oneobj.remove
-        File.unlink(one)
-        
-        # Then make sure we get multiple files
-        returns = []
-        5.times do |i|
-            path = File.join(dest, i.to_s)
-            returns << File.join(link.title, i.to_s)
-            File.open(path, "w") { |f| f.puts "" }
-        end
-        assert_nothing_raised { ret = link.linkrecurse(true) }
-
-        assert_equal(returns.sort, ret.collect { |f| f.title }.sort,
-            "Did not get links back")
-        
-        returns.each do |path|
-            obj = catalog.resource(:file, path)
-            assert(path, "did not get obj for %s" % path)
-            sdest = File.join(dest, File.basename(path))
-            assert_equal(sdest, obj.should(:target),
-                "target was not set correctly for %s" % path)
-        end
-    end
 
     def test_simplerecursivelinking
         source = tempfile()
@@ -116,7 +68,8 @@ class TestFileTarget < Test::Unit::TestCase
         assert(File.symlink?(linkpath), "path is not a link")
         assert_equal(file, File.readlink(linkpath))
 
-        assert_nil(catalog.resource(:file, sublink), "objects were not removed")
+        # Use classes for comparison, because the resource inspection is so large
+        assert_equal(NilClass, catalog.resource(:file, sublink).class, "dynamically generated resources were not removed")
         assert_equal([], link.evaluate, "Link is not in sync")
     end
 
@@ -195,20 +148,20 @@ class TestFileTarget < Test::Unit::TestCase
         source = tempfile()
         dest = tempfile()
 
-        objects = []
-        objects << Puppet.type(:exec).create(
+        resources = []
+        resources << Puppet.type(:exec).create(
             :command => "mkdir %s; touch %s/file" % [source, source],
             :title => "yay",
             :path => ENV["PATH"]
         )
-        objects << Puppet.type(:file).create(
+        resources << Puppet.type(:file).create(
             :ensure => source,
             :path => dest,
             :recurse => true,
-            :require => objects[0]
+            :require => resources[0]
         )
 
-        assert_apply(*objects)
+        assert_apply(*resources)
 
         link = File.join(dest, "file")
         assert(FileTest.symlink?(link), "Did not make link")
diff --git a/test/ral/type/filesources.rb b/test/ral/type/filesources.rb
index 9dd1d988e..2479eb491 100755
--- a/test/ral/type/filesources.rb
+++ b/test/ral/type/filesources.rb
@@ -53,94 +53,6 @@ class TestFileSources < Test::Unit::TestCase
         destfile = File.join(dest, "file")
         return source, dest, sourcefile, destfile
     end
-    
-    def test_source_retrieve
-        source = tempfile()
-        dest = tempfile()
-        
-        file = Puppet::Type.newfile :path => dest, :source => source,
-            :title => "copier"
-        
-        assert(file.property(:checksum), "source property did not create checksum property")
-        property = file.property(:source)
-        assert(property, "did not get source property")
-        
-        # Make sure the munge didn't actually change the source
-        assert_equal([source], property.should, "munging changed the source")
-        
-        currentvalue = nil
-        # Now make the dest a directory, and make sure the object sets :ensure
-        # up to create a directory
-        Dir.mkdir(source)
-        assert_nothing_raised do
-            currentvalue = property.retrieve
-        end
-        assert_equal(:directory, file.should(:ensure),
-            "Did not set to create directory")
-        
-        # And make sure the source property won't try to do anything with a
-        # remote dir
-        assert(property.insync?(currentvalue), "Source was out of sync even tho remote is dir")
-        
-        # Now remove the source, and make sure :ensure was not modified
-        Dir.rmdir(source)
-        assert_equal(:directory, file.should(:ensure),
-            "Did not keep :ensure setting")
-        
-        # Now have a remote file and make sure things work correctly
-        File.open(source, "w") { |f| f.puts "yay" }
-        File.chmod(0755, source)
-        
-        assert_nothing_raised do
-            property.retrieve
-        end
-        assert_equal(:file, file.should(:ensure),
-            "Did not make correct :ensure setting")
-        assert_equal(0755, file.should(:mode),
-            "Mode was not copied over")
-        
-        # Now let's make sure that we get the first found source
-        fake = tempfile()
-        property.should = [fake, source]
-        assert_nothing_raised do
-            property.retrieve
-        end
-        assert_equal(Digest::MD5.hexdigest(File.read(source)), property.checksum.sub(/^\{\w+\}/, ''), 
-            "Did not catch later source")
-    end
-    
-    def test_source_sync
-        source = tempfile()
-        dest = tempfile()
-
-        file = Puppet::Type.newfile :path => dest, :source => source,
-            :title => "copier"
-        property = file.property(:source)
-        
-        File.open(source, "w") { |f| f.puts "yay" }
-        
-        currentvalues = file.retrieve
-        assert(! property.insync?(currentvalues[property]), "source thinks it's in sync")
-        
-        event = nil
-        assert_nothing_raised do
-            event = property.sync
-        end
-        assert_equal(:file_created, event)
-        assert_equal(File.read(source), File.read(dest),
-            "File was not copied correctly")
-        
-        # Now write something different
-        File.open(source, "w") { |f| f.puts "rah" }
-        currentvalues = file.retrieve
-        assert(! property.insync?(currentvalues[property]), "source should be out of sync")
-        assert_nothing_raised do
-            event = property.sync
-        end
-        assert_equal(:file_changed, event)
-        assert_equal(File.read(source), File.read(dest),
-            "File was not copied correctly")
-    end
 
     def recursive_source_test(fromdir, todir)
         initstorage
@@ -259,7 +171,7 @@ class TestFileSources < Test::Unit::TestCase
         FileUtils.cd(todir) {
             %x{find . 2>/dev/null}.chomp.split(/\n/).each { |file|
                 if file =~ /file[0-9]+/
-                    File.unlink(file)
+                    FileUtils.rm_rf(file)
                 end
             }
         }