10 files changed, 285 insertions, 247 deletions

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index dd28b5588..9da761a8c 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -8,6 +8,8 @@ class Puppet::Transaction
     require 'puppet/transaction/change'
     require 'puppet/transaction/event'
     require 'puppet/transaction/event_manager'
+    require 'puppet/transaction/resource_harness'
+    require 'puppet/resource/status'
 
     attr_accessor :component, :catalog, :ignoreschedules
     attr_accessor :sorted_resources, :configurator
@@ -21,6 +23,9 @@ class Puppet::Transaction
     # Routes and stores any events and subscriptions.
     attr_reader :event_manager
 
+    # Handles most of the actual interacting with resources
+    attr_reader :resource_harness
+
     include Puppet::Util
     include Puppet::Util::Tagging
 
@@ -60,38 +65,13 @@ class Puppet::Transaction
 
     # Apply all changes for a resource
     def apply(resource)
-        begin
-            changes = resource.evaluate
-        rescue => detail
-            puts detail.backtrace if Puppet[:trace]
-
-            resource.err "Failed to retrieve current state of resource: %s" % detail
-
-            # Mark that it failed
-            @failures[resource] += 1
-
-            # And then return
-            return
-        end
-
-        changes = [changes] unless changes.is_a?(Array)
-
-        @resourcemetrics[:out_of_sync] += 1 if changes.length > 0
-
-        return if changes.empty? or ! allow_processing?(resource, changes)
-
-        apply_changes(resource, changes)
-
-        # If there were changes and the resource isn't in noop mode...
-        unless changes.empty? or resource.noop
-            # Record when we last synced
-            resource.cache(:synced, Time.now)
-
-            # Flush, if appropriate
-            if resource.respond_to?(:flush)
-                resource.flush
-            end
+        status = resource_harness.evaluate(resource)
+        add_resource_status(status)
+        status.events.each do |event|
+            event_manager.queue_event(resource, event)
         end
+    rescue => detail
+        resource.err "Could not evaluate: #{detail}"
     end
 
     # Apply each change in turn.
@@ -138,13 +118,6 @@ class Puppet::Transaction
         end
     end
 
-    # Are we deleting this resource?
-    def deleting?(changes)
-        changes.detect { |change|
-            change.property.name == :ensure and change.should == :absent
-        }
-    end
-
     # See if the resource generates new resources at evaluation time.
     def eval_generate(resource)
         generate_additional_resources(resource, :eval_generate)
@@ -237,13 +210,8 @@ class Puppet::Transaction
         event_manager.events
     end
 
-    # Determine whether a given resource has failed.
-    def failed?(obj)
-        if @failures[obj] > 0
-            return @failures[obj]
-        else
-            return false
-        end
+    def failed?(resource)
+        s = resource_status(resource) and s.failed?
     end
 
     # Does this resource have any failed dependencies?
@@ -252,17 +220,14 @@ class Puppet::Transaction
         # we check for failures in any of the vertexes above us.  It's not
         # enough to check the immediate dependencies, which is why we use
         # a tree from the reversed graph.
-        skip = false
-        deps = relationship_graph.dependencies(resource)
-        deps.each do |dep|
-            if fails = failed?(dep)
-                resource.notice "Dependency %s[%s] has %s failures" %
-                    [dep.class.name, dep.name, @failures[dep]]
-                skip = true
-            end
+        found_failed = false
+        relationship_graph.dependencies(resource).each do |dep|
+            next unless failed?(dep)
+            resource.notice "Dependency #{dep} has failures: #{resource_status(dep).failed}"
+            found_failed = true
         end
 
-        return skip
+        return found_failed
     end
 
     # A general method for recursively generating new resources from a
@@ -363,6 +328,10 @@ class Puppet::Transaction
         @report = Report.new
 
         @event_manager = Puppet::Transaction::EventManager.new(self)
+
+        @resource_harness = Puppet::Transaction::ResourceHarness.new(self)
+
+        @resource_status = {}
     end
 
     # Prefetch any providers that support it.  We don't support prefetching
@@ -407,6 +376,36 @@ class Puppet::Transaction
         catalog.relationship_graph
     end
 
