Fixing #1054 - transaction reports are always sent

This refactors how reports, catalogs, configurers, and transactions
are all related - the Configurer class manages the report, both
creating and sending it, so the transaction is now just responsible
for adding data to it.  I'm still a bit uncomfortable of the coupling
between transactions, the report, and configurer, but it's better than
it was.

This also fixes #2944 and #2973.

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

3 files changed, 189 insertions, 23 deletions

diff --git a/spec/unit/configurer.rb b/spec/unit/configurer.rb
index cd5102546..8a3577c59 100755
--- a/spec/unit/configurer.rb
+++ b/spec/unit/configurer.rb
@@ -21,12 +21,23 @@ describe Puppet::Configurer do
     end
 end
 
+describe Puppet::Configurer, "when initializing a report" do
+    it "should return an instance of a transaction report" do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Configurer.new
+        @agent.initialize_report.should be_instance_of(Puppet::Transaction::Report)
+    end
+end
+
 describe Puppet::Configurer, "when executing a catalog run" do
     before do
         Puppet.settings.stubs(:use).returns(true)
         @agent = Puppet::Configurer.new
         @agent.stubs(:facts_for_uploading).returns({})
         @agent.stubs(:retrieve_catalog).returns Puppet::Resource::Catalog.new
+
+        Puppet::Util::Log.stubs(:newdestination)
+        Puppet::Util::Log.stubs(:close)
     end
 
     it "should prepare for the run" do
@@ -35,6 +46,22 @@ describe Puppet::Configurer, "when executing a catalog run" do
         @agent.run
     end
 
+    it "should initialize a transaction report" do
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+
+        @agent.run
+    end
+
+    it "should set the report as a log destination" do
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+
+        Puppet::Util::Log.expects(:newdestination).with(report)
+
+        @agent.run
+    end
+
     it "should retrieve the catalog" do
         @agent.expects(:retrieve_catalog)
 
@@ -53,7 +80,7 @@ describe Puppet::Configurer, "when executing a catalog run" do
         catalog = stub 'catalog', :retrieval_duration= => nil
         @agent.expects(:retrieve_catalog).returns catalog
 
-        catalog.expects(:apply).with(:one => true)
+        catalog.expects(:apply).with { |args| args[:one] == true }
         @agent.run :one => true
     end
 
@@ -61,7 +88,7 @@ describe Puppet::Configurer, "when executing a catalog run" do
         catalog = stub 'catalog', :retrieval_duration= => nil
         @agent.expects(:retrieve_catalog).never
 
-        catalog.expects(:apply).with(:one => true)
+        catalog.expects(:apply)
         @agent.run :one => true, :catalog => catalog
     end
 
@@ -74,6 +101,126 @@ describe Puppet::Configurer, "when executing a catalog run" do
         catalog.expects(:apply).never # because we're not yielding
         @agent.run
     end
+
+    it "should send the report" do
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+        @agent.expects(:send_report).with { |r, trans| r == report }
+
+        @agent.run
+    end
+
+    it "should send the transaction report with a reference to the transaction if a run was actually made" do
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+
+        catalog = stub 'catalog', :retrieval_duration= => nil
+
+        trans = stub 'transaction'
+        catalog.expects(:apply).returns trans
+
+        @agent.expects(:send_report).with { |r, t| t == trans }
+
+        @agent.run :catalog => catalog
+    end
+
+    it "should send the transaction report even if the catalog could not be retrieved" do
+        @agent.expects(:retrieve_catalog).returns nil
+
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+        @agent.expects(:send_report)
+
+        @agent.run
+    end
+
+    it "should send the transaction report even if there is a failure" do
+        @agent.expects(:retrieve_catalog).raises "whatever"
+
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+        @agent.expects(:send_report)
+
+        lambda { @agent.run }.should raise_error
+    end
+
+    it "should remove the report as a log destination when the run is finished" do
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+
+        Puppet::Util::Log.expects(:close).with(report)
+
+        @agent.run
+    end
+
+    it "should return the report as the result of the run" do
+        report = stub 'report'
+        @agent.expects(:initialize_report).returns report
+
+        @agent.run.should equal(report)
+    end
+end
+
+describe Puppet::Configurer, "when sending a report" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @configurer = Puppet::Configurer.new
+
+        @report = stub 'report'
+        @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(:add_metrics_to_report).with(@report)
+        @configurer.send_report(@report, @trans)
+    end
+
+    it "should print a report summary if configured to do so" do
+        Puppet.settings[:summarize] = true
+
+        @report.expects(:summary).returns "stuff"
+
+        @configurer.expects(:puts).with("stuff")
+        @configurer.send_report(@report)
+    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)
+    end
+
+    it "should save the report if reporting is enabled" do
+        Puppet.settings[:report] = true
+
+        @report.expects(:save)
+        @configurer.send_report(@report)
+    end
+
+    it "should not save the report if reporting is disabled" do
+        Puppet.settings[:report] = false
+
+        @report.expects(:save).never
+        @configurer.send_report(@report)
+    end
+
+    it "should log but not fail if saving the report fails" do
+        Puppet.settings[:report] = true
+
+        @report.expects(:save).raises "whatever"
+
+        Puppet.expects(:err)
+        lambda { @configurer.send_report(@report) }.should_not raise_error
+    end
 end
 
 describe Puppet::Configurer, "when retrieving a catalog" do
diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb
index db672438d..d74b9a388 100755
--- a/spec/unit/resource/catalog.rb
+++ b/spec/unit/resource/catalog.rb
@@ -574,7 +574,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
         before :each do
             @catalog = Puppet::Resource::Catalog.new("host")
 
-            @catalog.retrieval_duration = Time.now
             @transaction = mock 'transaction'
             Puppet::Transaction.stubs(:new).returns(@transaction)
             @transaction.stubs(:evaluate)
@@ -587,11 +586,15 @@ describe Puppet::Resource::Catalog, "when compiling" do
             @catalog.apply
         end
 
-        it "should provide the catalog time to the transaction" do
-            @transaction.expects(:addtimes).with do |arg|
-                arg[:config_retrieval].should be_instance_of(Time)
-                true
-            end
+        it "should provide the catalog retrieval time to the transaction" do
+            @catalog.retrieval_duration = 5
+            @transaction.expects(:addtimes).with(:config_retrieval => 5)
+            @catalog.apply
+        end
+
+        it "should use a retrieval time of 0 if none is set in the catalog" do
+            @catalog.retrieval_duration = nil
+            @transaction.expects(:addtimes).with(:config_retrieval => 0)
             @catalog.apply
         end
 
@@ -668,20 +671,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
                 Puppet::Util::Storage.stubs(:store)
             end
 
-            it "should send a report if reporting is enabled" do
-                Puppet[:report] = true
-                @transaction.expects :send_report
-                @transaction.stubs :any_failed? => false
-                @catalog.apply
-            end
-
-            it "should send a report if report summaries are enabled" do
-                Puppet[:summarize] = true
-                @transaction.expects :send_report
-                @transaction.stubs :any_failed? => false
-                @catalog.apply
-            end
-
             it "should initialize the state database before applying a catalog" do
                 Puppet::Util::Storage.expects(:load)
 
@@ -708,7 +697,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
             it "should never send reports" do
                 Puppet[:report] = true
                 Puppet[:summarize] = true
-                @transaction.expects(:send_report).never
                 @catalog.apply
             end
 
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 1b3562153..260ed3703 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -18,6 +18,15 @@ describe Puppet::Transaction do
         @transaction.prefetch
     end
 
+    describe "when initializing" do
+        it "should accept a catalog and set an instance variable for it" do
+            catalog = stub 'catalog', :vertices => []
+
+            trans = Puppet::Transaction.new(catalog)
+            trans.catalog.should == catalog
+        end
+    end
+
     describe "when generating resources" do
         it "should finish all resources" do
             generator = stub 'generator', :depthfirst? => true, :tags => []
@@ -105,6 +114,28 @@ describe Puppet::Transaction do
             @transaction.skip?(@resource).should be_true
         end
     end
+
+    describe "when adding metrics to a report" do
+        before do
+            @catalog = Puppet::Resource::Catalog.new
+            @transaction = Puppet::Transaction.new(@catalog)
+
+            @report = stub 'report', :newmetric => nil, :time= => nil
+        end
+
+        [:resources, :time, :changes].each do |metric|
+            it "should add times for '#{metric}'" do
+                @report.expects(:newmetric).with { |m, v| m == metric }
+                @transaction.add_metrics_to_report(@report)
+            end
+        end
+
+        it "should set the transaction time to the current time" do
+            Time.expects(:now).returns "now"
+            @report.expects(:time=).with("now")
+            @transaction.add_metrics_to_report(@report)
+        end
+    end
 end
 
 describe Puppet::Transaction, " when determining tags" do