Merge pull request #5006 from chef/adamleff/fix-updated-resource-counter

Bug fix: updated_resource_count should only include updated resources

4 files changed, 31 insertions, 59 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index e852d11ab6..c8dd354f60 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -51,13 +51,12 @@ class Chef
     # and exports its data through a webhook-like mechanism to a configured
     # endpoint.
     class Reporter < EventDispatch::Base
-      attr_reader :updated_resources, :status, :exception, :error_descriptions,
-                  :expanded_run_list, :run_status, :http, :resource_count,
+      attr_reader :completed_resources, :status, :exception, :error_descriptions,
+                  :expanded_run_list, :run_status, :http,
                   :current_resource_report, :enabled
 
       def initialize
-        @updated_resources       = []
-        @resource_count          = 0
+        @completed_resources     = []
         @current_resource_loaded = nil
         @error_descriptions      = {}
         @expanded_run_list       = {}
@@ -111,20 +110,16 @@ class Chef
       end
 
       # see EventDispatch::Base#resource_up_to_date
-      # Mark our ResourceReport status accordingly, and increment the total
-      # resource count.
+      # Mark our ResourceReport status accordingly
       def resource_up_to_date(new_resource, action)
         current_resource_report.up_to_date unless nested_resource?(new_resource)
-        increment_resource_count
       end
 
       # see EventDispatch::Base#resource_skipped
-      # Increment the total resource count. If this is a top-level resource,
-      # we also create a ResourceReport instance (because a skipped resource
-      # does not trigger the resource_current_state_loaded event), and flag
-      # it as skipped.
+      # If this is a top-level resource, we create a ResourceReport instance
+      # (because a skipped resource does not trigger the
+      # resource_current_state_loaded event), and flag it as skipped.
       def resource_skipped(new_resource, action, conditional)