+    # Send off the transaction report.
+    def send_report
+        begin
+            report = generate_report()
+        rescue => detail
+            Puppet.err "Could not generate report: %s" % detail
+            return
+        end
+
+        if Puppet[:summarize]
+            puts report.summary
+        end
+
+        if Puppet[:report]
+            begin
+                report.save()
+            rescue => detail
+                Puppet.err "Reporting failed: %s" % detail
+            end
+        end
+    end
+
+    def add_resource_status(status)
+        @resource_status[status.resource] = status
+    end
+
+    def resource_status(resource)
+        @resource_status[resource.to_s]
+    end
+
     # Roll all completed changes back.
     def rollback
         @changes.reverse.collect do |change|
@@ -480,21 +479,6 @@ class Puppet::Transaction
     def appropriately_tagged?(resource)
         self.ignore_tags? or tags.empty? or resource.tagged?(*tags)
     end
-
-    private
-
-    def apply_change(resource, change)
-        @changes << change
-
-        event = change.forward
-
-        if event.status == "success"
-            @resourcemetrics[:applied] += 1
-        else
-            @failures[resource] += 1
-        end
-        event_manager.queue_event(resource, event)
-    end
 end
 
 require 'puppet/transaction/report'
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index da859910c..669b0ae98 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -26,14 +26,16 @@ class Puppet::Transaction::ResourceHarness
     def changes_to_perform(status, resource)
         current = resource.retrieve
 
+        resource.cache :checked, Time.now
+
         if param = resource.parameter(:ensure)
-            insync = param.insync?(current[:ensure])
-            return [Puppet::Transaction::Change.new(param, current[:ensure])] unless insync
-            return [] if param.should == :absent
+            return [] if absent_and_not_being_created?(current, param)
+            return [Puppet::Transaction::Change.new(param, current[:ensure])] unless ensure_is_insync?(current, param)
+            return [] if ensure_should_be_absent?(current, param)
         end
 
         resource.properties.reject { |p| p.name == :ensure }.find_all do |param|
-            ! param.insync?(current[param.name])
+            param_is_not_insync?(current, param)
         end.collect do |param|
             Puppet::Transaction::Change.new(param, current[param.name])
         end
@@ -51,6 +53,7 @@ class Puppet::Transaction::ResourceHarness
         return status
     rescue => detail
         resource.fail "Could not create resource status: #{detail}" unless status
+        puts detail.backtrace if Puppet[:trace]
         resource.err "Could not evaluate: #{detail}"
         status.failed = true
         return status
@@ -59,4 +62,22 @@ class Puppet::Transaction::ResourceHarness
     def initialize(transaction)
         @transaction = transaction
     end
+
+    private
+
+    def absent_and_not_being_created?(current, param)
+        current[:ensure] == :absent and param.should.nil?
+    end
+
+    def ensure_is_insync?(current, param)
+        param.insync?(current[:ensure])
+    end
+
+    def ensure_should_be_absent?(current, param)
+        param.should == :absent
+    end
+
+    def param_is_not_insync?(current, param)
+        ! param.insync?(current[param.name] || :absent)
+    end
 end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index d17a56d67..1df84f2df 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -620,31 +620,12 @@ class Type
         catalog.version
     end
 
-    # Meta-parameter methods:  These methods deal with the results
-    # of specifying metaparameters
-
-    private
-
     # Return all of the property objects, in the order specified in the
     # class.
     def properties
-        #debug "%s has %s properties" % [self,@parameters.length]
-        props = self.class.properties.collect { |prop|
-            @parameters[prop.name]
-        }.find_all { |p|
-            ! p.nil?
-        }.each do |prop|
-            unless prop.is_a?(Puppet::Property)
-                raise Puppet::DevError, "got a non-property %s(%s)" %
-                    [prop.class, prop.class.name]
-            end
-        end
-
-        props
+        self.class.properties.collect { |prop| @parameters[prop.name] }.compact
     end
 
-    public
-
     # Is this type's name isomorphic with the object?  That is, if the
     # name conflicts, does it necessarily mean that the objects conflict?
     # Defaults to true.
@@ -722,43 +703,6 @@ class Type
     ###############################
     # Code related to evaluating the resources.
 
