Report issue for each occurrence of duplication

This change fixes a bug in which we reported only the first occurrence
(or sexp) of duplicated code in a set, and dropped the rest. The others
were included in other_locations but not reported as an independent issue.

Author: ABaldwinHunter

lib/cc/engine/analyzers/reporter.rb

@@ -1,4 +1,4 @@
-require 'cc/engine/analyzers/violation'
+require 'cc/engine/analyzers/violations'
 require 'cc/engine/analyzers/file_thread_pool'
 require 'thread'
 
@@ -36,11 +36,13 @@ def process_files
 
         def report
           flay.report(StringIO.new).each do |issue|
-            violation = new_violation(issue)
+            violations = new_violations(issue)
 
-            unless reports.include?(violation.report_name)
-              reports.add(violation.report_name)
-              io.puts "#{violation.format.to_json}\0"
+            violations.each do |violation|
+              unless reports.include?(violation.report_name)
+                reports.add(violation.report_name)
+                io.puts "#{violation.format.to_json}\0"
+              end
             end
           end
         end
@@ -60,9 +62,9 @@ def flay
 
         attr_reader :engine_config, :language_strategy, :io
 
-        def new_violation(issue)
+        def new_violations(issue)
           hashes = flay.hashes[issue.structural_hash]
-          Violation.new(language_strategy, issue, hashes)
+          Violations.new(language_strategy, issue, hashes)
         end
 
         def flay_options

lib/cc/engine/analyzers/violation.rb

@@ -8,10 +8,11 @@ module Analyzers
       class Violation
         attr_reader :issue
 
-        def initialize(language_strategy, issue, hashes)
+        def initialize(language_strategy:, issue:, current_sexp:, other_sexps:)
           @language_strategy = language_strategy
           @issue = issue
-          @hashes = hashes
+          @current_sexp = current_sexp
+          @other_sexps = other_sexps
         end
 
         def format
@@ -34,19 +35,7 @@ def report_name
 
         private
 
-        attr_reader :language_strategy, :hashes
-
-        def current_sexp
-          @location ||= sorted_hashes.first
-        end
-
-        def sorted_hashes
-          @_sorted_hashes ||= hashes.sort_by(&:file)
-        end
-
-        def other_sexps
-          @other_locations ||= sorted_hashes.drop(1)
-        end
+        attr_reader :language_strategy, :other_sexps, :current_sexp
 
         def check_name
           if issue.identical?

lib/cc/engine/analyzers/violations.rb

@@ -0,0 +1,35 @@
+require "cc/engine/analyzers/violation"
+
+module CC
+  module Engine
+    module Analyzers
+      class Violations
+        def initialize(language_strategy, issue, hashes)
+          @language_strategy = language_strategy
+          @issue = issue
+          @hashes = hashes
+        end
+
+        def each
+          hashes.each_with_index do |sexp, i|
+            yield Violation.new(
+              current_sexp: sexp,
+              other_sexps: other_sexps(hashes.dup, i),
+              issue: issue,
+              language_strategy: language_strategy,
+            )
+          end
+        end
+
+        private
+
+        attr_reader :language_strategy, :issue, :hashes
+
+        def other_sexps(members, i)
+          members.delete_at(i)
+          members.sort_by(&:file)
+        end
+      end
+    end
+  end
+end

spec/cc/engine/analyzers/javascript/main_spec.rb

@@ -16,7 +16,8 @@
           console.log("hello JS!");
       EOJS
 
-      result = run_engine(engine_conf).strip
+      issues = run_engine(engine_conf).strip.split("\0")
+      result = issues.first.strip
       json = JSON.parse(result)
 
       expect(json["type"]).to eq("issue")
@@ -43,7 +44,8 @@
           console.log("helllllllllllllllllo JS!");
       EOJS
 
-      result = run_engine(engine_conf).strip
+      issues = run_engine(engine_conf).strip.split("\0")
+      result = issues.first.strip
       json = JSON.parse(result)
 
       expect(json["type"]).to eq("issue")
@@ -93,6 +95,7 @@
 
     result = run_engine(engine_conf).strip
     issues = result.split("\0")
+
     expect(issues.length).to eq 1
   end
 

spec/cc/engine/analyzers/php/main_spec.rb

@@ -29,7 +29,8 @@
           }
       EOPHP
 
-      result = run_engine(engine_conf).strip
+      issues = run_engine(engine_conf).strip.split("\0")
+      result = issues.first.strip
       json = JSON.parse(result)
 
       expect(json["type"]).to eq("issue")
@@ -50,7 +51,8 @@
 
     it "runs against complex files" do
       FileUtils.cp(fixture_path("symfony_configuration.php"), File.join(@code, "configuration.php"))
-      result = run_engine(engine_conf).strip
+      issues = run_engine(engine_conf).strip.split("\0")
+      result = issues.first.strip
 
       expect(result).to match "\"type\":\"issue\""
     end

spec/cc/engine/analyzers/python/main_spec.rb

@@ -16,7 +16,8 @@
 print("Hello", "python")
       EOJS
 
-      result = run_engine(engine_conf).strip
+      issues = run_engine(engine_conf).strip.split("\0")
+      result = issues.first.strip
       json = JSON.parse(result)
 
       expect(json["type"]).to eq("issue")
@@ -43,7 +44,8 @@
 print("Hello from the other side", "python")
       EOJS
 
