Merge branch 'maint/2.6.next/5408' into 2.6.next

* maint/2.6.next/5408:
  Modified the behavior of Puppet::Resource::Status as follows:
  (#5408) Reworked ResourceHarness so that code is clearer and all behaviors are tested

8 files changed, 315 insertions, 680 deletions

diff --git a/lib/puppet/resource/status.rb b/lib/puppet/resource/status.rb
index 2bdfbbfef..f34edc469 100644
--- a/lib/puppet/resource/status.rb
+++ b/lib/puppet/resource/status.rb
@@ -4,13 +4,13 @@ module Puppet
       include Puppet::Util::Tagging
       include Puppet::Util::Logging
 
-      ATTRIBUTES = [:resource, :node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time, :change_count]
-      attr_accessor *ATTRIBUTES
+      attr_accessor :resource, :node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time
 
       STATES = [:skipped, :failed, :failed_to_restart, :restarted, :changed, :out_of_sync, :scheduled]
       attr_accessor *STATES
 
       attr_reader :source_description, :default_log_level, :time, :resource
+      attr_reader :change_count, :out_of_sync_count
 
       # Provide a boolean method for each of the states.
       STATES.each do |attr|
@@ -28,6 +28,13 @@ module Puppet
         @events << event
         if event.status == 'failure'
           self.failed = true
+        elsif event.status == 'success'
+          @change_count += 1
+          @changed = true
+        end
+        if event.status != 'audit'
+          @out_of_sync_count += 1
+          @out_of_sync = true
         end
       end
 
@@ -38,6 +45,8 @@ module Puppet
       def initialize(resource)
         @source_description = resource.path
         @resource = resource.to_s
+        @change_count = 0
+        @out_of_sync_count = 0
 
         [:file, :line, :version].each do |attr|
           send(attr.to_s + "=", resource.send(attr))
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 2d49062dd..4db971477 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -6,7 +6,6 @@ require 'puppet/util/tagging'
 require 'puppet/application'
 
 class Puppet::Transaction
-  require 'puppet/transaction/change'
   require 'puppet/transaction/event'
   require 'puppet/transaction/event_manager'
   require 'puppet/transaction/resource_harness'
diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
deleted file mode 100644
index d57ac1917..000000000
--- a/lib/puppet/transaction/change.rb
+++ /dev/null
@@ -1,75 +0,0 @@
-require 'puppet/transaction'
-require 'puppet/transaction/event'
-
-# Handle all of the work around performing an actual change,
-# including calling 'sync' on the properties and producing events.
-class Puppet::Transaction::Change
-  attr_accessor :is, :should, :property, :proxy, :auditing, :old_audit_value
-
-  def auditing?
-    auditing
-  end
-
-  def initialize(property, currentvalue)
-    @property = property
-    @is = currentvalue
-
-    @should = property.should
-
-    @changed = false
-  end
-
-  def apply
-    event = property.event
-    event.previous_value = is
-    event.desired_value = should
-    event.historical_value = old_audit_value
-
-    if auditing? and old_audit_value != is
-      event.message = "audit change: previously recorded value #{property.is_to_s(old_audit_value)} has been changed to #{property.is_to_s(is)}"
-      event.status = "audit"
-      event.audited = true
-      brief_audit_message = " (previously recorded value was #{property.is_to_s(old_audit_value)})" 
-    else
-      brief_audit_message = "" 
-    end
-
-    if property.insync?(is)
-      # nothing happens
-    elsif noop?
-      event.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)#{brief_audit_message}"
-      event.status = "noop"
-    else
-      property.sync
-      event.message = [ property.change_to_s(is, should), brief_audit_message ].join
-      event.status = "success"
-    end
-    event
-  rescue => detail
-    puts detail.backtrace if Puppet[:trace]
-    event.status = "failure"
-
-    event.message = "change from #{property.is_to_s(is)} to #{property.should_to_s(should)} failed: #{detail}"
-    event
-  ensure
-    event.send_log
-  end
-
-  # Is our property noop?  This is used for generating special events.
-  def noop?
-    @property.noop
-  end
-
-  # The resource that generated this change.  This is used for handling events,
-  # and the proxy resource is used for generated resources, since we can't
-  # send an event to a resource we don't have a direct relationship with.  If we
-  # have a proxy resource, then the events will be considered to be from
-  # that resource, rather than us, so the graph resolution will still work.
-  def resource
-    self.proxy || @property.resource
-  end
-
-  def to_s
-    "change #{@property.change_to_s(@is, @should)}"
-  end
-end
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index c978e5545..cb9a193b9 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -7,22 +7,15 @@ class Puppet::Transaction::ResourceHarness
   attr_reader :transaction
 
   def allow_changes?(resource)