-    # This method is responsible for collecting property changes we always
-    # descend into the children before we evaluate our current properties.
-    # This returns any changes resulting from testing, thus 'collect' rather
-    # than 'each'.
-    def evaluate
-        if self.provider.is_a?(Puppet::Provider)
-            unless provider.class.suitable?
-                raise Puppet::Error, "Provider %s is not functional on this platform" % provider.class.name
-            end
-        end
-
-        # this only operates on properties, not properties + children
-        # it's important that we call retrieve() on the type instance,
-        # not directly on the property, because it allows the type to override
-        # the method, like pfile does
-        currentvalues = self.retrieve
-
-        changes = propertychanges(currentvalues).flatten
-
-        # now record how many changes we've resulted in
-        if changes.length > 0
-            self.debug "%s change(s)" %
-                [changes.length]
-        end
-
-        # If we're in noop mode, we don't want to store the checked time,
-        # because it will result in the resource not getting scheduled if
-        # someone were to apply the catalog in non-noop mode.
-        # We're going to go ahead and record that we checked if there were
-        # no changes, since it's unlikely it will affect the scheduling.
-        noop = noop?
-        if ! noop or (noop && changes.length == 0)
-            self.cache(:checked, Time.now)
-        end
-        return changes.flatten
-    end
-
     # Flush the provider, if it supports it.  This is called by the
     # transaction.
     def flush
@@ -808,7 +752,31 @@ class Type
 
     # retrieve the current value of all contained properties
     def retrieve
-        return currentpropvalues
+        if self.provider.is_a?(Puppet::Provider) and ! provider.class.suitable?
+            fail "Provider #{provider.class.name} is not functional on this host"
+        end
+
+        result = Puppet::Resource.new(type, title)
+
+        # Provide the name, so we know we'll always refer to a real thing
+        result[:name] = self[:name] unless self[:name] == title
+
+        if ensure_prop = property(:ensure) or (self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure))
+            result[:ensure] = ensure_state = ensure_prop.retrieve
+        else
+            ensure_state = nil
+        end
+
+        properties.each do |property|
+            next if property.name == :ensure
+            if ensure_state == :absent
+                result[property] = :absent
+            else
+                result[property] = property.retrieve
+            end
+        end
+
+        result
     end
 
     # Get a hash of the current properties.  Returns a hash with
@@ -848,42 +816,6 @@ class Type
         noop?
     end
 
-    # Retrieve the changes associated with all of the properties.
-    def propertychanges(currentvalues)
-        # If we are changing the existence of the object, then none of
-        # the other properties matter.
-        changes = []
-        ensureparam = @parameters[:ensure]
-
-        # This allows resource types to have 'ensure' be a parameter, which allows them to
-        # just pass the parameter on to other generated resources.
-        ensureparam = nil unless ensureparam.is_a?(Puppet::Property)
-        if ensureparam && !currentvalues.include?(ensureparam)
-            raise Puppet::DevError, "Parameter ensure defined but missing from current values"
-        end
-
-        if ensureparam and ! ensureparam.insync?(currentvalues[ensureparam])
-            changes << Puppet::Transaction::Change.new(ensureparam, currentvalues[ensureparam])
-        # Else, if the 'ensure' property is correctly absent, then do
-        # nothing
-        elsif ensureparam and currentvalues[ensureparam] == :absent
-            return []
-        else
-            changes = properties().find_all { |property|
-                currentvalues[property] ||= :absent
-                ! property.insync?(currentvalues[property])
-            }.collect { |property|
-                Puppet::Transaction::Change.new(property, currentvalues[property])
-            }
-        end
-
-        if Puppet[:debug] and changes.length > 0
-            self.debug("Changing " + changes.collect { |ch| ch.property.name }.join(","))
-        end
-
-        changes
-    end
-
     ###############################
     # Code related to managing resource instances.
     require 'puppet/transportable'
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index d995add0d..895c0b207 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -404,6 +404,16 @@ Puppet::Type.newtype(:file) do
 
         super
 
+        # If they've specified a source, we get our 'should' values
+        # from it.
+        unless self[:ensure]
+            if self[:target]
+                self[:ensure] = :symlink
+            elsif self[:content]
+                self[:ensure] = :file
+            end
+        end
+
         @title = self.class.canonicalize_ref(@title)
         @stat = nil
     end
