Merge branch 'ticket/2.6.next/5715' into 2.6.next

* ticket/2.6.next/5715:
  (#5715) Added attributes resource_type and title to Puppet::Resource::Status.
  (#5715) Removed attribute source_description from the YAML representation of Puppet::Resource::Status.
  (#5715) Removed unnecessary attributes from YAML of Puppet::Transaction::Event.
  (#5715) Make certain report attributes always present.
  (#5715) Changed the type of metric names to always be strings.
  (#5715) Add status attribute to reports.
  (#5715) Made the report "calculate" methods strictly functional.
  (#5715) Made the changes/total and events/total metrics always present
  (#5715) Refactor in preparation for adding a status attribute to reports.
  (#5715) Added total time to inspect reports and made inspect metrics more consistent.
  (#5715) Removed Puppet::Transaction::Report#external_times from YAML output.
  (#5715) Added total time metric to apply reports.
  (#5715) Removed redundant attribute Transaction::Event#version
  (#5715) Removed redundant attribute Resource::Status#version
  (#5715) Removed Puppet::Util::Log#version.
  (#5715) Removed the unused attribute Puppet::Transaction::Event#node
  (#5715) Removed Resource::Status#skipped_reason.  It was never used.
  (#5715) Prep work: Fixed add_statuses in report_spec.

23 files changed, 230 insertions, 159 deletions

diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb
index caa32a7c2..2f068a271 100644
--- a/lib/puppet/application/inspect.rb
+++ b/lib/puppet/application/inspect.rb
@@ -51,10 +51,8 @@ class Puppet::Application::Inspect < Puppet::Application
 
     @report.configuration_version = catalog.version
 
-    retrieval_time =  Time.now - retrieval_starttime
-    @report.add_times("config_retrieval", retrieval_time)
-
-    starttime = Time.now
+    inspect_starttime = Time.now
+    @report.add_times("config_retrieval", inspect_starttime - retrieval_starttime)
 
     catalog.to_ral.resources.each do |ral_resource|
       audited_attributes = ral_resource[:audit]
@@ -70,7 +68,9 @@ class Puppet::Application::Inspect < Puppet::Application
       @report.add_resource_status(status)
     end
 
-    @report.add_metric(:time, {"config_retrieval" => retrieval_time, "inspect" => Time.now - starttime})
+    finishtime = Time.now
+    @report.add_times("inspect", finishtime - inspect_starttime)
+    @report.finalize_report
 
     begin
       @report.save
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 070176554..d3c902576 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -170,8 +170,8 @@ class Puppet::Configurer
     send_report(report, transaction)
   end
 
-  def send_report(report, trans = nil)
-    trans.generate_report if trans
+  def send_report(report, trans)
+    report.finalize_report if trans
     puts report.summary if Puppet[:summarize]
     report.save if Puppet[:report]
   rescue => detail
diff --git a/lib/puppet/resource/status.rb b/lib/puppet/resource/status.rb
index f34edc469..ee83004bb 100644
--- a/lib/puppet/resource/status.rb
+++ b/lib/puppet/resource/status.rb
@@ -4,13 +4,15 @@ module Puppet
       include Puppet::Util::Tagging
       include Puppet::Util::Logging
 
-      attr_accessor :resource, :node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time
+      attr_accessor :resource, :node, :file, :line, :current_values, :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
+      attr_reader :change_count, :out_of_sync_count, :resource_type, :title
+
+      YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title}
 
       # Provide a boolean method for each of the states.
       STATES.each do |attr|
@@ -47,14 +49,22 @@ module Puppet
         @resource = resource.to_s
         @change_count = 0
         @out_of_sync_count = 0
+        @changed = false
+        @out_of_sync = false
 
-        [:file, :line, :version].each do |attr|
+        [:file, :line].each do |attr|
           send(attr.to_s + "=", resource.send(attr))
         end
 
         tag(*resource.tags)
         @time = Time.now
         @events = []
+        @resource_type = resource.type.to_s.capitalize
+        @title = resource.title
+      end
+
+      def to_yaml_properties
+        (YAML_ATTRIBUTES & instance_variables).sort
       end
 
       private
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 4db971477..aa650eea1 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -221,12 +221,6 @@ class Puppet::Transaction
     end
   end
 
-  # Generate a transaction report.
-  def generate_report
-    @report.calculate_metrics
-    @report
-  end
-
   # Should we ignore tags?
   def ignore_tags?
     ! (@catalog.host_config? or Puppet[:name] == "puppet")
@@ -284,26 +278,6 @@ 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: #{detail}"
-      return
-    end
-
-    puts report.summary if Puppet[:summarize]
-
-    if Puppet[:report]
-      begin
-        report.save
-      rescue => detail
-        Puppet.err "Reporting failed: #{detail}"
-      end
-    end
-  end
-
   def add_resource_status(status)
     report.add_resource_status status
   end
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index da5b14727..64980f1d9 100644
--- a/lib/puppet/transaction/event.rb
+++ b/lib/puppet/transaction/event.rb
@@ -7,7 +7,8 @@ class Puppet::Transaction::Event
   include Puppet::Util::Tagging
   include Puppet::Util::Logging
 
-  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :node, :version, :file, :line, :source_description, :audited]
+  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :file, :line, :source_description, :audited]
+  YAML_ATTRIBUTES = %w{@audited @property @previous_value @desired_value @historical_value @message @name @status @time}
   attr_accessor *ATTRIBUTES
   attr_writer :tags
   attr_accessor :time
@@ -16,6 +17,7 @@ class Puppet::Transaction::Event
   EVENT_STATUSES = %w{noop success failure audit}
 
   def initialize(*args)
+    @audited = false
     options = args.last.is_a?(Hash) ? args.pop : ATTRIBUTES.inject({}) { |hash, attr| hash[attr] = args.pop; hash }
     options.each { |attr, value| send(attr.to_s + "=", value) unless value.nil? }
 
@@ -46,6 +48,10 @@ class Puppet::Transaction::Event
     message
   end
 
+  def to_yaml_properties
+    (YAML_ATTRIBUTES & instance_variables).sort
+  end
+
   private
 
   # If it's a failure, use 'err', else use either the resource's log level (if available)
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index 8a928454f..8e04759ad 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -11,7 +11,7 @@ class Puppet::Transaction::Report
   indirects :report, :terminus_class => :processor
 
   attr_accessor :configuration_version
-  attr_reader :resource_statuses, :logs, :metrics, :host, :time, :kind
+  attr_reader :resource_statuses, :logs, :metrics, :host, :time, :kind, :status
 
   # This is necessary since Marshall doesn't know how to
   # dump hash with default proc (see below @records)
@@ -43,11 +43,23 @@ class Puppet::Transaction::Report
     @resource_statuses[status.resource] = status
   end
 
-  def calculate_metrics
-    calculate_resource_metrics
-    calculate_time_metrics
-    calculate_change_metrics
-    calculate_event_metrics
+  def compute_status(resource_metrics, change_metric)
+    if (resource_metrics["failed"] || 0) > 0
+      'failed'
+    elsif change_metric > 0
+      'changed'
+    else
+      'unchanged'
+    end
+  end
+
+  def finalize_report
+    resource_metrics = add_metric(:resources, calculate_resource_metrics)
+    add_metric(:time, calculate_time_metrics)
+    change_metric = calculate_change_metric
+    add_metric(:changes, {"total" => change_metric})
+    add_metric(:events, calculate_event_metrics)
+    @status = compute_status(resource_metrics, change_metric)
   end
 
   def initialize(kind, configuration_version=nil)
@@ -61,6 +73,7 @@ class Puppet::Transaction::Report
     @report_format = 2
     @puppet_version = Puppet.version
     @configuration_version = configuration_version
+    @status = 'failed' # assume failed until the report is finalized
   end
 
   def name
@@ -96,46 +109,46 @@ class Puppet::Transaction::Report
   # individual bits represent the presence of different metrics.
   def exit_status
     status = 0
-    status |= 2 if @metrics["changes"][:total] > 0
-    status |= 4 if @metrics["resources"][:failed] > 0
+    status |= 2 if @metrics["changes"]["total"] > 0
+    status |= 4 if @metrics["resources"]["failed"] > 0
     status
   end
 
-  private
+  def to_yaml_properties
+    (instance_variables - ["@external_times"]).sort
+  end
 
-  def calculate_change_metrics
-    metrics = Hash.new(0)
-    resource_statuses.each do |name, status|
-      metrics[:total] += status.change_count if status.change_count
-    end
+  private
 
-    add_metric(:changes, metrics)
+  def calculate_change_metric
+    resource_statuses.map { |name, status| status.change_count || 0 }.inject(0) { |a,b| a+b }
   end
 
   def calculate_event_metrics
     metrics = Hash.new(0)
+    metrics["total"] = 0
     resource_statuses.each do |name, status|
-      metrics[:total] += status.events.length
+      metrics["total"] += status.events.length
       status.events.each do |event|
         metrics[event.status] += 1
       end
     end
 
-    add_metric(:events, metrics)
+    metrics
   end
 
   def calculate_resource_metrics
     metrics = Hash.new(0)
-    metrics[:total] = resource_statuses.length
+    metrics["total"] = resource_statuses.length
 
     resource_statuses.each do |name, status|
 
       Puppet::Resource::Status::STATES.each do |state|
-        metrics[state] += 1 if status.send(state)
+        metrics[state.to_s] += 1 if status.send(state)
       end
     end
 
-    add_metric(:resources, metrics)
+    metrics
   end
 
   def calculate_time_metrics
@@ -149,6 +162,8 @@ class Puppet::Transaction::Report
       metrics[name.to_s.downcase] = value
     end
 
-    add_metric(:time, metrics)
+    metrics["total"] = metrics.values.inject(0) { |a,b| a+b }
+
+    metrics
   end
 end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1b6e7dcd7..ea3944b4e 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -446,7 +446,7 @@ class Type
   # Create a transaction event.  Called by Transaction or by
   # a property.
   def event(options = {})
-    Puppet::Transaction::Event.new({:resource => self, :file => file, :line => line, :tags => tags, :version => version}.merge(options))
+    Puppet::Transaction::Event.new({:resource => self, :file => file, :line => line, :tags => tags}.merge(options))
   end
 
   # Let the catalog determine whether a given cached value is
diff --git a/lib/puppet/util/log.rb b/lib/puppet/util/log.rb
index 7764dc1d1..3fdac3f69 100644
--- a/lib/puppet/util/log.rb
+++ b/lib/puppet/util/log.rb
@@ -189,7 +189,7 @@ class Puppet::Util::Log
     @levels.include?(level)
   end
 
-  attr_accessor :time, :remote, :file, :line, :version, :source
+  attr_accessor :time, :remote, :file, :line, :source
   attr_reader :level, :message
 
   def initialize(args)
@@ -203,7 +203,7 @@ class Puppet::Util::Log
       tags.each { |t| self.tag(t) }
     end
 
-    [:file, :line, :version].each do |attr|
+    [:file, :line].each do |attr|
       next unless value = args[attr]
       send(attr.to_s + "=", value)
     end
@@ -234,7 +234,7 @@ class Puppet::Util::Log
 
       descriptors[:tags].each { |t| tag(t) }
 
-      [:file, :line, :version].each do |param|
+      [:file, :line].each do |param|
         next unless descriptors[param]
         send(param.to_s + "=", descriptors[param])
       end
diff --git a/lib/puppet/util/log_paths.rb b/lib/puppet/util/log_paths.rb
index f59197ed1..2fefd4505 100644
--- a/lib/puppet/util/log_paths.rb
+++ b/lib/puppet/util/log_paths.rb
@@ -15,7 +15,7 @@ module Puppet::Util::LogPaths
 
     descriptors[:tags] = tags
 
-    [:path, :file, :line, :version].each do |param|
+    [:path, :file, :line].each do |param|
       next unless value = send(param)
       descriptors[param] = value
     end
diff --git a/lib/puppet/util/logging.rb b/lib/puppet/util/logging.rb
index f20444a3b..bc52b17f0 100644
--- a/lib/puppet/util/logging.rb
+++ b/lib/puppet/util/logging.rb
@@ -26,7 +26,7 @@ module Puppet::Util::Logging
   end
 
   def log_metadata
-    [:file, :line, :version, :tags].inject({}) do |result, attr|
+    [:file, :line, :tags].inject({}) do |result, attr|
       result[attr] = send(attr) if respond_to?(attr)
       result
     end
diff --git a/lib/puppet/util/metric.rb b/lib/puppet/util/metric.rb
index 8f55e7b44..09bbb6137 100644
--- a/lib/puppet/util/metric.rb
+++ b/lib/puppet/util/metric.rb
@@ -132,6 +132,7 @@ class Puppet::Util::Metric
   end
 
   def newvalue(name,value,label = nil)
+    raise ArgumentError.new("metric name #{name.inspect} is not a string") unless name.is_a? String
     label ||= labelize(name)
     @values.push [name,label,value]
   end
diff --git a/spec/integration/indirector/report/rest_spec.rb b/spec/integration/indirector/report/rest_spec.rb
index 7fa026b73..5b5b2ddd8 100644
--- a/spec/integration/indirector/report/rest_spec.rb
+++ b/spec/integration/indirector/report/rest_spec.rb
@@ -67,30 +67,26 @@ describe "Report REST Terminus" do
     report = Puppet::Transaction::Report.new("apply")
 
     resourcemetrics = {
-      :total => 12,
-      :out_of_sync => 20,
-      :applied => 45,
-      :skipped => 1,
-      :restarted => 23,
-      :failed_restarts => 1,
-      :scheduled => 10
+      "total" => 12,
+      "out_of_sync" => 20,
+      "applied" => 45,
+      "skipped" => 1,
+      "restarted" => 23,
+      "failed_restarts" => 1,
+      "scheduled" => 10
     }
     report.add_metric(:resources, resourcemetrics)
 
     timemetrics = {
-      :resource1 => 10,
-      :resource2 => 50,
-      :resource3 => 40,
-      :resource4 => 20,
+      "resource1" => 10,
+      "resource2" => 50,
+      "resource3" => 40,
+      "resource4" => 20,
     }
     report.add_metric(:times, timemetrics)
 
 
-          report.add_metric(
-        :changes,
-        
-      :total => 20
-    )
+    report.add_metric(:changes, "total" => 20)
 
     report.save
   end
diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb
index 3b2a44f0f..cf73a3706 100755
--- a/spec/unit/configurer_spec.rb
+++ b/spec/unit/configurer_spec.rb
@@ -233,16 +233,8 @@ describe Puppet::Configurer, "when sending a report" do
     @trans = stub 'transaction'
   end
 
-  it "should require a report" do
-    lambda { @configurer.send_report }.should raise_error(ArgumentError)
-  end
-
-  it "should allow specification of a transaction" do
-    lambda { @configurer.send_report(@report, @trans)  }.should_not raise_error(ArgumentError)
-  end
-
-  it "should use any provided transaction to add metrics to the report" do
-    @trans.expects(:generate_report)
+  it "should finalize the report" do
+    @report.expects(:finalize_report)
     @configurer.send_report(@report, @trans)
   end
 
@@ -252,28 +244,28 @@ describe Puppet::Configurer, "when sending a report" do
     @report.expects(:summary).returns "stuff"
 
     @configurer.expects(:puts).with("stuff")
-    @configurer.send_report(@report)
+    @configurer.send_report(@report, nil)
   end
 
   it "should not print a report summary if not configured to do so" do
     Puppet.settings[:summarize] = false
 
     @configurer.expects(:puts).never
-    @configurer.send_report(@report)
+    @configurer.send_report(@report, nil)
   end
 
   it "should save the report if reporting is enabled" do
     Puppet.settings[:report] = true
 
     @report.expects(:save)
-    @configurer.send_report(@report)
+    @configurer.send_report(@report, nil)
   end
 
   it "should not save the report if reporting is disabled" do
     Puppet.settings[:report] = false
 
     @report.expects(:save).never
-    @configurer.send_report(@report)
+    @configurer.send_report(@report, nil)
   end
 
   it "should log but not fail if saving the report fails" do
@@ -282,7 +274,7 @@ describe Puppet::Configurer, "when sending a report" do
     @report.expects(:save).raises "whatever"
 
     Puppet.expects(:err)
-    lambda { @configurer.send_report(@report) }.should_not raise_error
+    lambda { @configurer.send_report(@report, nil) }.should_not raise_error
   end
 end
 
diff --git a/spec/unit/parameter_spec.rb b/spec/unit/parameter_spec.rb
index 966bbfb81..f8ab05d62 100755
--- a/spec/unit/parameter_spec.rb
+++ b/spec/unit/parameter_spec.rb
@@ -52,8 +52,7 @@ describe Puppet::Parameter do
     @resource.expects(:line).returns 10
     @resource.expects(:file).returns "file"
     @resource.expects(:tags).returns %w{one two}
-    @resource.expects(:version).returns 50
-    @parameter.source_descriptors.should == {:tags=>["one", "two", "foo"], :path=>"//foo", :version=>50, :file => "file", :line => 10}
+    @parameter.source_descriptors.should == {:tags=>["one", "two", "foo"], :path=>"//foo", :file => "file", :line => 10}
   end
 
   describe "when returning the value" do
diff --git a/spec/unit/resource/status_spec.rb b/spec/unit/resource/status_spec.rb
index bda8aea00..4e76fa463 100755
--- a/spec/unit/resource/status_spec.rb
+++ b/spec/unit/resource/status_spec.rb
@@ -10,7 +10,12 @@ describe Puppet::Resource::Status do
     @status = Puppet::Resource::Status.new(@resource)
   end
 
-  [:node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time].each do |attr|
+  it "should compute type and title correctly" do
+    @status.resource_type.should == "File"
+    @status.title.should == "/my/file"
+  end
+
+  [:node, :file, :line, :current_values, :status, :evaluation_time].each do |attr|
     it "should support #{attr}" do
       @status.send(attr.to_s + "=", "foo")
       @status.send(attr).should == "foo"
@@ -38,7 +43,7 @@ describe Puppet::Resource::Status do
     Puppet::Resource::Status.new(@resource).source_description.should == "/my/path"
   end
 
-  [:file, :line, :version].each do |attr|
+  [:file, :line].each do |attr|
     it "should copy the resource's #{attr}" do
       @resource.expects(attr).returns "foo"
       Puppet::Resource::Status.new(@resource).send(attr).should == "foo"
@@ -74,7 +79,7 @@ describe Puppet::Resource::Status do
       @status.send_log :notice, "my message"
     end
 
-    [:file, :line, :version].each do |attr|
+    [:file, :line].each do |attr|
       it "should pass the #{attr}" do
         Puppet::Util::Log.expects(:new).with { |args| args[attr] == "my val" }
         @status.send(attr.to_s + "=", "my val")
@@ -112,20 +117,20 @@ describe Puppet::Resource::Status do
   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
+    @status.changed.should == false
+    @status.out_of_sync.should == 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
+    @status.changed.should == 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
+    @status.out_of_sync.should == false
   end
 
   ['failure', 'noop', 'success'].each do |event_status|
@@ -135,4 +140,14 @@ describe Puppet::Resource::Status do
       @status.out_of_sync.should == true
     end
   end
+
+  describe "When converting to YAML" do
+    it "should include only documented attributes" do
+      @status.file = "/foo.rb"
+      @status.line = 27
+      @status.evaluation_time = 2.7
+      @status.tags = %w{one two}
+      @status.to_yaml_properties.should == Puppet::Resource::Status::YAML_ATTRIBUTES.sort
+    end
+  end
 end
diff --git a/spec/unit/transaction/event_spec.rb b/spec/unit/transaction/event_spec.rb
index 9cf6bc165..6ed14722b 100755
--- a/spec/unit/transaction/event_spec.rb
+++ b/spec/unit/transaction/event_spec.rb
@@ -5,7 +5,7 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/transaction/event'
 
 describe Puppet::Transaction::Event do
-  [:previous_value, :desired_value, :property, :resource, :name, :message, :node, :version, :file, :line, :tags].each do |attr|
+  [:previous_value, :desired_value, :property, :resource, :name, :message, :file, :line, :tags, :audited].each do |attr|
     it "should support #{attr}" do
       event = Puppet::Transaction::Event.new
       event.send(attr.to_s + "=", "foo")
@@ -46,6 +46,12 @@ describe Puppet::Transaction::Event do
     Puppet::Transaction::Event.new.time.should be_instance_of(Time)
   end
 
+  describe "audit property" do
+    it "should default to false" do
+      Puppet::Transaction::Event.new.audited.should == false
+    end
+  end
+
   describe "when sending logs" do
     before do
       Puppet::Util::Log.stubs(:new)
@@ -83,7 +89,7 @@ describe Puppet::Transaction::Event do
       Puppet::Transaction::Event.new(:tags => %w{one two}).send_log
     end
 
-    [:file, :line, :version].each do |attr|
+    [:file, :line].each do |attr|
       it "should pass the #{attr}" do
         Puppet::Util::Log.expects(:new).with { |args| args[attr] == "my val" }
         Puppet::Transaction::Event.new(attr => "my val").send_log
@@ -105,4 +111,17 @@ describe Puppet::Transaction::Event do
       Puppet::Transaction::Event.new(:resource => "Foo[bar]").send_log
     end
   end
+
+  describe "When converting to YAML" do
+    it "should include only documented attributes" do
+      resource = Puppet::Type.type(:file).new(:title => "/tmp/foo")
+      event = Puppet::Transaction::Event.new(:source_description => "/my/param", :resource => resource,
+                                             :file => "/foo.rb", :line => 27, :tags => %w{one two},
+                                             :desired_value => 7, :historical_value => 'Brazil',
+                                             :message => "Help I'm trapped in a spec test",
+                                             :name => :mode_changed, :previous_value => 6, :property => :mode,
+                                             :status => 'success')
+      event.to_yaml_properties.should == Puppet::Transaction::Event::YAML_ATTRIBUTES.sort
+    end
+  end
 end
diff --git a/spec/unit/transaction/report_spec.rb b/spec/unit/transaction/report_spec.rb
index 34c6ecd96..766d4f14d 100755
--- a/spec/unit/transaction/report_spec.rb
+++ b/spec/unit/transaction/report_spec.rb
@@ -101,27 +101,34 @@ describe Puppet::Transaction::Report do
   describe "when computing exit status" do
     it "should produce 2 if changes are present" do
       report = Puppet::Transaction::Report.new("apply")
-      report.add_metric("changes", {:total => 1})
-      report.add_metric("resources", {:failed => 0})
+      report.add_metric("changes", {"total" => 1})
+      report.add_metric("resources", {"failed" => 0})
       report.exit_status.should == 2
     end
 
     it "should produce 4 if failures are present" do
       report = Puppet::Transaction::Report.new("apply")
-      report.add_metric("changes", {:total => 0})
-      report.add_metric("resources", {:failed => 1})
+      report.add_metric("changes", {"total" => 0})
+      report.add_metric("resources", {"failed" => 1})
       report.exit_status.should == 4
     end
 
     it "should produce 6 if both changes and failures are present" do
       report = Puppet::Transaction::Report.new("apply")
-      report.add_metric("changes", {:total => 1})
-      report.add_metric("resources", {:failed => 1})
+      report.add_metric("changes", {"total" => 1})
+      report.add_metric("resources", {"failed" => 1})
       report.exit_status.should == 6
     end
   end
 
-  describe "when calculating metrics" do
+  describe "before finalizing the report" do
+    it "should have a status of 'failed'" do
+      report = Puppet::Transaction::Report.new("apply")
+      report.status.should == 'failed'
+    end
+  end
+
+  describe "when finalizing the report" do
     before do
       @report = Puppet::Transaction::Report.new("apply")
     end
@@ -135,7 +142,7 @@ describe Puppet::Transaction::Report do
     end
 
     def add_statuses(count, type = :file)
-      3.times do |i|
+      count.times do |i|
         status = Puppet::Resource::Status.new(Puppet::Type.type(type).new(:title => "/my/path#{i}"))
         yield status if block_given?
         @report.add_resource_status status
@@ -145,7 +152,7 @@ describe Puppet::Transaction::Report do
 
     [:time, :resources, :changes, :events].each do |type|
       it "should add #{type} metrics" do
-        @report.calculate_metrics
+        @report.finalize_report
         @report.metrics[type.to_s].should be_instance_of(Puppet::Transaction::Metric)
       end
     end
@@ -154,25 +161,38 @@ describe Puppet::Transaction::Report do
       it "should provide the total number of resources" do
         add_statuses(3)
 
-        @report.calculate_metrics
-        metric(:resources, :total).should == 3
+        @report.finalize_report
+        metric(:resources, "total").should == 3
       end
 
       Puppet::Resource::Status::STATES.each do |state|
         it "should provide the number of #{state} resources as determined by the status objects" do
           add_statuses(3) { |status| status.send(state.to_s + "=", true) }
 
-          @report.calculate_metrics
-          metric(:resources, state).should == 3
+          @report.finalize_report
+          metric(:resources, state.to_s).should == 3
         end
       end
+
+      it "should mark the report as 'failed' if there are failing resources" do
+        add_statuses(1) { |status| status.failed = true }
+        @report.finalize_report
+        @report.status.should == 'failed'
+      end
     end
 
     describe "for changes" do
-      it "should provide the number of changes from the resource statuses" do
+      it "should provide the number of changes from the resource statuses and mark the report as 'changed'" do
         add_statuses(3) { |status| 3.times { status << Puppet::Transaction::Event.new(:status => 'success') } }
-        @report.calculate_metrics
-        metric(:changes, :total).should == 9
+        @report.finalize_report
+        metric(:changes, "total").should == 9
+        @report.status.should == 'changed'
+      end
+
+      it "should provide a total even if there are no changes, and mark the report as 'unchanged'" do
+        @report.finalize_report
+        metric(:changes, "total").should == 0
+        @report.status.should == 'unchanged'
       end
     end
 
@@ -188,7 +208,7 @@ describe Puppet::Transaction::Report do
           status.evaluation_time = 3
         end
 
-        @report.calculate_metrics
+        @report.finalize_report
 
         metric(:time, "file").should == 3
         metric(:time, "exec").should == 6
@@ -197,18 +217,32 @@ describe Puppet::Transaction::Report do
 
       it "should add any provided times from external sources" do
         @report.add_times :foobar, 50
-        @report.calculate_metrics
+        @report.finalize_report
         metric(:time, "foobar").should == 50
       end
+
+      it "should have a total time" do
+        add_statuses(3, :file) do |status|
+          status.evaluation_time = 1.25
+        end
+        @report.add_times :config_retrieval, 0.5
+        @report.finalize_report
+        metric(:time, "total").should == 4.25
+      end
     end
 
     describe "for events" do
       it "should provide the total number of events" do
         add_statuses(3) do |status|
-          3.times { |i| status.add_event(Puppet::Transaction::Event.new) }
+          3.times { |i| status.add_event(Puppet::Transaction::Event.new :status => 'success') }
         end
-        @report.calculate_metrics
-        metric(:events, :total).should == 9
+        @report.finalize_report
+        metric(:events, "total").should == 9
+      end
+
+      it "should provide the total even if there are no events" do
+        @report.finalize_report
+        metric(:events, "total").should == 0
       end
 
       Puppet::Transaction::Event::EVENT_STATUSES.each do |status_name|
@@ -221,7 +255,7 @@ describe Puppet::Transaction::Report do
             end
           end
 
-          @report.calculate_metrics
+          @report.finalize_report
           metric(:events, status_name).should == 9
         end
       end
@@ -236,7 +270,7 @@ describe Puppet::Transaction::Report do
       trans = catalog.apply
 
       @report = trans.report
-      @report.calculate_metrics
+      @report.finalize_report
     end
 
     %w{Changes Total Resources}.each do |main|
@@ -245,4 +279,12 @@ describe Puppet::Transaction::Report do
       end
     end
   end
+
+  describe "when outputting yaml" do
+    it "should not include @external_times" do
+      report = Puppet::Transaction::Report.new('apply')
+      report.add_times('config_retrieval', 1.0)
+      report.to_yaml_properties.should_not include('@external_times')
+    end
+  end
 end
diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb
index 387deca61..771c7b4b0 100755
--- a/spec/unit/transaction/resource_harness_spec.rb
+++ b/spec/unit/transaction/resource_harness_spec.rb
@@ -208,11 +208,11 @@ describe Puppet::Transaction::ResourceHarness do
                       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)
+                      status.changed.should == (expected_change_count != 0)
+                      status.out_of_sync.should == (expected_out_of_sync_count != 0)
 
                       # Check the :synced field on state.yml
-                      synced_should_be_set = !noop_mode && status.changed != nil
+                      synced_should_be_set = !noop_mode && status.changed
                       (@harness.cached(resource, :synced) != nil).should == synced_should_be_set
                     end; end
                   end
diff --git a/spec/unit/transaction_spec.rb b/spec/unit/transaction_spec.rb
index 566c90438..862413a31 100755
--- a/spec/unit/transaction_spec.rb
+++ b/spec/unit/transaction_spec.rb
@@ -58,11 +58,6 @@ describe Puppet::Transaction do
     @transaction.report.resource_statuses[resource.to_s].should equal(status)
   end
 
-  it "should calculate metrics on and report the report when asked to generate a report" do
-    @transaction.report.expects(:calculate_metrics)
-    @transaction.generate_report.should equal(@transaction.report)
-  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)
diff --git a/spec/unit/type_spec.rb b/spec/unit/type_spec.rb
index 48b00ec4a..b7a08977e 100755
--- a/spec/unit/type_spec.rb
+++ b/spec/unit/type_spec.rb
@@ -116,7 +116,7 @@ describe Puppet::Type do
     catalog.version = 50
     catalog.add_resource resource
 
-    resource.source_descriptors.should == {:version=>50, :tags=>["mount", "foo"], :path=>"/Mount[foo]"}
+    resource.source_descriptors.should == {:tags=>["mount", "foo"], :path=>"/Mount[foo]"}
   end
 
   it "should consider its type to be the name of its class" do
@@ -153,7 +153,7 @@ describe Puppet::Type do
       @resource.event.default_log_level.should == :warning
     end
 
-    {:file => "/my/file", :line => 50, :tags => %{foo bar}, :version => 50}.each do |attr, value|
+    {:file => "/my/file", :line => 50, :tags => %{foo bar}}.each do |attr, value|
       it "should set the #{attr}" do
         @resource.stubs(attr).returns value
         @resource.event.send(attr).should == value
diff --git a/spec/unit/util/log_spec.rb b/spec/unit/util/log_spec.rb
index 7d96fe190..f3fd1b051 100755
--- a/spec/unit/util/log_spec.rb
+++ b/spec/unit/util/log_spec.rb
@@ -120,7 +120,7 @@ describe Puppet::Util::Log do
       Puppet::Util::Log.new(:level => "notice", :message => :foo, :source => "foo")
     end
 
-    [:file, :line, :version].each do |attr|
+    [:file, :line].each do |attr|
       it "should use #{attr} if provided" do
         Puppet::Util::Log.any_instance.expects(attr.to_s + "=").with "foo"
         Puppet::Util::Log.new(:level => "notice", :message => :foo, attr => "foo")
@@ -177,23 +177,12 @@ describe Puppet::Util::Log do
         log = Puppet::Util::Log.new(:level => "notice", :message => :foo)
         log.expects(:tag).with("tag")
         log.expects(:tag).with("tag2")
-        log.expects(:version=).with(100)
 
         log.source = source
 
         log.source.should == "path"
       end
 
-      it "should copy over any version information" do
-        catalog = Puppet::Resource::Catalog.new
-        catalog.version = 25
-        source = Puppet::Type.type(:file).new :path => "/foo/bar"
-        catalog.add_resource source
-
-        log = Puppet::Util::Log.new(:level => "notice", :message => :foo, :source => source)
-        log.version.should == 25
-      end
-
       it "should copy over any file and line information" do
         source = Puppet::Type.type(:file).new :path => "/foo/bar"
         source.file = "/my/file"
@@ -212,4 +201,22 @@ describe Puppet::Util::Log do
       end
     end
   end
+
+  describe "to_yaml" do
+    it "should not include the @version attribute" do
+      log = Puppet::Util::Log.new(:level => "notice", :message => :foo, :version => 100)
+      log.to_yaml_properties.should_not include('@version')
+    end
+
+    it "should include attributes @level, @message, @source, @tags, and @time" do
+      log = Puppet::Util::Log.new(:level => "notice", :message => :foo, :version => 100)
+      log.to_yaml_properties.should == %w{@level @message @source @tags @time}
+    end
+
+    it "should include attributes @file and @line if specified" do
+      log = Puppet::Util::Log.new(:level => "notice", :message => :foo, :file => "foo", :line => 35)
+      log.to_yaml_properties.should include('@file')
+      log.to_yaml_properties.should include('@line')
+    end
+  end
 end
diff --git a/spec/unit/util/logging_spec.rb b/spec/unit/util/logging_spec.rb
index 46ae5386f..411cd17a9 100755
--- a/spec/unit/util/logging_spec.rb
+++ b/spec/unit/util/logging_spec.rb
@@ -81,7 +81,7 @@ describe Puppet::Util::Logging do
       @logger.notice ["foo", "bar", "baz"]
     end
 
-    [:file, :line, :version, :tags].each do |attr|
+    [:file, :line, :tags].each do |attr|
       it "should include #{attr} if available" do
         @logger.singleton_class.send(:attr_accessor, attr)
 
diff --git a/spec/unit/util/metric_spec.rb b/spec/unit/util/metric_spec.rb
index 72571ee4a..600b88f85 100755
--- a/spec/unit/util/metric_spec.rb
+++ b/spec/unit/util/metric_spec.rb
@@ -59,7 +59,7 @@ describe Puppet::Util::Metric do
   end
 
   it "should support a label for values" do
-    @metric.newvalue(:foo, 10, "label")
+    @metric.newvalue("foo", 10, "label")
     @metric.values[0][1].should == "label"
   end
 
@@ -69,19 +69,19 @@ describe Puppet::Util::Metric do
   end
 
   it "should return its values sorted by label" do
-    @metric.newvalue(:foo, 10, "b")
-    @metric.newvalue(:bar, 10, "a")
+    @metric.newvalue("foo", 10, "b")
+    @metric.newvalue("bar", 10, "a")
 
-    @metric.values.should == [[:bar, "a", 10], [:foo, "b", 10]]
+    @metric.values.should == [["bar", "a", 10], ["foo", "b", 10]]
   end
 
   it "should use an array indexer method to retrieve individual values" do
-    @metric.newvalue(:foo, 10)
-    @metric[:foo].should == 10
+    @metric.newvalue("foo", 10)
+    @metric["foo"].should == 10
   end
 
   it "should return nil if the named value cannot be found" do
-    @metric[:foo].should == 0
+    @metric["foo"].should == 0
   end
 
   # LAK: I'm not taking the time to develop these tests right now.