-    return true unless resource.purging? and resource.deleting?
-    return true unless deps = relationship_graph.dependents(resource) and ! deps.empty? and deps.detect { |d| ! d.deleting? }
-
-    deplabel = deps.collect { |r| r.ref }.join(",")
-    plurality = deps.length > 1 ? "":"s"
-    resource.warning "#{deplabel} still depend#{plurality} on me -- not purging"
-    false
-  end
-
-  def apply_changes(status, changes)
-    changes.each do |change|
-      status << change.apply
-
-      cache(change.property.resource, change.property.name, change.is) if change.auditing?
+    if resource.purging? and resource.deleting? and deps = relationship_graph.dependents(resource) \
+            and ! deps.empty? and deps.detect { |d| ! d.deleting? }
+      deplabel = deps.collect { |r| r.ref }.join(",")
+      plurality = deps.length > 1 ? "":"s"
+      resource.warning "#{deplabel} still depend#{plurality} on me -- not purging"
+      false
+    else
+      true
     end
-    status.changed = true
   end
 
   # Used mostly for scheduling and auditing at this point.
@@ -35,66 +28,112 @@ class Puppet::Transaction::ResourceHarness
     Puppet::Util::Storage.cache(resource)[name] = value
   end
 
-  def changes_to_perform(status, resource)
+  def perform_changes(resource)
     current = resource.retrieve_resource
 
     cache resource, :checked, Time.now
 
     return [] if ! allow_changes?(resource)
 
-    audited = copy_audited_parameters(resource, current)
+    current_values = current.to_hash
+    historical_values = Puppet::Util::Storage.cache(resource).dup
+    desired_values = resource.to_resource.to_hash
+    audited_params = (resource[:audit] || []).map { |p| p.to_sym }
+    synced_params = []
 
-    if param = resource.parameter(:ensure)
-      return [] if absent_and_not_being_created?(current, param)
-      unless ensure_is_insync?(current, param)
-        audited.keys.reject{|name| name == :ensure}.each do |name|
-          resource.parameter(name).notice "audit change: previously recorded value #{audited[name]} has been changed to #{current[param]}"
-          cache(resource, name, current[param])
+    # Record the current state in state.yml.
+    audited_params.each do |param|
+      cache(resource, param, current_values[param])
+    end
+
+    # Update the machine state & create logs/events
+    events = []
+    ensure_param = resource.parameter(:ensure)
+    if desired_values[:ensure] && !ensure_param.insync?(current_values[:ensure])
+      events << apply_parameter(ensure_param, current_values[:ensure], audited_params.include?(:ensure), historical_values[:ensure])
+      synced_params << :ensure
+    elsif current_values[:ensure] != :absent
+      work_order = resource.properties # Note: only the resource knows what order to apply changes in
+      work_order.each do |param|
+        if !param.insync?(current_values[param.name])
+          events << apply_parameter(param, current_values[param.name], audited_params.include?(param.name), historical_values[param.name])
+          synced_params << param.name
         end
-        return [Puppet::Transaction::Change.new(param, current[:ensure])]
       end
-      return [] if ensure_should_be_absent?(current, param)
     end
 
-    resource.properties.reject { |param| param.name == :ensure }.select do |param|
-      (audited.include?(param.name) && audited[param.name] != current[param.name]) || (param.should != nil && !param_is_insync?(current, param))
-    end.collect do |param|
-      change = Puppet::Transaction::Change.new(param, current[param.name])
-      change.auditing = true if audited.include?(param.name)
-      change.old_audit_value = audited[param.name]
-      change
+    # Add more events to capture audit results
+    audited_params.each do |param_name|
+      if historical_values.include?(param_name)
+        if historical_values[param_name] != current_values[param_name] && !synced_params.include?(param_name)
+          event = create_change_event(resource.parameter(param_name), current_values[param_name], true, historical_values[param_name])
+          event.send_log
+          events << event
+        end
+      else
+        resource.property(param_name).notice "audit change: newly-recorded value #{current_values[param_name]}"
+      end
     end
+
+    events
   end
 
-  def copy_audited_parameters(resource, current)
-    return {} unless audit = resource[:audit]
-    audit = Array(audit).collect { |p| p.to_sym }
-    audited = {}
-    audit.find_all do |param|
-      if value = cached(resource, param)
-        audited[param] = value
-      else
-        resource.property(param).notice "audit change: newly-recorded recorded value #{current[param]}"
-        cache(resource, param, current[param])
-      end
+  def create_change_event(property, current_value, do_audit, historical_value)
+    event = property.event
+    event.previous_value = current_value
+    event.desired_value = property.should
+    event.historical_value = historical_value
+
+    if do_audit and historical_value != current_value
+      event.message = "audit change: previously recorded value #{property.is_to_s(historical_value)} has been changed to #{property.is_to_s(current_value)}"
+      event.status = "audit"
+      event.audited = true
     end
 