@@ -630,7 +640,6 @@ Puppet::Type.newtype(:file) do
         expire
     end
 
-    # a wrapper method to make sure the file exists before doing anything
     def retrieve
         if source = parameter(:source)
             source.copy_source_values
@@ -784,24 +793,6 @@ Puppet::Type.newtype(:file) do
         self.fail "File written to disk did not match checksum; discarding changes (%s vs %s)" % [checksum, newsum]
     end
 
-    # Override the parent method, because we don't want to generate changes
-    # when the file is missing and there is no 'ensure' state.
-    def propertychanges(currentvalues)
-        unless self.stat
-            found = false
-            ([:ensure] + CREATORS).each do |prop|
-                if @parameters.include?(prop)
-                    found = true
-                    break
-                end
-            end
-            unless found
-                return []
-            end
-        end
-        super
-    end
-
     # There are some cases where all of the work does not get done on
     # file creation/modification, so we have to do some extra checking.
     def property_fix
diff --git a/spec/integration/transaction.rb b/spec/integration/transaction.rb
index 1b8b11d48..a387bf7c9 100755
--- a/spec/integration/transaction.rb
+++ b/spec/integration/transaction.rb
@@ -19,9 +19,9 @@ describe Puppet::Transaction do
 
         transaction = Puppet::Transaction.new(catalog)
 
-        resource.expects(:evaluate).raises "this is a failure"
+        resource.expects(:retrieve).raises "this is a failure"
 
-        child_resource.expects(:evaluate).never
+        child_resource.expects(:retrieve).never
 
         transaction.evaluate
     end
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index fe6e2ddc6..d1e46a206 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -7,7 +7,24 @@ require 'puppet_spec/files'
 describe Puppet::Type.type(:file) do
     include PuppetSpec::Files
 
+    it "should not attempt to manage files that do not exist if no means of creating the file is specified" do
+        file = Puppet::Type.type(:file).new :path => "/my/file", :mode => "755"
+        catalog = Puppet::Resource::Catalog.new
+        catalog.add_resource file
+
+        file.parameter(:mode).expects(:retrieve).never
+
+        transaction = Puppet::Transaction.new(catalog)
+        transaction.resource_harness.evaluate(file).should_not be_failed
+    end
+
     describe "when writing files" do
+        before do
+            Puppet::Util::Log.newdestination :console
+        end
+
+        after { Puppet::Util::Log.close :console }
+
         it "should backup files to a filebucket when one is configured" do
             bucket = Puppet::Type.type(:filebucket).new :path => tmpfile("filebucket"), :name => "mybucket"
             file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => "mybucket", :content => "foo"
@@ -49,6 +66,8 @@ describe Puppet::Type.type(:file) do
 
             File.chmod(0111, dir) # make it non-writeable
 
+            Puppet::Util::Log.stubs(:newmessage)
+
             catalog.apply
 
             File.read(file[:path]).should == "bar\n"
@@ -75,7 +94,7 @@ describe Puppet::Type.type(:file) do
         end
 
         it "should backup directories to the local filesystem by copying the whole directory" do
-            file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => ".bak", :content => "foo"
+            file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => ".bak", :content => "foo", :force => true
             catalog = Puppet::Resource::Catalog.new
             catalog.add_resource file
 
@@ -92,7 +111,7 @@ describe Puppet::Type.type(:file) do
 
         it "should backup directories to filebuckets by backing up each file separately" do
             bucket = Puppet::Type.type(:filebucket).new :path => tmpfile("filebucket"), :name => "mybucket"
-            file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => "mybucket", :content => "foo"
+            file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => "mybucket", :content => "foo", :force => true
             catalog = Puppet::Resource::Catalog.new
             catalog.add_resource file, bucket
 
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index a4d6b1e3a..fba2518a5 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -5,18 +5,56 @@ require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/transaction'
 
 describe Puppet::Transaction do
+    before do
+        @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
+    end
+
     it "should delegate its event list to the event manager" do
         @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
         @transaction.event_manager.expects(:events).returns %w{my events}
         @transaction.events.should == %w{my events}
     end
 
