Calculate diffs for config errors

Author: adangel

lib/pmdtester/parsers/pmd_report_document.rb

@@ -13,7 +13,7 @@ class PmdReportDocument < Nokogiri::XML::SAX::Document
     def initialize(branch_name, working_dir, filter_set = nil)
       @violations = CollectionByFile.new
       @errors = CollectionByFile.new
-      @configerrors = {}
+      @configerrors = Hash.new { |hash, key| hash[key] = [] }
 
       @infos_by_rules = {}
       @current_violations = []
@@ -32,35 +32,16 @@ def start_element(name, attrs = [])
 
       case name
       when 'file'
-        @current_filename = remove_work_dir!(attrs['name'])
-        @current_violations = []
+        handle_start_file attrs
       when 'violation'
-        @current_violation = PmdViolation.new(
-          branch: @branch_name,
-          fname: @current_filename,
-          info_url: attrs['externalInfoUrl'],
-          bline: attrs['beginline'].to_i,
-          rule_name: attrs['rule'],
-          ruleset_name: attrs['ruleset'].freeze
-        )
+        handle_start_violation attrs
       when 'error'
-        @current_filename = remove_work_dir!(attrs['filename'])
-
-        @current_error = PmdError.new(
-          branch: @branch_name,
-          filename: @current_filename,
-          short_message: remove_work_dir!(attrs['msg'])
-        )
+        handle_start_error attrs
+      when 'configerror'
+        handle_start_configerror attrs
       end
     end
 