-    audited
+    event
+  end
+
+  def apply_parameter(property, current_value, do_audit, historical_value)
+    event = create_change_event(property, current_value, do_audit, historical_value)
+
+    if event.audited && historical_value
+      brief_audit_message = " (previously recorded value was #{property.is_to_s(historical_value)})"
+    else
+      brief_audit_message = ""
+    end
+
+    if property.noop
+      event.message = "current_value #{property.is_to_s(current_value)}, should be #{property.should_to_s(property.should)} (noop)#{brief_audit_message}"
+      event.status = "noop"
+    else
+      property.sync
+      event.message = [ property.change_to_s(current_value, property.should), brief_audit_message ].join
+      event.status = "success"
+    end
+    event
+  rescue => detail
+    puts detail.backtrace if Puppet[:trace]
+    event.status = "failure"
+
+    event.message = "change from #{property.is_to_s(current_value)} to #{property.should_to_s(property.should)} failed: #{detail}"
+    event
+  ensure
+    event.send_log
   end
 
   def evaluate(resource)
     start = Time.now
     status = Puppet::Resource::Status.new(resource)
 
-    if changes = changes_to_perform(status, resource) and ! changes.empty?
-      status.out_of_sync = true
-      status.change_count = changes.length
-      apply_changes(status, changes)
-      if ! resource.noop?
-        cache(resource, :synced, Time.now)
-        resource.flush if resource.respond_to?(:flush)
-      end
+    perform_changes(resource).each do |event|
+      status << event
     end
+
+    if status.changed? && ! resource.noop?
+      cache(resource, :synced, Time.now)
+      resource.flush if resource.respond_to?(:flush)
+    end
+
     return status
   rescue => detail
     resource.fail "Could not create resource status: #{detail}" unless status
diff --git a/spec/unit/resource/status_spec.rb b/spec/unit/resource/status_spec.rb
index 425015a13..bda8aea00 100755
--- a/spec/unit/resource/status_spec.rb
+++ b/spec/unit/resource/status_spec.rb
@@ -10,7 +10,7 @@ describe Puppet::Resource::Status do
     @status = Puppet::Resource::Status.new(@resource)
   end
 
-  [:node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time, :change_count].each do |attr|
+  [:node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time].each do |attr|
     it "should support #{attr}" do
       @status.send(attr.to_s + "=", "foo")
       @status.send(attr).should == "foo"
@@ -100,4 +100,39 @@ describe Puppet::Resource::Status do
     (@status << event).should equal(@status)
     @status.events.should == [event]
   end
+
+  it "should count the number of successful events and set changed" do
+    3.times{ @status << Puppet::Transaction::Event.new(:status => 'success') }
+    @status.change_count.should == 3
+
+    @status.changed.should == true
+    @status.out_of_sync.should == true
+  end
+
+  it "should not start with any changes" do
+    @status.change_count.should == 0
+
+    @status.changed.should be_false
+    @status.out_of_sync.should be_false
+  end
+
+  it "should not treat failure, audit, or noop events as changed" do
+    ['failure', 'audit', 'noop'].each do |s| @status << Puppet::Transaction::Event.new(:status => s) end
+    @status.change_count.should == 0
+    @status.changed.should be_false
+  end
+
+  it "should not treat audit events as out of sync" do
+    @status << Puppet::Transaction::Event.new(:status => 'audit')
+    @status.out_of_sync_count.should == 0
+    @status.out_of_sync.should be_false
+  end
+
+  ['failure', 'noop', 'success'].each do |event_status|
+    it "should treat #{event_status} events as out of sync" do
+      3.times do @status << Puppet::Transaction::Event.new(:status => event_status) end
+      @status.out_of_sync_count.should == 3
+      @status.out_of_sync.should == true
+    end
+  end
 end