+    it "should be able to accept resource status instances" do
+        resource = Puppet::Type.type(:notify).new :title => "foobar"
+        status = Puppet::Resource::Status.new(resource)
+        @transaction.add_resource_status(status)
+        @transaction.resource_status(resource).should equal(status)
+    end
+
+    it "should be able to look resource status up by resource reference" do
+        resource = Puppet::Type.type(:notify).new :title => "foobar"
+        status = Puppet::Resource::Status.new(resource)
+        @transaction.add_resource_status(status)
+        @transaction.resource_status(resource.to_s).should equal(status)
+    end
+
+    it "should return nil when asked for a status that has not been created" do
+        @transaction.resource_status("File[/foo]").should be_nil
+    end
+
+    it "should consider a resource to be failed if a status instance exists for that resource and indicates it is failed" do
+        resource = Puppet::Type.type(:notify).new :name => "yayness"
+        status = Puppet::Resource::Status.new(resource)
+        status.failed = "some message"
+        @transaction.add_resource_status(status)
+        @transaction.should be_failed(resource)
+    end
+
+   it "should consider a resource to have failed dependencies if any of its dependencies are failed"
+
     describe "when initializing" do
         it "should create an event manager" do
             @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
             @transaction.event_manager.should be_instance_of(Puppet::Transaction::EventManager)
             @transaction.event_manager.transaction.should equal(@transaction)
         end
+
+        it "should create a resource harness" do
+            @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
+            @transaction.resource_harness.should be_instance_of(Puppet::Transaction::ResourceHarness)
+            @transaction.resource_harness.transaction.should equal(@transaction)
+        end
     end
 
     describe "when evaluating a resource" do
@@ -58,61 +96,38 @@ describe Puppet::Transaction do
         end
     end
 
-    describe "when applying changes" do
+    describe "when applying a resource" do
         before do
+            @resource = Puppet::Type.type(:file).new :path => "/my/file"
+            @status = Puppet::Resource::Status.new(@resource)
+
             @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
             @transaction.event_manager.stubs(:queue_event)
-
-            @resource = stub 'resource'
-            @property = stub 'property', :is_to_s => "is", :should_to_s => "should"
-
-            @event = stub 'event', :status => "success"
-            @change = stub 'change', :property => @property, :changed= => nil, :forward => @event, :is => "is", :should => "should"
+            @transaction.resource_harness.stubs(:evaluate).returns(@status)
         end
 
-        it "should apply each change" do
-            c1 = stub 'c1', :property => @property, :changed= => nil
-            c1.expects(:forward).returns @event
-            c2 = stub 'c2', :property => @property, :changed= => nil
-            c2.expects(:forward).returns @event
-
-            @transaction.apply_changes(@resource, [c1, c2])
+        it "should use its resource harness to apply the resource" do
+            @transaction.resource_harness.expects(:evaluate).with(@resource)
+            @transaction.apply(@resource)
         end
 
-        it "should queue the events from each change" do
-            c1 = stub 'c1', :forward => stub("event1", :status => "success"), :property => @property, :changed= => nil
-            c2 = stub 'c2', :forward => stub("event2", :status => "success"), :property => @property, :changed= => nil
-
-            @transaction.event_manager.expects(:queue_event).with(@resource, c1.forward)
-            @transaction.event_manager.expects(:queue_event).with(@resource, c2.forward)
-
-            @transaction.apply_changes(@resource, [c1, c2])
+        it "should add the resulting resource status to its status list" do
+            @transaction.apply(@resource)
+            @transaction.resource_status(@resource).should be_instance_of(Puppet::Resource::Status)
         end
 
-        it "should store the change in the transaction's change list" do
-            @transaction.apply_changes(@resource, [@change])
-            @transaction.changes.should include(@change)
+        it "should queue any events added to the resource status" do
+            @status.expects(:events).returns %w{a b}
+            @transaction.event_manager.expects(:queue_event).with(@resource, "a")
+            @transaction.event_manager.expects(:queue_event).with(@resource, "b")
+            @transaction.apply(@resource)
         end
 