-        increment_resource_count
         return if nested_resource?(new_resource)
 
         update_current_resource_report(
@@ -138,19 +133,17 @@ class Chef
 
       # see EventDispatch::Base#resource_updated
       # Flag the current ResourceReport instance as updated (as long as it's
-      # a top-level resource) and increment the total resource count.
+      # a top-level resource).
       def resource_updated(new_resource, action)
         current_resource_report.updated unless nested_resource?(new_resource)
-        increment_resource_count
       end
 
       # see EventDispatch::Base#resource_failed
       # Flag the current ResourceReport as failed and supply the exception as
-      # long as it's a top-level resource, increment the total resource count,
-      # and update the run error text with the proper Formatter.
+      # long as it's a top-level resource, and update the run error text
+      # with the proper Formatter.
       def resource_failed(new_resource, action, exception)
         current_resource_report.failed(exception) unless nested_resource?(new_resource)
-        increment_resource_count
         update_error_description(
           Formatters::ErrorMapper.resource_failed(
             new_resource,
@@ -167,7 +160,7 @@ class Chef
       def resource_completed(new_resource)
         if current_resource_report && !nested_resource?(new_resource)
           current_resource_report.finish
-          add_updated_resource(current_resource_report)
+          add_completed_resource(current_resource_report)
           update_current_resource_report(nil)
         end
       end
@@ -278,8 +271,7 @@ class Chef
           Chef::DataCollector::Messages.run_end_message(
             run_status: run_status,
             expanded_run_list: expanded_run_list,
-            total_resource_count: resource_count,
-            updated_resources: updated_resources,
+            completed_resources: completed_resources,
             status: opts[:status],
             error_descriptions: error_descriptions
           ).to_json
@@ -305,12 +297,8 @@ class Chef
         Chef::Config[:data_collector][:token]
       end
 
-      def increment_resource_count
-        @resource_count += 1
-      end
-
-      def add_updated_resource(resource_report)
-        @updated_resources << resource_report
+      def add_completed_resource(resource_report)
+        @completed_resources << resource_report
       end
 
       def disable_data_collector_reporter
diff --git a/lib/chef/data_collector/messages.rb b/lib/chef/data_collector/messages.rb
index b6114a8bec..e23262c9e4 100644
--- a/lib/chef/data_collector/messages.rb
+++ b/lib/chef/data_collector/messages.rb
@@ -70,15 +70,15 @@ class Chef
           "message_type"           => "run_converge",
           "node_name"              => run_status.node.name,
           "organization_name"      => organization,
-          "resources"              => reporter_data[:updated_resources].map(&:for_json),
+          "resources"              => reporter_data[:completed_resources].map(&:for_json),
           "run_id"                 => run_status.run_id,
           "run_list"               => run_status.node.run_list.for_json,
           "start_time"             => run_status.start_time.utc.iso8601,
           "end_time"               => run_status.end_time.utc.iso8601,
           "source"                 => collector_source,
           "status"                 => reporter_data[:status],
-          "total_resource_count"   => reporter_data[:total_resource_count],
-          "updated_resource_count" => reporter_data[:updated_resources].count,
+          "total_resource_count"   => reporter_data[:completed_resources].count,
+          "updated_resource_count" => reporter_data[:completed_resources].select { |r| r.status == "updated" }.count,
         }
 
         message["error"] = {
diff --git a/spec/unit/data_collector/messages_spec.rb b/spec/unit/data_collector/messages_spec.rb
index aacca6444d..dee86a3907 100644
--- a/spec/unit/data_collector/messages_spec.rb
+++ b/spec/unit/data_collector/messages_spec.rb
@@ -61,12 +61,13 @@ describe Chef::DataCollector::Messages do
   end
 
   describe '#run_end_message' do
-    let(:run_status)    { Chef::RunStatus.new(Chef::Node.new, Chef::EventDispatch::Dispatcher.new) }
-    let(:resource)      { double("resource", for_json: "resource_data") }
+    let(:run_status) { Chef::RunStatus.new(Chef::Node.new, Chef::EventDispatch::Dispatcher.new) }
+    let(:resource1)  { double("resource1", for_json: "resource_data", status: "updated") }
+    let(:resource2)  { double("resource2", for_json: "resource_data", status: "skipped") }
     let(:reporter_data) do
       {
         run_status: run_status,
-        updated_resources: [resource],
+        completed_resources: [resource1, resource2],
       }
     end
 
@@ -116,6 +117,12 @@ describe Chef::DataCollector::Messages do
         end
         expect(extra_fields).to eq([])
       end
+
+      it "only includes updated resources in its count" do
+        message = Chef::DataCollector::Messages.run_end_message(reporter_data)
+        expect(message["total_resource_count"]).to eq(2)
+        expect(message["updated_resource_count"]).to eq(1)
+      end
     end
 
     context "when the run was not successful" do
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index 6065cbc8b1..78b0aaf97d 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -226,17 +226,11 @@ describe Chef::DataCollector::Reporter do
     let(:resource_report) { double("resource_report") }
 
     before do
-      allow(reporter).to receive(:increment_resource_count)
       allow(reporter).to receive(:nested_resource?)
       allow(reporter).to receive(:current_resource_report).and_return(resource_report)
       allow(resource_report).to receive(:up_to_date)
     end
 
-    it "increments the resource count" do
-      expect(reporter).to receive(:increment_resource_count)
-      reporter.resource_up_to_date(new_resource, action)
-    end
-
     context "when the resource is a nested resource" do
       it "does not mark the resource report as up-to-date" do
         allow(reporter).to receive(:nested_resource?).with(new_resource).and_return(true)
@@ -261,17 +255,11 @@ describe Chef::DataCollector::Reporter do
     let(:resource_report) { double("resource_report") }
 
     before do
-      allow(reporter).to receive(:increment_resource_count)
       allow(reporter).to receive(:nested_resource?)
       allow(reporter).to receive(:current_resource_report).and_return(resource_report)
       allow(resource_report).to receive(:skipped)
     end
 
-    it "increments the resource count" do
-      expect(reporter).to receive(:increment_resource_count)
-      reporter.resource_skipped(new_resource, action, conditional)
-    end
-
     context "when the resource is a nested resource" do
       it "does not mark the resource report as skipped" do
         allow(reporter).to receive(:nested_resource?).with(new_resource).and_return(true)
@@ -307,11 +295,6 @@ describe Chef::DataCollector::Reporter do
       allow(resource_report).to receive(:updated)
     end
 
-    it "increments the resource count" do
-      expect(reporter).to receive(:increment_resource_count)
-      reporter.resource_updated("new_resource", "action")
-    end
-
     it "marks the resource report as updated" do
       expect(resource_report).to receive(:updated)
       reporter.resource_updated("new_resource", "action")
@@ -326,7 +309,6 @@ describe Chef::DataCollector::Reporter do
     let(:resource_report) { double("resource_report") }
 
     before do
-      allow(reporter).to receive(:increment_resource_count)
       allow(reporter).to receive(:update_error_description)
       allow(reporter).to receive(:current_resource_report).and_return(resource_report)
       allow(resource_report).to receive(:failed)
@@ -334,11 +316,6 @@ describe Chef::DataCollector::Reporter do
       allow(error_mapper).to receive(:for_json)
     end
 
-    it "increments the resource count" do
-      expect(reporter).to receive(:increment_resource_count)
-      reporter.resource_failed(new_resource, action, exception)
-    end
-
     it "updates the error description" do
       expect(Chef::Formatters::ErrorMapper).to receive(:resource_failed).with(
         new_resource,
@@ -372,7 +349,7 @@ describe Chef::DataCollector::Reporter do
     let(:resource_report) { double("resource_report") }
 
     before do
-      allow(reporter).to receive(:add_updated_resource)
+      allow(reporter).to receive(:add_completed_resource)
       allow(reporter).to receive(:update_current_resource_report)
       allow(resource_report).to receive(:finish)
     end
@@ -380,7 +357,7 @@ describe Chef::DataCollector::Reporter do
     context "when there is no current resource report" do
       it "does not add the updated resource" do
         allow(reporter).to receive(:current_resource_report).and_return(nil)
-        expect(reporter).not_to receive(:add_updated_resource)
+        expect(reporter).not_to receive(:add_completed_resource)
         reporter.resource_completed(new_resource)
       end
     end
@@ -393,7 +370,7 @@ describe Chef::DataCollector::Reporter do
       context "when the resource is a nested resource" do
         it "does not add the updated resource" do
           allow(reporter).to receive(:nested_resource?).with(new_resource).and_return(true)
-          expect(reporter).not_to receive(:add_updated_resource)
+          expect(reporter).not_to receive(:add_completed_resource)
           reporter.resource_completed(new_resource)
         end
       end
@@ -409,7 +386,7 @@ describe Chef::DataCollector::Reporter do
         end
 
         it "adds the resource to the updated resource list" do
-          expect(reporter).to receive(:add_updated_resource).with(resource_report)
+          expect(reporter).to receive(:add_completed_resource).with(resource_report)
           reporter.resource_completed(new_resource)
         end