diff --git a/spec/unit/transaction/change_spec.rb b/spec/unit/transaction/change_spec.rb
deleted file mode 100755
index fbc662df0..000000000
--- a/spec/unit/transaction/change_spec.rb
+++ /dev/null
@@ -1,206 +0,0 @@
-#!/usr/bin/env ruby
-
-require File.dirname(__FILE__) + '/../../spec_helper'
-
-require 'puppet/transaction/change'
-
-describe Puppet::Transaction::Change do
-  Change = Puppet::Transaction::Change
-
-  describe "when initializing" do
-    before do
-      @property = stub 'property', :path => "/property/path", :should => "shouldval"
-    end
-
-    it "should require the property and current value" do
-      lambda { Change.new }.should raise_error
-    end
-
-    it "should set its property to the provided property" do
-      Change.new(@property, "value").property.should == :property
-    end
-
-    it "should set its 'is' value to the provided value" do
-      Change.new(@property, "value").is.should == "value"
-    end
-
-    it "should retrieve the 'should' value from the property" do
-      # Yay rspec :)
-      Change.new(@property, "value").should.should == @property.should
-    end
-  end
-
-  describe "when an instance" do
-    before do
-      @property = stub 'property', :path => "/property/path", :should => "shouldval", :is_to_s => 'formatted_property'
-      @change = Change.new(@property, "value")
-    end
-
-    it "should be noop if the property is noop" do
-      @property.expects(:noop).returns true
-      @change.noop?.should be_true
-    end
-
-    it "should be auditing if set so" do
-      @change.auditing = true
-      @change.must be_auditing
-    end
-
-    it "should set its resource to the proxy if it has one" do
-      @change.proxy = :myresource
-      @change.resource.should == :myresource
-    end
-
-    it "should set its resource to the property's resource if no proxy is set" do
-      @property.expects(:resource).returns :myresource
-      @change.resource.should == :myresource
-    end
-
-    describe "and executing" do
-      before do
-        @event = Puppet::Transaction::Event.new(:myevent)
-        @event.stubs(:send_log)
-        @change.stubs(:noop?).returns false
-        @property.stubs(:event).returns @event
-
-        @property.stub_everything
-        @property.stubs(:resource).returns "myresource"
-        @property.stubs(:name).returns :myprop
-      end
-
-      describe "in noop mode" do
-        before { @change.stubs(:noop?).returns true }
-
-        it "should log that it is in noop" do
-          @property.expects(:is_to_s)
-          @property.expects(:should_to_s)
-
-          @event.expects(:message=).with { |msg| msg.include?("should be") }
-
-          @change.apply
-        end
-
-        it "should produce a :noop event and return" do
-          @property.stub_everything
-          @property.expects(:sync).never.never.never.never.never # VERY IMPORTANT
-
-          @event.expects(:status=).with("noop")
-
-          @change.apply.should == @event
-        end
-      end
-
-      describe "in audit mode" do
-        before do 
-          @change.auditing = true
-          @change.old_audit_value = "old_value"
-          @property.stubs(:insync?).returns(true)
-        end
-
-        it "should log that it is in audit mode" do
-          message = nil
-          @event.expects(:message=).with { |msg| message = msg }
-
-          @change.apply
-          message.should == "audit change: previously recorded value formatted_property has been changed to formatted_property"
-        end
-
-        it "should produce a :audit event and return" do
-          @property.stub_everything
-
-          @event.expects(:status=).with("audit")
-
-          @change.apply.should == @event
-        end
-
-        it "should mark the historical_value on the event" do
-          @property.stub_everything
-
-          @change.apply.historical_value.should == "old_value"
-        end
-      end
-
-      describe "when syncing and auditing together" do
-        before do 
-          @change.auditing = true
-          @change.old_audit_value = "old_value"
-          @property.stubs(:insync?).returns(false)
-        end
-
-        it "should sync the property" do
-          @property.expects(:sync)
-
-          @change.apply
-        end
-
-        it "should produce a success event" do
-          @property.stub_everything
-
-          @change.apply.status.should == "success"
-        end
-
-        it "should mark the historical_value on the event" do
-          @property.stub_everything
-
-          @change.apply.historical_value.should == "old_value"
-        end
-      end
-
-      it "should sync the property" do
-        @property.expects(:sync)
-
-        @change.apply
-      end
-
-      it "should return the default event if syncing the property returns nil" do
-        @property.stubs(:sync).returns nil
-
-        @property.expects(:event).with(nil).returns @event
-
-        @change.apply.should == @event
-      end
-
-      it "should return the default event if syncing the property returns an empty array" do
-        @property.stubs(:sync).returns []
-
-        @property.expects(:event).with(nil).returns @event
-
-        @change.apply.should == @event
-      end
-
-      it "should log the change" do
-        @property.expects(:sync).returns [:one]
-
-        @event.expects(:send_log)
-
-        @change.apply
-      end
-
-      it "should set the event's message to the change log" do
-        @property.expects(:change_to_s).returns "my change"
-        @change.apply.message.should == "my change"
-      end
-
-      it "should set the event's status to 'success'" do
-        @change.apply.status.should == "success"
-      end
-
-      describe "and the change fails" do
-        before { @property.expects(:sync).raises "an exception" }
-
-        it "should catch the exception and log the err" do
-          @event.expects(:send_log)
-          lambda { @change.apply }.should_not raise_error
-        end
-
-        it "should mark the event status as 'failure'" do
-          @change.apply.status.should == "failure"
-        end
-
-        it "should set the event log to a failure log" do
-          @change.apply.message.should be_include("failed")
-        end
-      end
-    end
-  end
-end
diff --git a/spec/unit/transaction/report_spec.rb b/spec/unit/transaction/report_spec.rb
index 604c2f54d..34c6ecd96 100755
--- a/spec/unit/transaction/report_spec.rb
+++ b/spec/unit/transaction/report_spec.rb
@@ -170,7 +170,7 @@ describe Puppet::Transaction::Report do
 
     describe "for changes" do
       it "should provide the number of changes from the resource statuses" do