-        it "should increment the number of applied resources" do
-            @transaction.apply_changes(@resource, [@change])
-            @transaction.resourcemetrics[:applied].should == 1
-        end
-
-        describe "and a change fails" do
-            before do
-                @event.stubs(:status).returns "failure"
-            end
-
-            it "should increment the failures" do
-                @transaction.apply_changes(@resource, [@change])
-                @transaction.should be_any_failed
-            end
-
-            it "should queue the event" do
-                @transaction.event_manager.expects(:queue_event).with(@resource, @event)
-                @transaction.apply_changes(@resource, [@change])
-            end
+        it "should log and skip any resources that cannot be applied" do
+            @transaction.resource_harness.expects(:evaluate).raises ArgumentError
+            @resource.expects(:err)
+            @transaction.apply(@resource)
+            @transaction.resource_status(@resource).should be_nil
         end
     end
 
diff --git a/spec/unit/transaction/resource_harness.rb b/spec/unit/transaction/resource_harness.rb
index 9d741260a..d6a36db86 100755
--- a/spec/unit/transaction/resource_harness.rb
+++ b/spec/unit/transaction/resource_harness.rb
@@ -98,6 +98,11 @@ describe Puppet::Transaction::ResourceHarness do
             @harness.changes_to_perform(@status, @resource)
         end
 
+        it "should cache that the resource was checked" do
+            @resource.expects(:cache).with { |name, time| name == :checked and time.is_a?(Time) }
+            @harness.changes_to_perform(@status, @resource)
+        end
+
         it "should create changes with the appropriate property and current value" do
             @resource[:ensure] = :present
             @current_state[:ensure] = :absent
@@ -115,14 +120,16 @@ describe Puppet::Transaction::ResourceHarness do
                 @current_state[:ensure] = :absent
                 @current_state[:mode] = :absent
 
+                @resource.stubs(:retrieve).returns @current_state
+
                 changes = @harness.changes_to_perform(@status, @resource)
                 changes.length.should == 1
                 changes[0].property.name.should == :ensure
             end
         end
 
-        describe "and the 'ensure' parameter is present, should be set to 'absent', and is correctly set to 'absent'" do
-            it "should return an empty array" do
+        describe "and the 'ensure' parameter should be set to 'absent', and is correctly set to 'absent'" do
+            it "should return no changes" do
                 @resource[:ensure] = :absent
                 @resource[:mode] = "755"
                 @current_state[:ensure] = :absent
@@ -132,6 +139,17 @@ describe Puppet::Transaction::ResourceHarness do
             end
         end
 
+        describe "and the 'ensure' parameter is 'absent' and there is no 'desired value'" do
+            it "should return no changes" do
+                @resource.newattr(:ensure)
+                @resource[:mode] = "755"
+                @current_state[:ensure] = :absent
+                @current_state[:mode] = :absent
+
+                @harness.changes_to_perform(@status, @resource).should == []
+            end
+        end
+
         describe "and non-'ensure' parameters are not in sync" do
             it "should return a change for each parameter that is not in sync" do
                 @resource[:ensure] = :present
diff --git a/spec/unit/type.rb b/spec/unit/type.rb
index 9e6469c0f..f5953aa9c 100755
--- a/spec/unit/type.rb
+++ b/spec/unit/type.rb
@@ -34,6 +34,15 @@ describe Puppet::Type do
         resource.parameter(:fstype).must be_instance_of(Puppet::Type.type(:mount).attrclass(:fstype))
     end
 
+    it "should be able to retrieve all set properties" do
+        resource = Puppet::Type.type(:mount).new(:name => "foo", :fstype => "bar", :pass => 1, :ensure => :present)
+        props = resource.properties
+        props.should_not be_include(nil)
+        [:fstype, :ensure, :pass].each do |name|
+            props.should be_include(resource.parameter(name))
+        end
+    end
+
     it "should have a method for setting default values for resources" do
         Puppet::Type.type(:mount).new(:name => "foo").should respond_to(:set_default)
     end
@@ -350,32 +359,49 @@ describe Puppet::Type do
     end
 
     describe "when retrieving current property values" do