-    # Modifies the string in place and returns it
-    # (this is what sub! does, except it returns nil if no replacement occurred)
-    def remove_work_dir!(str)
-      str.sub!(/#{@working_dir}/, '')
-      str
-    end
-
     def characters(string)
       @cur_text << remove_work_dir!(string)
     end
@@ -85,10 +66,22 @@ def end_element(name)
         @errors.add_all(@current_filename, [@current_error])
         @current_filename = nil
         @current_error = nil
+      when 'configerror'
+        @configerrors[@current_configerror.rulename].push(@current_configerror)
+        @current_configerror = nil
       end
       @cur_text.clear
     end
 
+    private
+
+    # Modifies the string in place and returns it
+    # (this is what sub! does, except it returns nil if no replacement occurred)
+    def remove_work_dir!(str)
+      str.sub!(/#{@working_dir}/, '')
+      str
+    end
+
     def finish_text!
       res = @cur_text.strip!.dup.freeze
       @cur_text.clear
@@ -105,5 +98,35 @@ def match_filter_set?(violation)
 
       @filter_set.include?(rule_ref)
     end
+
+    def handle_start_file(attrs)
+      @current_filename = remove_work_dir!(attrs['name'])
+      @current_violations = []
+    end
+
+    def handle_start_violation(attrs)
+      @current_violation = PmdViolation.new(
+        branch: @branch_name,
+        fname: @current_filename,
+        info_url: attrs['externalInfoUrl'],
+        bline: attrs['beginline'].to_i,
+        rule_name: attrs['rule'],
+        ruleset_name: attrs['ruleset'].freeze
+      )
+    end
+
+    def handle_start_error(attrs)
+      @current_filename = remove_work_dir!(attrs['filename'])
+
+      @current_error = PmdError.new(
+        branch: @branch_name,
+        filename: @current_filename,
+        short_message: remove_work_dir!(attrs['msg'])
+      )
+    end
+
+    def handle_start_configerror(attrs)
+      @current_configerror = PmdConfigError.new(attrs, @branch_name)
+    end
   end
 end

lib/pmdtester/pmd_configerror.rb

@@ -13,13 +13,13 @@ class PmdConfigError
     #   <xs:attribute name="msg" type="xs:string" use="required" />
     # </xs:complexType>
     attr_reader :attrs
-    attr_accessor :text
+    attr_accessor :old_error
 
     def initialize(attrs, branch)
       @attrs = attrs
 
+      @changed = false
       @branch = branch
-      @text = ''
     end
 
     def rulename
@@ -30,14 +30,31 @@ def msg
       @attrs['msg']
     end
 
+    def sort_key
+      rulename
+    end
+
     def changed?
-      false
+      @changed
     end
 
     def eql?(other)
       rulename.eql?(other.rulename) && msg.eql?(other.msg)
     end
 
+    def try_merge?(other)
+      if branch != BASE &&
+         branch != other.branch &&
+         rulename == other.rulename &&
+         !changed? # not already changed
+        @changed = true
+        @old_error = other
+        true
+      end
+
+      false
+    end
+
     def hash
       [rulename, msg].hash
     end

lib/pmdtester/pmd_tester_utils.rb

@@ -28,9 +28,7 @@ def parse_pmd_report(report_file, branch, report_details, filter_set = nil)
       parser = Nokogiri::XML::SAX::Parser.new(doc)
       parser.parse_file(report_file) if File.exist?(report_file)
       Report.new(
-        violations_by_file: doc.violations,
-        errors_by_file: doc.errors,
-        infos_by_rule: doc.infos_by_rules,
+        report_document: doc,
 
         timestamp: report_details.timestamp,
         exec_time: report_details.execution_time

lib/pmdtester/report_diff.rb

@@ -50,30 +50,38 @@ def initialize(name, info_url)
   class Report
     attr_reader :violations_by_file,
                 :errors_by_file,
+                :configerrors_by_rule,
                 :exec_time,
                 :timestamp,
                 :infos_by_rule
 
-    def initialize(violations_by_file:,
-                   errors_by_file:,
-                   exec_time:,
-                   timestamp:,
-                   infos_by_rule:)
-      @violations_by_file = violations_by_file
-      @errors_by_file = errors_by_file
+    def initialize(report_document: nil,
+                   exec_time: 0,
+                   timestamp: '0')
+      initialize_empty
+      initialize_with_report_document report_document unless report_document.nil?
       @exec_time = exec_time
       @timestamp = timestamp
-      @infos_by_rule = infos_by_rule
     end
 
     def self.empty
-      new(
-        violations_by_file: CollectionByFile.new,
-        errors_by_file: CollectionByFile.new,
-        exec_time: 0,
-        timestamp: '0',
-        infos_by_rule: {}
-      )
+      new
+    end
+
+    private
+
+    def initialize_with_report_document(report_document)
+      @violations_by_file = report_document.violations
+      @errors_by_file = report_document.errors
+      @configerrors_by_rule = report_document.configerrors
+      @infos_by_rule = report_document.infos_by_rules
+    end
+
+    def initialize_empty
+      @violations_by_file = CollectionByFile.new
+      @errors_by_file = CollectionByFile.new
+      @configerrors_by_rule = {}
+      @infos_by_rule = {}
     end
   end
 
@@ -85,9 +93,11 @@ class ReportDiff
 
     attr_reader :error_counts
     attr_reader :violation_counts
+    attr_reader :configerror_counts
 
     attr_accessor :violation_diffs_by_file
     attr_accessor :error_diffs_by_file
+    attr_accessor :configerror_diffs_by_rule
 
     attr_accessor :rule_infos_union
     attr_accessor :base_report
@@ -96,15 +106,17 @@ class ReportDiff
     def initialize(base_report:, patch_report:)
       @violation_counts = RunningDiffCounters.new(base_report.violations_by_file.total_size)
       @error_counts = RunningDiffCounters.new(base_report.errors_by_file.total_size)
-      @violation_diffs_by_rule = {}
-
-      @base_report = base_report
-      @patch_report = patch_report
+      @configerror_counts = RunningDiffCounters.new(base_report.configerrors_by_rule.values.flatten.length)
 
-      @rule_infos_union = base_report.infos_by_rule.dup
       @violation_diffs_by_file = {}
       @error_diffs_by_file = {}
+      @configerror_diffs_by_rule = {}
+
+      @rule_infos_union = base_report.infos_by_rule.dup
+      @base_report = base_report
+      @patch_report = patch_report
 
+      @violation_diffs_by_rule = {}
       diff_with(patch_report)
     end
 
@@ -123,11 +135,13 @@ def rule_summaries
     def diff_with(patch_report)
       @violation_counts.patch_total = patch_report.violations_by_file.total_size
       @error_counts.patch_total = patch_report.errors_by_file.total_size
+      @configerror_counts.patch_total = patch_report.configerrors_by_rule.values.flatten.length
 
       @violation_diffs_by_file = build_diffs(@violation_counts, &:violations_by_file)
       count(@violation_diffs_by_file) { |v| getvdiff(v.rule_name) } # record the diffs in the rule counter
 
       @error_diffs_by_file = build_diffs(@error_counts, &:errors_by_file)
+      @configerror_diffs_by_rule = build_diffs(@configerror_counts, &:configerrors_by_rule)
 
       count_by_rule(@base_report.violations_by_file, base: true)
       count_by_rule(@patch_report.violations_by_file, base: false)
@@ -178,6 +192,7 @@ def build_diffs(counters, &getter)
     end
 
     # @param diff_h a hash { filename => list[violation]}, containing those that changed
+    # in case of config errors it's a hash { rulename => list[configerror] }
     def merge_changed_items(diff_h)
       diff_h.each do |fname, different|
         different.sort_by!(&:sort_key)

lib/pmdtester/runner.rb

@@ -126,21 +126,23 @@ def get_projects(file_path)
     def summarize_diffs
       error_total = RunningDiffCounters.new(0)
       violations_total = RunningDiffCounters.new(0)
+      configerrors_total = RunningDiffCounters.new(0)
 
       @projects.each do |project|
         diff = project.report_diff
 
         # in case we are in single mode, there might be no diffs (only the patch branch is available)
-        unless diff.nil?
-          error_total.merge!(diff.error_counts)
-          violations_total.merge!(diff.violation_counts)
-        end
+        next if diff.nil?
+
+        error_total.merge!(diff.error_counts)
+        violations_total.merge!(diff.violation_counts)
+        configerrors_total.merge!(diff.configerror_counts)
       end
 
       {
         errors: error_total.to_h,
         violations: violations_total.to_h,
-        configerrors: RunningDiffCounters.new(0).to_h # note: this is now ignored
+        configerrors: configerrors_total.to_h
       }
     end
 

test/test_diff_builder.rb

@@ -59,6 +59,26 @@ def test_error_diffs
     assert_equal(1, error_diffs['Patch1.java'].size)
   end
 
+  def test_configerrors_diffs
+    base_report_path = 'test/resources/diff_builder/test_configerrors_diffs_base.xml'
+    patch_report_path = 'test/resources/diff_builder/test_configerrors_diffs_patch.xml'
+    diffs_report = build_report_diff(base_report_path, patch_report_path,
+                                     BASE_REPORT_INFO_PATH, PATCH_REPORT_INFO_PATH)
+    configerrors_diffs = diffs_report.configerror_diffs_by_rule
+    keys = configerrors_diffs.keys
+
+    assert_counters_empty(diffs_report.violation_counts)
+    assert_counters_empty(diffs_report.error_counts)
+    assert_counters_eq(diffs_report.configerror_counts,
+                       base_total: 4, patch_total: 3, changed_total: 5)
+    assert_changes_eq(diffs_report.configerror_counts,
+                      removed: 3, added: 1, changed: 1)
+
+    assert_equal(%w[RuleBase1 RuleBoth2 RulePatch1], keys)
+    assert_equal(2, configerrors_diffs['RuleBase1'].size)
+    assert_equal(2, configerrors_diffs['RuleBoth2'].size)
+  end
+
   private
 
   def assert_counters_empty(counters)