-        add_statuses(3) { |status| status.change_count = 3 }
+        add_statuses(3) { |status| 3.times { status << Puppet::Transaction::Event.new(:status => 'success') } }
         @report.calculate_metrics
         metric(:changes, :total).should == 9
       end
diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb
index b143c21ed..387deca61 100755
--- a/spec/unit/transaction/resource_harness_spec.rb
+++ b/spec/unit/transaction/resource_harness_spec.rb
@@ -49,354 +49,188 @@ describe Puppet::Transaction::ResourceHarness do
       @harness.evaluate(@resource).should be_failed
     end
 
-    it "should use the status and retrieved state to determine which changes need to be made" do
-      @harness.expects(:changes_to_perform).with(@status, @resource).returns []
-      @harness.evaluate(@resource)
-    end
-
-    it "should mark the status as out of sync and apply the created changes if there are any" do
-      changes = %w{mychanges}
-      @harness.expects(:changes_to_perform).returns changes
-      @harness.expects(:apply_changes).with(@status, changes)
-      @harness.evaluate(@resource).should be_out_of_sync
-    end
-
-    it "should cache the last-synced time" do
-      changes = %w{mychanges}
-      @harness.stubs(:changes_to_perform).returns changes
-      @harness.stubs(:apply_changes)
-      @harness.expects(:cache).with { |resource, name, time| name == :synced and time.is_a?(Time) }
-      @harness.evaluate(@resource)
-    end
-
-    it "should flush the resource when applying changes if appropriate" do
-      changes = %w{mychanges}
-      @harness.stubs(:changes_to_perform).returns changes
-      @harness.stubs(:apply_changes)
-      @resource.expects(:flush)
-      @harness.evaluate(@resource)
-    end
-
-    it "should use the status and retrieved state to determine which changes need to be made" do
-      @harness.expects(:changes_to_perform).with(@status, @resource).returns []
-      @harness.evaluate(@resource)
-    end
-
-    it "should not attempt to apply changes if none need to be made" do
-      @harness.expects(:changes_to_perform).returns []
-      @harness.expects(:apply_changes).never
-      @harness.evaluate(@resource).should_not be_out_of_sync
-    end
-
     it "should store the resource's evaluation time in the resource status" do
       @harness.evaluate(@resource).evaluation_time.should be_instance_of(Float)
     end
-
-    it "should set the change count to the total number of changes" do
-      changes = %w{a b c d}
-      @harness.expects(:changes_to_perform).returns changes
-      @harness.expects(:apply_changes).with(@status, changes)
-      @harness.evaluate(@resource).change_count.should == 4
-    end
-  end
-
-  describe "when creating changes" do
-    before do
-      @current_state = Puppet::Resource.new(:file, "/my/file")
-      @resource.stubs(:retrieve).returns @current_state
-      Puppet.features.stubs(:root?).returns true
-    end
-
-    it "should retrieve the current values from the resource" do
-      @resource.expects(:retrieve).returns @current_state
-      @harness.changes_to_perform(@status, @resource)
-    end
-
-    it "should cache that the resource was checked" do
-      @harness.expects(:cache).with { |resource, 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
-
-      change = stub 'change'
-      Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:ensure), :absent).returns change
-
-      @harness.changes_to_perform(@status, @resource)[0].should equal(change)
-    end
-
-    it "should not attempt to manage properties that do not have desired values set" do
-      mode = @resource.newattr(:mode)
-      @current_state[:mode] = :absent
-
-      mode.expects(:insync?).never
-
-      @harness.changes_to_perform(@status, @resource)
-    end
-
-#   it "should copy audited parameters" do
-#     @resource[:audit] = :mode
-#     @harness.cache(@resource, :mode, "755")
-#     @harness.changes_to_perform(@status, @resource)
-#     @resource[:mode].should == "755"
-#   end
-
-    it "should mark changes created as a result of auditing as auditing changes" do
-      @current_state[:mode] = 0644
-      @resource[:audit] = :mode
-      @harness.cache(@resource, :mode, "755")
-      @harness.changes_to_perform(@status, @resource)[0].must be_auditing
-    end
-
-    describe "and the 'ensure' parameter is present but not in sync" do
-      it "should return a single change for the 'ensure' parameter" do
-        @resource[:ensure] = :present
-        @resource[:mode] = "755"
-        @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 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
-        @current_state[:mode] = :absent
-
-        @harness.changes_to_perform(@status, @resource).should == []
-      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
-        @resource[:mode] = "755"
-        @resource[:owner] = 0
-        @current_state[:ensure] = :present
-        @current_state[:mode] = 0444
-        @current_state[:owner] = 50
-
-        mode = stub_everything 'mode_change'
-        owner = stub_everything 'owner_change'
-        Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:mode), 0444).returns mode
-        Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:owner), 50).returns owner
-
-        changes = @harness.changes_to_perform(@status, @resource)
-        changes.length.should == 2
-        changes.should be_include(mode)
-        changes.should be_include(owner)
-      end
-    end
-
-    describe "and all parameters are in sync" do
-      it "should return an empty array" do
-        @resource[:ensure] = :present
-        @resource[:mode] = "755"
-        @current_state[:ensure] = :present
-        @current_state[:mode] = "755"
-        @harness.changes_to_perform(@status, @resource).should == []
-      end
-    end
   end
 
   describe "when applying changes" do