-        # Use 'mount' as an example, because it doesn't override 'retrieve'
         before do
             @resource = Puppet::Type.type(:mount).new(:name => "foo", :fstype => "bar", :pass => 1, :ensure => :present)
-            @properties = {}
+            @resource.property(:ensure).stubs(:retrieve).returns :absent
+        end
+
+        it "should fail if its provider is unsuitable" do
+            @resource = Puppet::Type.type(:mount).new(:name => "foo", :fstype => "bar", :pass => 1, :ensure => :present)
+            @resource.provider.class.expects(:suitable?).returns false
+            lambda { @resource.retrieve }.should raise_error(Puppet::Error)
         end
 
-        it "should return a hash containing values for all set properties" do
+        it "should return a Puppet::Resource instance with its type and title set appropriately" do
+            result = @resource.retrieve
+            result.should be_instance_of(Puppet::Resource)
+            result.type.should == "Mount"
+            result.title.should == "foo"
+        end
+
+        it "should set the name of the returned resource if its own name and title differ" do
+            @resource[:name] = "my name"
+            @resource.title = "other name"
+            @resource.retrieve[:name].should == "my name"
+        end
+
+        it "should provide a value for all set properties" do
             values = @resource.retrieve
-            [@resource.property(:fstype), @resource.property(:pass)].each { |property| values.should be_include(property) }
+            [:ensure, :fstype, :pass].each { |property| values[property].should_not be_nil }
         end
 
-        it "should not call retrieve on non-ensure properties if the resource is absent" do
-            @resource.property(:ensure).expects(:retrieve).returns :absent
-            @resource.property(:fstype).expects(:retrieve).never
-            @resource.retrieve[@resource.property(:fstype)]
+        it "should provide a value for 'ensure' even if no desired value is provided" do
+            @resource = Puppet::Type.type(:file).new(:path => "/my/file/that/can't/exist")
         end
 
-        it "should set all values to :absent if the resource is absent" do
+        it "should not call retrieve on non-ensure properties if the resource is absent and should consider the property absent" do
             @resource.property(:ensure).expects(:retrieve).returns :absent
-            @resource.retrieve[@resource.property(:fstype)].should == :absent
+            @resource.property(:fstype).expects(:retrieve).never
+            @resource.retrieve[:fstype].should == :absent
         end
 
         it "should include the result of retrieving each property's current value if the resource is present" do
             @resource.property(:ensure).expects(:retrieve).returns :present
             @resource.property(:fstype).expects(:retrieve).returns 15
-            @resource.retrieve[@resource.property(:fstype)].should == 15
+            @resource.retrieve[:fstype] == 15
         end
     end
 
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 206a50ed9..afb050a1f 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -125,6 +125,19 @@ describe Puppet::Type.type(:file) do
         file.autorequire.should be_empty
     end
 
+
+    describe "when initializing" do
+        it "should set a desired 'ensure' value if none is set and 'content' is set" do
+            file = Puppet::Type::File.new(:name => "/my/file", :content => "/foo/bar")
+            file[:ensure].should == :file
+        end
+
+        it "should set a desired 'ensure' value if none is set and 'target' is set" do
+            file = Puppet::Type::File.new(:name => "/my/file", :target => "/foo/bar")
+            file[:ensure].should == :symlink
+        end
+    end
+
     describe "when validating attributes" do
         %w{path backup recurse recurselimit source replace force ignore links purge sourceselect}.each do |attr|
             it "should have a '#{attr}' parameter" do
@@ -807,4 +820,23 @@ describe Puppet::Type.type(:file) do
             file.finish
         end
     end
+
+    describe "when writing the file" do
+        it "should propagate failures encountered when renaming the temporary file" do
+            File.stubs(:open)
+
+            File.expects(:rename).raises ArgumentError
+            file = Puppet::Type::File.new(:name => "/my/file", :backup => "puppet")
+
+            lambda { file.write("something", :content) }.should raise_error(Puppet::Error)
+        end
+    end
+
+    describe "when retrieving the current file state" do
+        it "should copy the source values if the 'source' parameter is set" do
+            file = Puppet::Type::File.new(:name => "/my/file", :source => "/foo/bar")
+            file.parameter(:source).expects(:copy_source_values)
+            file.retrieve
+        end
+    end
 end