Bug fix: updated_resource_count should only include updated resources

After adding up-to-date and skipped resources to the DataCollector
resource list, the logic that computed the number of resources
updated needed to change accordingly. This change fixes that
bug and also eliminates an unnecessary counter that tracks
the total number of 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