-    before do
-      @change1 = stub 'change1', :apply => stub("event", :status => "success"), :auditing? => false
-      @change2 = stub 'change2', :apply => stub("event", :status => "success"), :auditing? => false
-      @changes = [@change1, @change2]
-    end
-
-    it "should apply the change" do
-      @change1.expects(:apply).returns( stub("event", :status => "success") )
-      @change2.expects(:apply).returns( stub("event", :status => "success") )
-
-      @harness.apply_changes(@status, @changes)
-    end
-
-    it "should mark the resource as changed" do
-      @harness.apply_changes(@status, @changes)
-
-      @status.should be_changed
-    end
-
-    it "should queue the resulting event" do
-      @harness.apply_changes(@status, @changes)
-
-      @status.events.should be_include(@change1.apply)
-      @status.events.should be_include(@change2.apply)
-    end
-
-    it "should cache the new value if it is an auditing change" do
-      @change1.expects(:auditing?).returns true
-      property = stub 'property', :name => "foo", :resource => "myres"
-      @change1.stubs(:property).returns property
-      @change1.stubs(:is).returns "myval"
-
-      @harness.apply_changes(@status, @changes)
-
-      @harness.cached("myres", "foo").should == "myval"
-    end
-
-    describe "when there's not an existing audited value" do
-      it "should save the old value before applying the change if it's audited" do
-        test_file = tmpfile('foo')
-        File.open(test_file, "w", 0750).close
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == "750"
-
-        (File.stat(test_file).mode & 0777).should == 0755
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/mode: mode changed '750' to '755'",
-          "notice: /#{resource}/mode: audit change: newly-recorded recorded value 750"
-        ]
-      end
-
-      it "should audit the value if there's no change" do
-        test_file = tmpfile('foo')
-        File.open(test_file, "w", 0755).close
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == "755"
-
-        (File.stat(test_file).mode & 0777).should == 0755
-
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/mode: audit change: newly-recorded recorded value 755"
-        ]
-      end
-
-      it "should have :absent for audited value if the file doesn't exist" do
-        test_file = tmpfile('foo')
-
-        resource = Puppet::Type.type(:file).new :ensure => 'present', :path => test_file, :mode => '755', :audit => :mode
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == :absent
-
-        (File.stat(test_file).mode & 0777).should == 0755
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/ensure: created",
-          "notice: /#{resource}/mode: audit change: newly-recorded recorded value absent"
-        ]
-      end
-
-      it "should do nothing if there are no changes to make and the stored value is correct" do
-        test_file = tmpfile('foo')
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode, :ensure => 'absent'
-        @harness.cache(resource, :mode, :absent)
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == :absent
-
-        File.exists?(test_file).should == false
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ []
-      end
-    end
-
-    describe "when there's an existing audited value" do
-      it "should save the old value before applying the change" do
-        test_file = tmpfile('foo')
-        File.open(test_file, "w", 0750).close
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :audit => :mode
-        @harness.cache(resource, :mode, '555')
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == "750"
-
-        (File.stat(test_file).mode & 0777).should == 0750
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to 750"
-        ]
-      end
-
-      it "should save the old value before applying the change" do
-        test_file = tmpfile('foo')
-        File.open(test_file, "w", 0750).close
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
-        @harness.cache(resource, :mode, '555')
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == "750"
-
-        (File.stat(test_file).mode & 0777).should == 0755
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/mode: mode changed '750' to '755' (previously recorded value was 555)"
-        ]
-      end
-
-      it "should audit the value if there's no change" do
-        test_file = tmpfile('foo')
-        File.open(test_file, "w", 0755).close
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
-        @harness.cache(resource, :mode, '555')
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == "755"
-
-        (File.stat(test_file).mode & 0777).should == 0755
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to 755"
-        ]
-      end
-
-      it "should have :absent for audited value if the file doesn't exist" do
-        test_file = tmpfile('foo')
-
-        resource = Puppet::Type.type(:file).new :ensure => 'present', :path => test_file, :mode => '755', :audit => :mode
-        @harness.cache(resource, :mode, '555')
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == :absent
-
-        (File.stat(test_file).mode & 0777).should == 0755
-
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
-          "notice: /#{resource}/ensure: created", "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to absent"
-        ]
-      end
-
-      it "should do nothing if there are no changes to make and the stored value is correct" do
-        test_file = tmpfile('foo')
-        File.open(test_file, "w", 0755).close
-
-        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
-        @harness.cache(resource, :mode, '755')
-
-        @harness.evaluate(resource)
-        @harness.cached(resource, :mode).should == "755"
-
-        (File.stat(test_file).mode & 0777).should == 0755
-        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ []
-      end
+    [false, true].each do |noop_mode|; describe (noop_mode ? "in noop mode" : "in normal mode") do
+      [nil, '750'].each do |machine_state|; describe (machine_state ? "with a file initially present" : "with no file initially present") do
+        [nil, '750', '755'].each do |yaml_mode|
+          [nil, :file, :absent].each do |yaml_ensure|; describe "with mode=#{yaml_mode.inspect} and ensure=#{yaml_ensure.inspect} stored in state.yml" do
+            [false, true].each do |auditing_ensure|
+              [false, true].each do |auditing_mode|
+                auditing = []
+                auditing.push(:mode) if auditing_mode
+                auditing.push(:ensure) if auditing_ensure
+                [nil, :file, :absent].each do |ensure_property| # what we set "ensure" to in the manifest
+                  [nil, '750', '755'].each do |mode_property| # what we set "mode" to in the manifest
+                    manifest_settings = {}
+                    manifest_settings[:audit] = auditing if !auditing.empty?
+                    manifest_settings[:ensure] = ensure_property if ensure_property
+                    manifest_settings[:mode] = mode_property if mode_property
+                    describe "with manifest settings #{manifest_settings.inspect}" do; it "should behave properly" do
+                      # Set up preconditions
+                      test_file = tmpfile('foo')
+                      if machine_state
+                        File.open(test_file, 'w', machine_state.to_i(8)).close
+                      end
+
+                      Puppet[:noop] = noop_mode
+                      params = { :path => test_file, :backup => false }
+                      params.merge!(manifest_settings)
+                      resource = Puppet::Type.type(:file).new params
+
+                      @harness.cache(resource, :mode, yaml_mode) if yaml_mode
+                      @harness.cache(resource, :ensure, yaml_ensure) if yaml_ensure
+
+                      resource.expects(:err).never # make sure no exceptions get swallowed
+                      status = @harness.evaluate(resource) # do the thing
+
+                      # check that the state of the machine has been properly updated
+                      expected_logs = []
+                      if auditing_mode
+                        @harness.cached(resource, :mode).should == (machine_state || :absent)
+                      else
+                        @harness.cached(resource, :mode).should == yaml_mode
+                      end
+                      if auditing_ensure
+                        @harness.cached(resource, :ensure).should == (machine_state ? :file : :absent)
+                      else
+                        @harness.cached(resource, :ensure).should == yaml_ensure
+                      end
+                      if ensure_property == :file
+                        file_would_be_there_if_not_noop = true
+                      elsif ensure_property == nil
+                        file_would_be_there_if_not_noop = machine_state != nil
+                      else # ensure_property == :absent
+                        file_would_be_there_if_not_noop = false
+                      end
+                      file_should_be_there = noop_mode ? machine_state != nil : file_would_be_there_if_not_noop
+                      File.exists?(test_file).should == file_should_be_there
+                      if file_should_be_there
+                        if noop_mode
+                          expected_file_mode = machine_state
+                        else
+                          expected_file_mode = mode_property || machine_state
+                        end
+                        if !expected_file_mode
+                          # we didn't specify a mode and the file was created, so mode comes from umode
+                        else
+                          file_mode = File.stat(test_file).mode & 0777
+                          file_mode.should == expected_file_mode.to_i(8)
+                        end
+                      end
+
+                      # Test log output for the "mode" parameter
+                      previously_recorded_mode_already_logged = false
+                      if machine_state && file_would_be_there_if_not_noop && mode_property && machine_state != mode_property
+                        if noop_mode
+                          what_happened = "current_value #{machine_state}, should be #{mode_property} (noop)"
+                        else
+                          what_happened = "mode changed '#{machine_state}' to '#{mode_property}'"
+                        end
+                        if auditing_mode && yaml_mode && yaml_mode != machine_state
+                          previously_recorded_mode_already_logged = true
+                          expected_logs << "notice: /#{resource}/mode: #{what_happened} (previously recorded value was #{yaml_mode})"
+                        else
+                          expected_logs << "notice: /#{resource}/mode: #{what_happened}"
+                        end
+                      end
+                      if @harness.cached(resource, :mode) && @harness.cached(resource, :mode) != yaml_mode
+                        if yaml_mode
+                          unless previously_recorded_mode_already_logged
+                            expected_logs << "notice: /#{resource}/mode: audit change: previously recorded value #{yaml_mode} has been changed to #{@harness.cached(resource, :mode)}"
+                          end
+                        else
+                          expected_logs << "notice: /#{resource}/mode: audit change: newly-recorded value #{@harness.cached(resource, :mode)}"
+                        end
+                      end
+
+                      # Test log output for the "ensure" parameter
+                      previously_recorded_ensure_already_logged = false
+                      if file_would_be_there_if_not_noop != (machine_state != nil)
+                        if noop_mode
+                          what_happened = "current_value #{machine_state ? 'file' : 'absent'}, should be #{file_would_be_there_if_not_noop ? 'file' : 'absent'} (noop)"
+                        else
+                          what_happened = file_would_be_there_if_not_noop ? 'created' : 'removed'
+                        end
+                        if auditing_ensure && yaml_ensure && yaml_ensure != (machine_state ? :file : :absent)
+                          previously_recorded_ensure_already_logged = true
+                          expected_logs << "notice: /#{resource}/ensure: #{what_happened} (previously recorded value was #{yaml_ensure})"
+                        else
+                          expected_logs << "notice: /#{resource}/ensure: #{what_happened}"
+                        end
+                      end
+                      if @harness.cached(resource, :ensure) && @harness.cached(resource, :ensure) != yaml_ensure
+                        if yaml_ensure
+                          unless previously_recorded_ensure_already_logged
+                            expected_logs << "notice: /#{resource}/ensure: audit change: previously recorded value #{yaml_ensure} has been changed to #{@harness.cached(resource, :ensure)}"
+                          end
+                        else
+                          expected_logs << "notice: /#{resource}/ensure: audit change: newly-recorded value #{@harness.cached(resource, :ensure)}"
+                        end
+                      end
+
+                      # Actually check the logs.
+                      @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ expected_logs
+
+                      # All the log messages should show up as events except the "newly-recorded" ones.
+                      expected_event_logs = @logs.reject {|l| l.message =~ /newly-recorded/ }
+                      status.events.map {|e| e.message}.should =~ expected_event_logs.map {|l| l.message }
+
+                      # Check change count - this is the number of changes that actually occurred.
+                      expected_change_count = 0
+                      if (machine_state != nil) != file_should_be_there
+                        expected_change_count = 1
+                      elsif machine_state != nil
+                        if expected_file_mode != machine_state
+                          expected_change_count = 1
+                        end
+                      end
+                      status.change_count.should == expected_change_count
+
+                      # Check out of sync count - this is the number
+                      # of changes that would have occurred in
+                      # non-noop mode.
+                      expected_out_of_sync_count = 0
+                      if (machine_state != nil) != file_would_be_there_if_not_noop
+                        expected_out_of_sync_count = 1
+                      elsif machine_state != nil
+                        if mode_property != nil && mode_property != machine_state
+                          expected_out_of_sync_count = 1
+                        end
+                      end
+                      if !noop_mode
+                        expected_out_of_sync_count.should == expected_change_count
+                      end
+                      status.out_of_sync_count.should == expected_out_of_sync_count
+
+                      # Check legacy summary fields
+                      status.changed.should == (expected_change_count == 0 ? nil : true)
+                      status.out_of_sync.should == (expected_out_of_sync_count == 0 ? nil : true)
+
+                      # Check the :synced field on state.yml
+                      synced_should_be_set = !noop_mode && status.changed != nil
+                      (@harness.cached(resource, :synced) != nil).should == synced_should_be_set
+                    end; end
+                  end
+                end
+              end
+            end
+          end; end
+        end
+      end; end
+    end; end
+
+    it "should not apply changes if allow_changes?() returns false" do
+      test_file = tmpfile('foo')
+      resource = Puppet::Type.type(:file).new :path => test_file, :backup => false, :ensure => :file
+      resource.expects(:err).never # make sure no exceptions get swallowed
+      @harness.expects(:allow_changes?).with(resource).returns false
+      status = @harness.evaluate(resource)
+      File.exists?(test_file).should == false
     end
   end