-      result = run_engine(engine_conf).strip
+      issues = run_engine(engine_conf).strip.split("\0")
+      result = issues.first.strip
       json = JSON.parse(result)
 
       expect(json["type"]).to eq("issue")

spec/cc/engine/analyzers/ruby/main_spec.rb

@@ -28,7 +28,8 @@ module CC::Engine::Analyzers
             end
         EORUBY
 
-        result = run_engine(engine_conf).strip
+        issues = run_engine(engine_conf).strip.split("\0")
+        result = issues.first.strip
         json = JSON.parse(result)
 
         expect(json["type"]).to eq("issue")
@@ -47,6 +48,38 @@ module CC::Engine::Analyzers
         expect(json["fingerprint"]).to eq("f21b75bbd135ec3ae6638364d5c73762")
       end
 
+      it "creates an issue for each occurrence of the duplicated code" do
+        create_source_file("foo.rb", <<-EORUBY)
+            describe '#ruby?' do
+              before { subject.type = 'ruby' }
+
+              it 'returns true' do
+                expect(subject.ruby?).to be true
+              end
+            end
+
+            describe '#js?' do
+              before { subject.type = 'js' }
+
+              it 'returns true' do
+                expect(subject.js?).to be true
+              end
+            end
+
+            describe '#whaddup?' do
+              before { subject.type = 'js' }
+
+              it 'returns true' do
+                expect(subject.js?).to be true
+              end
+            end
+        EORUBY
+
+        issues = run_engine(engine_conf).strip.split("\0")
+
+        expect(issues.length).to eq(3)
+      end
+
       it "skips unparsable files" do
         create_source_file("foo.rb", <<-EORUBY)
           ---

spec/cc/engine/analyzers/violations_spec.rb

@@ -0,0 +1,93 @@
+require "spec_helper"
+require "cc/engine/duplication"
+
+module CC::Engine::Analyzers
+  RSpec.describe Violations do
+    describe "#each" do
+      it "yields correct number of violations" do
+        issue = double(:issue, mass: 10, identical?: true)
+        hashes = sexps
+        language_strategy = double(:language_strategy, calculate_points: 30)
+
+        violations = []
+
+        Violations.new(language_strategy, issue, hashes).each do |v|
+          violations << v
+        end
+
+        expect(violations.length).to eq(3)
+      end
+
+      it "yields violation objects with correct information" do
+        issue = double(:issue, mass: 10, identical?: true)
+        hashes = sexps
+        language_strategy = double(:language_strategy, calculate_points: 30)
+
+        violations = []
+
+        Violations.new(language_strategy, issue, hashes).each do |v|
+          violations << v
+        end
+
+        first_formatted = violations[0].format
+        second_formatted = violations[1].format
+        third_formatted = violations[2].format
+
+        expect(first_formatted[:type]).to eq("issue")
+        expect(first_formatted[:check_name]).to eq("Identical code")
+        expect(first_formatted[:description]).to eq("Identical code found in 2 other locations")
+        expect(first_formatted[:categories]).to eq(["Duplication"])
+        expect(first_formatted[:remediation_points]).to eq(30)
+        expect(first_formatted[:location]).to eq({:path=>"file.rb", :lines=>{:begin=>3, :end=>9}})
+        expect(first_formatted[:other_locations]).to eq([
+          { :path => "file.rb", :lines => { :begin => 5, :end => 9} },
+          { :path => "file.rb", :lines => { :begin => 7, :end => 9} },
+        ])
+        expect(first_formatted[:fingerprint]).to eq("c2712b56bff2becf4ae2a8469e1171c7")
+
+        expect(second_formatted[:location]).to eq({:path=>"file.rb", :lines=>{:begin=>5, :end=>9}})
+        expect(second_formatted[:other_locations]).to eq([
+          { :path => "file.rb", :lines => { :begin => 3, :end => 9} },
+          { :path => "file.rb", :lines => { :begin => 7, :end => 9} },
+        ])
+
+        expect(third_formatted[:location]).to eq({:path=>"file.rb", :lines=>{:begin=>7, :end=>9}})
+        expect(third_formatted[:other_locations]).to eq([
+          { :path => "file.rb", :lines => { :begin => 3, :end => 9} },
+          { :path => "file.rb", :lines => { :begin => 5, :end => 9} },
+        ])
+      end
+
+      def sexps
+        source = <<-SOURCE
+          begin
+            foo
+          rescue Exception => e
+            Jekyll.logger.warn "Error reading file \#{File.join(base, name)}: \#{e.message}"
+          rescue Exception => e
+            Jekyll.logger.warn "Error reading file \#{File.join(base, name)}: \#{e.message}"
+          rescue Exception => e
+            Jekyll.logger.warn "Error reading file \#{File.join(base, name)}: \#{e.message}"
+          end
+        SOURCE
+
+        flay = Flay.new({
+          diff: false,
+          mass: CC::Engine::Analyzers::Ruby::Main::DEFAULT_MASS_THRESHOLD,
+          summary: false,
+          verbose: false,
+          number: true,
+          timeout: 10,
+          liberal: false,
+          fuzzy: false,
+          only: nil,
+        })
+
+        sexp = RubyParser.new.process(source, "file.rb")
+        flay.process_sexp(sexp)
+        report = flay.analyze[0]
+        sexps = flay.hashes[report.structural_hash]
+      end
+    end
+  end
+end