From 2c7162bba79121fac432e231accaa9963868a02f Mon Sep 17 00:00:00 2001 From: ABaldwinHunter Date: Mon, 25 Jan 2016 16:59:56 -0500 Subject: [PATCH] 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. --- lib/cc/engine/analyzers/reporter.rb | 16 +-- lib/cc/engine/analyzers/violation.rb | 19 +--- lib/cc/engine/analyzers/violations.rb | 35 ++++++ .../engine/analyzers/javascript/main_spec.rb | 6 +- spec/cc/engine/analyzers/php/main_spec.rb | 6 +- spec/cc/engine/analyzers/python/main_spec.rb | 6 +- spec/cc/engine/analyzers/ruby/main_spec.rb | 35 +++++- spec/cc/engine/analyzers/violations_spec.rb | 107 ++++++++++++++++++ 8 files changed, 201 insertions(+), 29 deletions(-) create mode 100644 lib/cc/engine/analyzers/violations.rb create mode 100644 spec/cc/engine/analyzers/violations_spec.rb diff --git a/lib/cc/engine/analyzers/reporter.rb b/lib/cc/engine/analyzers/reporter.rb index bbe9f83d..91ea3ef7 100644 --- a/lib/cc/engine/analyzers/reporter.rb +++ b/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 diff --git a/lib/cc/engine/analyzers/violation.rb b/lib/cc/engine/analyzers/violation.rb index 4f39fbf1..7c129a4b 100644 --- a/lib/cc/engine/analyzers/violation.rb +++ b/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? diff --git a/lib/cc/engine/analyzers/violations.rb b/lib/cc/engine/analyzers/violations.rb new file mode 100644 index 00000000..0ecbb85e --- /dev/null +++ b/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 diff --git a/spec/cc/engine/analyzers/javascript/main_spec.rb b/spec/cc/engine/analyzers/javascript/main_spec.rb index 3fa0517b..d16fb37f 100644 --- a/spec/cc/engine/analyzers/javascript/main_spec.rb +++ b/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") diff --git a/spec/cc/engine/analyzers/php/main_spec.rb b/spec/cc/engine/analyzers/php/main_spec.rb index c6898473..5145dcb1 100644 --- a/spec/cc/engine/analyzers/php/main_spec.rb +++ b/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 diff --git a/spec/cc/engine/analyzers/python/main_spec.rb b/spec/cc/engine/analyzers/python/main_spec.rb index 9c3da701..a1e1ed6a 100644 --- a/spec/cc/engine/analyzers/python/main_spec.rb +++ b/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") diff --git a/spec/cc/engine/analyzers/ruby/main_spec.rb b/spec/cc/engine/analyzers/ruby/main_spec.rb index b3af3bac..01e97097 100644 --- a/spec/cc/engine/analyzers/ruby/main_spec.rb +++ b/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) --- diff --git a/spec/cc/engine/analyzers/violations_spec.rb b/spec/cc/engine/analyzers/violations_spec.rb new file mode 100644 index 00000000..1447f5c0 --- /dev/null +++ b/spec/cc/engine/analyzers/violations_spec.rb @@ -0,0 +1,107 @@ +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=>1, :end=>5}}) + expect(first_formatted[:other_locations]).to eq([ + { :path => "file.rb", :lines => { :begin => 9, :end => 13} }, + { :path => "file.rb", :lines => { :begin => 17, :end => 21} }, + ]) + expect(first_formatted[:fingerprint]).to eq("c2712b56bff2becf4ae2a8469e1171c7") + + expect(second_formatted[:location]).to eq({:path=>"file.rb", :lines=>{:begin=>9, :end=>13}}) + expect(second_formatted[:other_locations]).to eq([ + { :path => "file.rb", :lines => { :begin => 1, :end => 5} }, + { :path => "file.rb", :lines => { :begin => 17, :end => 21} }, + ]) + + expect(third_formatted[:location]).to eq({:path=>"file.rb", :lines=>{:begin=>17, :end=>21}}) + expect(third_formatted[:other_locations]).to eq([ + { :path => "file.rb", :lines => { :begin => 1, :end => 5} }, + { :path => "file.rb", :lines => { :begin => 9, :end => 13} }, + ]) + end + + def sexps + source = <<-SOURCE +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 + 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