From 472aa510cdf17e6eadb7155a21f2ee33d385eea9 Mon Sep 17 00:00:00 2001 From: John Pignata Date: Sun, 31 Jan 2016 12:41:06 -0500 Subject: [PATCH 1/8] Bump rubocop to 0.36.0 See: https://github.com/bbatsov/rubocop/blob/master/CHANGELOG.md#0360-14012016 --- .rubocop.yml | 3 +++ Gemfile | 2 +- Gemfile.lock | 32 +++++++++++++++----------------- lib/cc/engine/rubocop.rb | 8 +++++++- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index a84782df..93c63e01 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,3 +1,6 @@ +AllCops: + TargetRubyVersion: 2.2 + Metrics/ModuleLength: Exclude: - 'spec/**/*' diff --git a/Gemfile b/Gemfile index 8a48bba1..691c2783 100644 --- a/Gemfile +++ b/Gemfile @@ -1,7 +1,7 @@ source 'https://rubygems.org' gem "activesupport", require: false -gem "rubocop", require: false +gem "rubocop", "~> 0.36.0", require: false gem "rubocop-rspec", require: false gem "safe_yaml" gem "pry", require: false diff --git a/Gemfile.lock b/Gemfile.lock index 06948bbb..5d4db92a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ GEM remote: https://rubygems.org/ specs: - activesupport (4.2.4) + activesupport (4.2.5.1) i18n (~> 0.7) json (~> 1.7, >= 1.7.7) minitest (~> 5.1) @@ -9,44 +9,39 @@ GEM tzinfo (~> 1.1) ansi (1.5.0) ast (2.2.0) - astrolabe (1.3.1) - parser (~> 2.2) builder (3.2.2) coderay (1.1.0) i18n (0.7.0) json (1.8.3) metaclass (0.0.4) method_source (0.8.2) - minitest (5.8.0) - minitest-reporters (1.0.20) + minitest (5.8.4) + minitest-reporters (1.1.7) ansi builder minitest (>= 5.0) ruby-progressbar mocha (1.1.0) metaclass (~> 0.0.1) - parser (2.2.3.0) - ast (>= 1.1, < 3.0) + parser (2.3.0.2) + ast (~> 2.2) powerpack (0.1.1) - pry (0.10.1) + pry (0.10.3) coderay (~> 1.1.0) method_source (~> 0.8.1) slop (~> 3.4) - rainbow (2.0.0) - rake (10.4.2) - rubocop (0.35.1) - astrolabe (~> 1.3) - parser (>= 2.2.3.0, < 3.0) + rainbow (2.1.0) + rake (10.5.0) + rubocop (0.36.0) + parser (>= 2.3.0.0, < 3.0) powerpack (~> 0.1) rainbow (>= 1.99.1, < 3.0) ruby-progressbar (~> 1.7) - tins (<= 1.6.0) - rubocop-rspec (1.3.0) + rubocop-rspec (1.3.1) ruby-progressbar (1.7.5) safe_yaml (1.0.4) slop (3.6.0) thread_safe (0.3.5) - tins (1.6.0) tzinfo (1.2.2) thread_safe (~> 0.1) @@ -60,6 +55,9 @@ DEPENDENCIES mocha pry rake - rubocop + rubocop (~> 0.36.0) rubocop-rspec safe_yaml + +BUNDLED WITH + 1.11.2 diff --git a/lib/cc/engine/rubocop.rb b/lib/cc/engine/rubocop.rb index a88cc9ad..8561cb31 100644 --- a/lib/cc/engine/rubocop.rb +++ b/lib/cc/engine/rubocop.rb @@ -43,7 +43,8 @@ def category(cop_name) end def inspect_file(path) - parsed = RuboCop::ProcessedSource.from_file(path) + ruby_version = target_ruby_version(path) + parsed = RuboCop::ProcessedSource.from_file(path, ruby_version) rubocop_team_for_path(path).inspect_file(parsed).each do |violation| next if violation.disabled? decorated_violation = ViolationDecorator.new(violation) @@ -74,6 +75,11 @@ def rubocop_team_for_path(path) RuboCop::Cop::Team.new(RuboCop::Cop::Cop.all, rubocop_config) end + def target_ruby_version(path) + config_store = rubocop_config_store.for(path) + config_store["AllCops"] && config_store["AllCops"]["TargetRubyVersion"] + end + def violation_positions(location) if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange) first_line = location.line From efb8e762ec2be9f61bab1bcef9941f3795ae3919 Mon Sep 17 00:00:00 2001 From: John Pignata Date: Sun, 31 Jan 2016 15:13:38 -0500 Subject: [PATCH 2/8] Refactor Rubocop runner into more objects Code Climate triggered a warning that this class was too large and looking closely it seems to have several different responsibilities mixed together. At the same time, a lot of the language used in this engine doesn't match the language of how we talk about our domain. These changes aim to break down the Rubocop class into collaborators and to refine the language to bring its concerns closer to our domain language. * Introduce SourceFile * Rename ViolationDecorator to Issue * Move presentation concerns into Issue * Refine remediation point names to better match what they represent * Begin to improve coverage on remediation point generation --- .rubocop.yml | 10 +- config/cops.yml | 20 ++-- lib/cc/engine/file_list_resolver.rb | 14 +-- lib/cc/engine/issue.rb | 125 +++++++++++++++++++++ lib/cc/engine/rubocop.rb | 121 ++++---------------- lib/cc/engine/source_file.rb | 42 +++++++ lib/cc/engine/violation_decorator.rb | 57 ---------- spec/cc/engine/issue_spec.rb | 79 +++++++++++++ spec/cc/engine/rubocop_spec.rb | 16 +-- spec/cc/engine/violation_decorator_spec.rb | 37 ------ 10 files changed, 301 insertions(+), 220 deletions(-) create mode 100644 lib/cc/engine/issue.rb create mode 100644 lib/cc/engine/source_file.rb delete mode 100644 lib/cc/engine/violation_decorator.rb create mode 100644 spec/cc/engine/issue_spec.rb delete mode 100644 spec/cc/engine/violation_decorator_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 93c63e01..fb82bcab 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -21,10 +21,14 @@ Style/PercentLiteralDelimiters: '%W': '[]' Style/StringLiterals: Enabled: false -Style/TrailingComma: - Enabled: false Style/DotPosition: EnforcedStyle: 'trailing' Enabled: true -Style/MultilineOperationIndentation: +Style/MultilineMethodCallIndentation: + Enabled: false +Style/TrailingCommaInArguments: + Enabled: false +Style/TrailingCommaInLiteral: + Enabled: false +Style/ConditionalAssignment: Enabled: false diff --git a/config/cops.yml b/config/cops.yml index 450e5d70..cc995219 100644 --- a/config/cops.yml +++ b/config/cops.yml @@ -1,25 +1,25 @@ Metrics/AbcSize: base_points: 1_000_000 - violation_points: 70_000 + overage_points: 70_000 Metrics/BlockNesting: - base_points: 100_000 + remediation_points: 100_000 Metrics/ClassLength: base_points: 5_000_000 - violation_points: 35_000 -Metrics/CyclomaticComplexity: # This check is per method + overage_points: 35_000 +Metrics/CyclomaticComplexity: base_points: 1_000_000 - violation_points: 70_000 + overage_points: 70_000 Metrics/LineLength: - base_points: 50_000 + remediation_points: 50_000 Metrics/MethodLength: base_points: 1_000_000 - violation_points: 70_000 + overage_points: 70_000 Metrics/ModuleLength: base_points: 5_000_000 - violation_points: 35_000 + overage_points: 35_000 Metrics/ParameterList: base_points: 500_000 - violation_points: 100_000 + overage_points: 100_000 Metrics/PerceivedComplexity: base_points: 1_000_000 - violation_points: 70_000 + overage_points: 70_000 diff --git a/lib/cc/engine/file_list_resolver.rb b/lib/cc/engine/file_list_resolver.rb index 4a9874df..2db1771a 100644 --- a/lib/cc/engine/file_list_resolver.rb +++ b/lib/cc/engine/file_list_resolver.rb @@ -1,11 +1,11 @@ module CC module Engine class FileListResolver - def initialize(code:, engine_config: {}, rubocop_config_store:) - @code = code + def initialize(root:, engine_config: {}, config_store:) + @root = root @exclude_paths = engine_config["exclude_paths"] || [] @include_paths = engine_config["include_paths"] - @rubocop_config_store = rubocop_config_store + @config_store = config_store end def expanded_list @@ -39,7 +39,7 @@ def include_based_files_to_inspect end def local_path(path) - realpath = Pathname.new(@code).realpath.to_s + realpath = Pathname.new(@root).realpath.to_s path.gsub(%r{^#{realpath}/}, '') end @@ -47,13 +47,13 @@ def rubocop_file_to_include?(file) if file =~ /\.rb$/ true else - dir, basename = File.split(file) - @rubocop_config_store.for(dir).file_to_include?(basename) + root, basename = File.split(file) + @config_store.for(root).file_to_include?(basename) end end def rubocop_runner - @rubocop_runner ||= RuboCop::Runner.new({}, @rubocop_config_store) + @rubocop_runner ||= RuboCop::Runner.new({}, @config_store) end end end diff --git a/lib/cc/engine/issue.rb b/lib/cc/engine/issue.rb new file mode 100644 index 00000000..b7bd25db --- /dev/null +++ b/lib/cc/engine/issue.rb @@ -0,0 +1,125 @@ +require 'safe_yaml' +SafeYAML::OPTIONS[:default_mode] = :safe + +module CC + module Engine + class Issue < SimpleDelegator + MULTIPLIER_REGEX = %r{\[([\d\.]+)\/([\d\.]+)\]} + DEFAULT_REMEDIATION_POINTS = 50_000 + DEFAULT_BASE_POINTS = 200_000 + DEFAULT_OVERAGE_POINTS = 50_000 + + def initialize(issue, path, cop_list: nil) + @path = path + @cop_list = cop_list + super(issue) + end + + # rubocop:disable Metrics/MethodLength + def to_json + hash = { + type: "Issue", + check_name: "Rubocop/#{cop_name}", + description: message, + categories: [category], + remediation_points: remediation_points, + location: { + path: path, + positions: positions, + }, + } + hash[:content] = { body: content_body } if content_body.present? + hash.to_json + end + + def remediation_points + if multiplier? + base_points + overage_points + else + cop_definition.fetch("remediation_points", DEFAULT_REMEDIATION_POINTS) + end + end + + private + + attr_reader :path + + def multiplier? + message.match(MULTIPLIER_REGEX) + end + + def base_points + cop_definition.fetch("base_points", DEFAULT_BASE_POINTS) + end + + def cop_definition + @cop_definition ||= cop_list.fetch(cop_name, {}) + end + + def cop_list + @cop_list ||= YAML.load_file(expand_config_path("cops.yml")) + end + + def expand_config_path(path) + File.expand_path("../../../../config/#{path}", __FILE__) + end + + def overage_points + overage_points = cop_definition. + fetch("overage_points", DEFAULT_OVERAGE_POINTS) + + overage_points * multiplier + end + + def multiplier + result = message.scan(MULTIPLIER_REGEX) + score, max = result[0] + score.to_i - max.to_i + end + + def category + CategoryParser.new(cop_name).category + end + + def positions + { + begin: { + column: columns.first, + line: lines.first, + }, + end: { + column: columns.last, + line: lines.last, + } + } + end + + # Increment column value as columns are 0-based in parser + def columns + return @columns if defined?(@columns) + + if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange) + @columns = [location.column + 1, location.column + 1] + else + @columns = [location.column + 1, location.last_column + 1] + end + end + + def lines + return @lines if defined?(@lines) + + if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange) + @lines = [location.line, location.line] + else + @lines = [location.first_line, location.last_line] + end + end + + def content_body + return @content_body if defined?(@content_body) + content_path = expand_config_path("contents/#{cop_name.underscore}.md") + @content_body = File.exist?(content_path) && File.read(content_path) + end + end + end +end diff --git a/lib/cc/engine/rubocop.rb b/lib/cc/engine/rubocop.rb index 8561cb31..f6b8e2c1 100644 --- a/lib/cc/engine/rubocop.rb +++ b/lib/cc/engine/rubocop.rb @@ -1,133 +1,58 @@ require "json" +require "delegate" require "pathname" require "rubocop" require "rubocop/cop/method_complexity_patch" require "rubocop/cop/method_length" require "rubocop/cop/class_length" +require "cc/engine/source_file" require "cc/engine/category_parser" require "cc/engine/file_list_resolver" -require "cc/engine/violation_decorator" +require "cc/engine/issue" require "active_support" require "active_support/core_ext" module CC module Engine class Rubocop - def initialize(code, engine_config, io) - @code = code + def initialize(root, engine_config, io) + @root = root @engine_config = engine_config || {} @io = io end def run - Dir.chdir(@code) do + Dir.chdir(root) do files_to_inspect.each do |path| - inspect_file(path) + SourceFile.new( + config_store: config_store.for(path), + io: io, + path: path, + root: root, + ).inspect end end end private - def files_to_inspect - @file_list_resolver = FileListResolver.new( - code: @code, - engine_config: @engine_config, - rubocop_config_store: rubocop_config_store - ) - @file_list_resolver.expanded_list - end - - def category(cop_name) - CategoryParser.new(cop_name).category - end - - def inspect_file(path) - ruby_version = target_ruby_version(path) - parsed = RuboCop::ProcessedSource.from_file(path, ruby_version) - rubocop_team_for_path(path).inspect_file(parsed).each do |violation| - next if violation.disabled? - decorated_violation = ViolationDecorator.new(violation) - json = violation_json(decorated_violation, local_path(path)) - @io.print "#{json}\0" - end - end + attr_reader :root, :engine_config, :io - def local_path(path) - realpath = Pathname.new(@code).realpath.to_s - path.gsub(%r{^#{realpath}/}, '') + def files_to_inspect + @files_to_inspect ||= FileListResolver.new( + config_store: config_store, + engine_config: engine_config, + root: root, + ).expanded_list end - def rubocop_config_store - @rubocop_config_store ||= begin - Dir.chdir(@code) do - config_store = RuboCop::ConfigStore.new - if (config_file = @engine_config["config"]) - config_store.options_config = config_file - end - config_store + def config_store + @config_store ||= RuboCop::ConfigStore.new.tap do |config_store| + if (config_file = engine_config["config"]) + config_store.options_config = config_file end end end - - def rubocop_team_for_path(path) - rubocop_config = rubocop_config_store.for(path) - RuboCop::Cop::Team.new(RuboCop::Cop::Cop.all, rubocop_config) - end - - def target_ruby_version(path) - config_store = rubocop_config_store.for(path) - config_store["AllCops"] && config_store["AllCops"]["TargetRubyVersion"] - end - - def violation_positions(location) - if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange) - first_line = location.line - last_line = location.line - first_column = location.column - last_column = location.column - else - first_line = location.first_line - last_line = location.last_line - first_column = location.column - last_column = location.last_column - end - - { - begin: { - column: first_column + 1, # columns are 0-based in Parser - line: first_line, - }, - end: { - column: last_column + 1, - line: last_line, - } - } - end - - def violation_json(violation, local_path) - violation_hash = { - type: "Issue", - check_name: "Rubocop/#{violation.cop_name}", - description: violation.message, - categories: [category(violation.cop_name)], - remediation_points: violation.remediation_points, - location: { - path: local_path, - positions: violation_positions(violation.location), - }, - } - body = content_body(violation.cop_name) - violation_hash.merge!(content: { body: body }) if body.present? - violation_hash.to_json - end - - def content_body(cop_name) - path = File.expand_path( - "../../../../config/contents/#{cop_name.underscore}.md", __FILE__ - ) - File.exist?(path) ? File.read(path) : nil - end end end end diff --git a/lib/cc/engine/source_file.rb b/lib/cc/engine/source_file.rb new file mode 100644 index 00000000..1c1ef93c --- /dev/null +++ b/lib/cc/engine/source_file.rb @@ -0,0 +1,42 @@ +module CC + module Engine + class SourceFile + def initialize(config_store:, io:, path:, root:) + @config_store = config_store + @io = io + @path = path + @root = root + end + + def inspect + rubocop_team.inspect_file(processed_source).each do |offense| + next if offense.disabled? + + io.print Issue.new(offense, display_path).to_json + io.print "\0" + end + end + + private + + attr_reader :config_store, :io, :path, :root + + def processed_source + RuboCop::ProcessedSource.from_file(path, target_ruby_version) + end + + def target_ruby_version + config_store["AllCops"] && config_store["AllCops"]["TargetRubyVersion"] + end + + def rubocop_team + RuboCop::Cop::Team.new(RuboCop::Cop::Cop.all, config_store) + end + + def display_path + realpath = Pathname.new(root).realpath.to_s + path.gsub(%r{^#{realpath}/}, "") + end + end + end +end diff --git a/lib/cc/engine/violation_decorator.rb b/lib/cc/engine/violation_decorator.rb deleted file mode 100644 index ea73e343..00000000 --- a/lib/cc/engine/violation_decorator.rb +++ /dev/null @@ -1,57 +0,0 @@ -require 'safe_yaml' -SafeYAML::OPTIONS[:default_mode] = :safe - -module CC - module Engine - class ViolationDecorator < SimpleDelegator - MULTIPLIER_REGEX = %r{\[([\d\.]+)\/([\d\.]+)\]} - - DEFAULT_REMEDIATION_POINTS = 200_000 - DEFAULT_POINTS_PER_VIOLATION = 50_000 - - def remediation_points - if multiplier? - base_points + violation_points - else - base_points - end - end - - private - - def base_points - cop_list. - fetch(cop_name, {}). - fetch("base_points", DEFAULT_REMEDIATION_POINTS) - end - - def violation_points - per_violation_points = cop_list. - fetch(cop_name, {}). - fetch("violation_points", DEFAULT_POINTS_PER_VIOLATION) - - per_violation_points * multiplier - end - - def multiplier - result = message.scan(MULTIPLIER_REGEX) - score, max = result[0] - score.to_i - max.to_i - end - - def multiplier? - message.match(MULTIPLIER_REGEX) - end - - def cop_list - return @cop_list if @cop_list - - cops_path = File.expand_path( - File.join(File.dirname(__FILE__), "../../../config/cops.yml") - ) - - @cop_list = YAML.load_file(cops_path) - end - end - end -end diff --git a/spec/cc/engine/issue_spec.rb b/spec/cc/engine/issue_spec.rb new file mode 100644 index 00000000..252a27ba --- /dev/null +++ b/spec/cc/engine/issue_spec.rb @@ -0,0 +1,79 @@ +require "spec_helper" +require "cc/engine/issue" + +module CC::Engine + describe Issue do + describe "#remediation points" do + describe "cop has configured remediation points" do + describe "without a multiplier" do + it "returns the configured remediation points" do + cop_list = { + "Metrics/BlockNesting" => { + "remediation_points" => 300_000, + } + } + offense = OpenStruct.new + offense.cop_name = "Metrics/BlockNesting" + offense.message = "This has no multiplier" + + issue = Issue.new(offense, "/code/file", cop_list: cop_list) + + issue.remediation_points.must_equal(300_000) + end + end + + describe "with a multiplier" do + it "calculates remediation points using the configured base and overage points" do + cop_list = { + "Metrics/AbcSize" => { + "base_points" => 5_000_000, + "overage_points" => 100_000, + } + } + offense = OpenStruct.new + offense.cop_name = "Metrics/AbcSize" + offense.message = "This has a [32/20] multiplier" + + issue = Issue.new(offense, "/code/file", cop_list: cop_list) + + base_points = 5_000_000 + overage_points = 100_000 * 12 + + issue.remediation_points.must_equal(base_points + overage_points) + end + end + end + + describe "cop has no configured remediation points" do + describe "without a multiplier" do + it "returns the default remediation points" do + offense = OpenStruct.new + offense.cop_name = "Some/UnconfiguredCop" + offense.message = "This has no multiplier" + + issue = Issue.new(offense, "/code/file.rb") + + issue.remediation_points.must_equal( + Issue::DEFAULT_REMEDIATION_POINTS + ) + end + end + + describe "with a multiplier" do + it "calculates remediation points using the default base and overage points" do + offense = OpenStruct.new + offense.cop_name = "Some/UnconfiguredCop" + offense.message = "This has a [22/20] multiplier" + + issue = Issue.new(offense, "/code/file") + + base_points = Issue::DEFAULT_BASE_POINTS + overage_points = Issue::DEFAULT_OVERAGE_POINTS * 2 + + issue.remediation_points.must_equal(base_points + overage_points) + end + end + end + end + end +end diff --git a/spec/cc/engine/rubocop_spec.rb b/spec/cc/engine/rubocop_spec.rb index 86932002..42a0b423 100644 --- a/spec/cc/engine/rubocop_spec.rb +++ b/spec/cc/engine/rubocop_spec.rb @@ -292,12 +292,12 @@ def method EORUBY output = run_engine issues = output.split("\0").map { |issue| JSON.parse(issue) } - violation = issues.find do |issue| - issue["check_name"] == "Rubocop/Metrics/MethodLength" + issue = issues.find do |i| + i["check_name"] == "Rubocop/Metrics/MethodLength" end - assert_equal 1, violation["location"]["positions"]["begin"]["line"] - assert_equal 14, violation["location"]["positions"]["end"]["line"] + assert_equal 1, issue["location"]["positions"]["begin"]["line"] + assert_equal 14, issue["location"]["positions"]["end"]["line"] end it "shows full source of long classes" do @@ -308,12 +308,12 @@ class Awesome EORUBY output = run_engine issues = output.split("\0").map { |issue| JSON.parse(issue) } - violation = issues.find do |issue| - issue["check_name"] == "Rubocop/Metrics/ClassLength" + issue = issues.find do |i| + i["check_name"] == "Rubocop/Metrics/ClassLength" end - assert_equal 1, violation["location"]["positions"]["begin"]["line"] - assert_equal 105, violation["location"]["positions"]["end"]["line"] + assert_equal 1, issue["location"]["positions"]["begin"]["line"] + assert_equal 105, issue["location"]["positions"]["end"]["line"] end def includes_check?(output, cop_name) diff --git a/spec/cc/engine/violation_decorator_spec.rb b/spec/cc/engine/violation_decorator_spec.rb deleted file mode 100644 index 8c1f9d88..00000000 --- a/spec/cc/engine/violation_decorator_spec.rb +++ /dev/null @@ -1,37 +0,0 @@ -require "spec_helper" -require "cc/engine/violation_decorator" - -module CC::Engine - describe ViolationDecorator do - describe "#remediation points" do - it "returns remediation points for violation" do - fake_violation = Minitest::Mock.new - fake_violation.expect :cop_name, :unknown_cop - fake_violation.expect :message, "This has no multiplier!" - - decorated_violation = ViolationDecorator.new(fake_violation) - - decorated_violation.remediation_points.must_equal( - ViolationDecorator::DEFAULT_REMEDIATION_POINTS - ) - end - - it "returns remedation points plus per violation points multiplied by overage" do - fake_violation = Minitest::Mock.new - fake_violation.expect :cop_name, :unknown_cop - fake_violation.expect :cop_name, :unknown_cop - fake_violation.expect :message, "This has [22/20] multiplier!" - fake_violation.expect :message, "This has [22/20] multiplier!" - - decorated_violation = ViolationDecorator.new(fake_violation) - - violation_pointss = ViolationDecorator::DEFAULT_POINTS_PER_VIOLATION * 2 - base_points = ViolationDecorator::DEFAULT_REMEDIATION_POINTS - - decorated_violation.remediation_points.must_equal( - violation_pointss + base_points - ) - end - end - end -end From d6512a0e00ed22c2cc895d1c2ec7509d1f26aee6 Mon Sep 17 00:00:00 2001 From: John Pignata Date: Sun, 31 Jan 2016 16:00:59 -0500 Subject: [PATCH 3/8] Convert suite to rspec --- Gemfile | 4 +- Gemfile.lock | 28 ++++++------ Rakefile | 13 ++---- spec/cc/engine/category_parser_spec.rb | 18 +++++--- spec/cc/engine/issue_spec.rb | 16 ++----- spec/cc/engine/rubocop_spec.rb | 59 +++++++++++++++----------- spec/spec_helper.rb | 6 +-- 7 files changed, 72 insertions(+), 72 deletions(-) diff --git a/Gemfile b/Gemfile index 691c2783..e0f80458 100644 --- a/Gemfile +++ b/Gemfile @@ -7,8 +7,6 @@ gem "safe_yaml" gem "pry", require: false group :test do - gem "mocha" gem "rake" - gem "minitest" - gem "minitest-reporters" + gem "rspec" end diff --git a/Gemfile.lock b/Gemfile.lock index 5d4db92a..17828bf8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -7,22 +7,13 @@ GEM minitest (~> 5.1) thread_safe (~> 0.3, >= 0.3.4) tzinfo (~> 1.1) - ansi (1.5.0) ast (2.2.0) - builder (3.2.2) coderay (1.1.0) + diff-lcs (1.2.5) i18n (0.7.0) json (1.8.3) - metaclass (0.0.4) method_source (0.8.2) minitest (5.8.4) - minitest-reporters (1.1.7) - ansi - builder - minitest (>= 5.0) - ruby-progressbar - mocha (1.1.0) - metaclass (~> 0.0.1) parser (2.3.0.2) ast (~> 2.2) powerpack (0.1.1) @@ -32,6 +23,19 @@ GEM slop (~> 3.4) rainbow (2.1.0) rake (10.5.0) + rspec (3.3.0) + rspec-core (~> 3.3.0) + rspec-expectations (~> 3.3.0) + rspec-mocks (~> 3.3.0) + rspec-core (3.3.1) + rspec-support (~> 3.3.0) + rspec-expectations (3.3.0) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.3.0) + rspec-mocks (3.3.1) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.3.0) + rspec-support (3.3.0) rubocop (0.36.0) parser (>= 2.3.0.0, < 3.0) powerpack (~> 0.1) @@ -50,11 +54,9 @@ PLATFORMS DEPENDENCIES activesupport - minitest - minitest-reporters - mocha pry rake + rspec rubocop (~> 0.36.0) rubocop-rspec safe_yaml diff --git a/Rakefile b/Rakefile index c3d10e07..7b48a012 100644 --- a/Rakefile +++ b/Rakefile @@ -1,10 +1,5 @@ -require 'rake/testtask' +require "rspec/core/rake_task" -Rake.add_rakelib 'lib/tasks' - -Rake::TestTask.new do |t| - t.test_files = Dir.glob('spec/**/*_spec.rb') - t.libs = %w[lib spec] -end - -task(default: :test) +Rake.add_rakelib "lib/tasks" +RSpec::Core::RakeTask.new(:spec) +task default: :spec diff --git a/spec/cc/engine/category_parser_spec.rb b/spec/cc/engine/category_parser_spec.rb index ac075e0e..ef63a929 100644 --- a/spec/cc/engine/category_parser_spec.rb +++ b/spec/cc/engine/category_parser_spec.rb @@ -3,16 +3,22 @@ module CC::Engine describe CategoryParser do - it "returns a category for the cop name" do - CategoryParser.new("Rails/Delegate").category.must_equal("Clarity") + it "returns the category mapped to the full cop name if present" do + category_parser = CategoryParser.new("Rails/Delegate") + + expect(category_parser.category).to eq("Clarity") end - it "returns a category for the cop" do - CategoryParser.new("Performance/Sup").category.must_equal("BugRisk") + it "returns the category mapped to the cop category if present" do + category_parser = CategoryParser.new("Rails/ScopeArgs") + + expect(category_parser.category).to eq("BugRisk") end - it "returns a default" do - CategoryParser.new("Sup").category.must_equal("Style") + it "returns Style as the default" do + category_parser = CategoryParser.new("Pretend") + + expect(category_parser.category).to eq("Style") end end end diff --git a/spec/cc/engine/issue_spec.rb b/spec/cc/engine/issue_spec.rb index 252a27ba..b9d83efd 100644 --- a/spec/cc/engine/issue_spec.rb +++ b/spec/cc/engine/issue_spec.rb @@ -15,10 +15,9 @@ module CC::Engine offense = OpenStruct.new offense.cop_name = "Metrics/BlockNesting" offense.message = "This has no multiplier" - issue = Issue.new(offense, "/code/file", cop_list: cop_list) - issue.remediation_points.must_equal(300_000) + expect(issue.remediation_points).to eq(300_000) end end @@ -33,13 +32,11 @@ module CC::Engine offense = OpenStruct.new offense.cop_name = "Metrics/AbcSize" offense.message = "This has a [32/20] multiplier" - issue = Issue.new(offense, "/code/file", cop_list: cop_list) - base_points = 5_000_000 overage_points = 100_000 * 12 - issue.remediation_points.must_equal(base_points + overage_points) + expect(issue.remediation_points).to eq(base_points + overage_points) end end end @@ -50,12 +47,9 @@ module CC::Engine offense = OpenStruct.new offense.cop_name = "Some/UnconfiguredCop" offense.message = "This has no multiplier" - issue = Issue.new(offense, "/code/file.rb") - issue.remediation_points.must_equal( - Issue::DEFAULT_REMEDIATION_POINTS - ) + expect(issue.remediation_points).to eq(Issue::DEFAULT_REMEDIATION_POINTS) end end @@ -64,13 +58,11 @@ module CC::Engine offense = OpenStruct.new offense.cop_name = "Some/UnconfiguredCop" offense.message = "This has a [22/20] multiplier" - issue = Issue.new(offense, "/code/file") - base_points = Issue::DEFAULT_BASE_POINTS overage_points = Issue::DEFAULT_OVERAGE_POINTS * 2 - issue.remediation_points.must_equal(base_points + overage_points) + expect(issue.remediation_points).to eq(base_points + overage_points) end end end diff --git a/spec/cc/engine/rubocop_spec.rb b/spec/cc/engine/rubocop_spec.rb index 42a0b423..7965f895 100644 --- a/spec/cc/engine/rubocop_spec.rb +++ b/spec/cc/engine/rubocop_spec.rb @@ -18,7 +18,7 @@ def method output = run_engine - assert includes_check?(output, "Lint/UselessAssignment") + expect(includes_check?(output, "Lint/UselessAssignment")).to be true end it "reads the configured ruby_style file" do @@ -38,8 +38,8 @@ def method config = { "config" => "rubocop.yml" } output = run_engine(config) - assert includes_check?(output, "Style/AndOr") - assert !includes_check?(output, "Lint/UselessAssignment") + expect(includes_check?(output, "Style/AndOr")).to be true + expect(includes_check?(output, "Lint/UselessAssignment")).to be false end it "respects the default .rubocop.yml file" do @@ -58,8 +58,8 @@ def method output = run_engine - assert includes_check?(output, "Style/AndOr") - assert !includes_check?(output, "Lint/UselessAssignment") + expect(includes_check?(output, "Style/AndOr")).to be true + expect(includes_check?(output, "Lint/UselessAssignment")).to be false end it "reads a file with a #!.*ruby declaration at the top" do @@ -73,7 +73,8 @@ def method end EORUBY output = run_engine - assert includes_check?(output, "Lint/UselessAssignment") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be true end it "uses excludes from the specified YAML config" do @@ -92,7 +93,8 @@ def method ) config = { "config" => "rubocop.yml" } output = run_engine(config) - assert !includes_check?(output, "Lint/UselessAssignment") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be false end it "uses exclusions passed in via the config hash" do @@ -107,7 +109,8 @@ def method EORUBY config = { "exclude_paths" => ["my_script"] } output = run_engine(config) - assert !includes_check?(output, "Lint/UselessAssignment") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be false end it "layers config exclusions on top of the YAML config" do @@ -129,11 +132,12 @@ def method ) config = { "config" => "rubocop.yml", "exclude_paths" => ["bar.rb"] } output = run_engine(config) - assert !includes_check?(output, "Lint/UselessAssignment") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be false end it "handles different locations properly" do - RuboCop::Cop::Team.any_instance.expects(:inspect_file).returns( + allow_any_instance_of(RuboCop::Cop::Team).to receive(:inspect_file).and_return( [ OpenStruct.new( location: RuboCop::Cop::Lint::Syntax::PseudoSourceRange.new( @@ -164,7 +168,8 @@ def method "end" => { "column" => 1, "line" => 1 } } } - assert_equal location, result["location"] + + expect(result["location"]).to eq(location) end it "includes complete method body for cyclomatic complexity issue" do @@ -192,7 +197,7 @@ def method(a,b,c,d,e,f,g) end EORUBY output = run_engine - assert includes_check?(output, "Metrics/CyclomaticComplexity") + expect(includes_check?(output, "Metrics/CyclomaticComplexity")).to be true json = JSON.parse('[' + output.split("\u0000").join(',') + ']') @@ -206,7 +211,8 @@ def method(a,b,c,d,e,f,g) "end" => { "column" => 14, "line" => 21 } } } - assert_equal location, result["location"] + + expect(result["location"]).to eq(location) end it "includes issue content when available" do @@ -215,7 +221,7 @@ def method(a,b,c,d,e,f,g) output = run_engine - assert includes_content_for?(output, "Metrics/ClassLength") + expect(includes_content_for?(output, "Metrics/ClassLength")).to be true end it "uses only include_paths when they're passed in via the config hash" do @@ -243,8 +249,9 @@ def method output = run_engine( "include_paths" => %w[included_root_file.rb subdir/] ) - assert !includes_check?(output, "Lint/UselessAssignment") - assert !includes_check?(output, "Style/AndOr") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be false + expect(includes_check?(output, "Style/AndOr")).to be false end it "ignores non-Ruby files even when passed in as include_paths" do @@ -253,9 +260,11 @@ def method output = run_engine( "include_paths" => %w[config.yml] ) - refute(issues(output).detect do |i| + issue = issues(output).detect do |i| i["description"] == "unexpected token tCOLON" - end) + end + + expect(issue).to be nil end it "includes Ruby files even if they don't end with .rb" do @@ -267,7 +276,8 @@ def method end EORUBY output = run_engine("include_paths" => %w[Rakefile]) - assert includes_check?(output, "Lint/UselessAssignment") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be true end it "skips local disables" do @@ -280,7 +290,8 @@ def method end EORUBY output = run_engine - refute includes_check?(output, "Lint/UselessAssignment") + + expect(includes_check?(output, "Lint/UselessAssignment")).to be false end it "shows full source of long methods" do @@ -296,8 +307,8 @@ def method i["check_name"] == "Rubocop/Metrics/MethodLength" end - assert_equal 1, issue["location"]["positions"]["begin"]["line"] - assert_equal 14, issue["location"]["positions"]["end"]["line"] + expect(issue["location"]["positions"]["begin"]["line"]).to eq(1) + expect(issue["location"]["positions"]["end"]["line"]).to eq(14) end it "shows full source of long classes" do @@ -312,8 +323,8 @@ class Awesome i["check_name"] == "Rubocop/Metrics/ClassLength" end - assert_equal 1, issue["location"]["positions"]["begin"]["line"] - assert_equal 105, issue["location"]["positions"]["end"]["line"] + expect(issue["location"]["positions"]["begin"]["line"]).to eq(1) + expect(issue["location"]["positions"]["end"]["line"]).to eq(105) end def includes_check?(output, cop_name) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 91472195..a5796422 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,5 +1 @@ -require 'minitest/spec' -require 'minitest/autorun' -require "minitest/reporters" -require "mocha/mini_test" -Minitest::Reporters.use! Minitest::Reporters::DefaultReporter.new +require "rspec" From 266eb9de8fecdbd34b351ffda4d9e7235067c4c3 Mon Sep 17 00:00:00 2001 From: John Pignata Date: Sun, 31 Jan 2016 22:18:20 -0500 Subject: [PATCH 4/8] Add coverage for Issue --- config/cops.yml | 6 ++++-- lib/cc/engine/issue.rb | 23 ++++++++++------------- spec/cc/engine/issue_spec.rb | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/config/cops.yml b/config/cops.yml index cc995219..9c647e08 100644 --- a/config/cops.yml +++ b/config/cops.yml @@ -2,7 +2,8 @@ Metrics/AbcSize: base_points: 1_000_000 overage_points: 70_000 Metrics/BlockNesting: - remediation_points: 100_000 + base_points: 100_000 + overage_points: 50_000 Metrics/ClassLength: base_points: 5_000_000 overage_points: 35_000 @@ -10,7 +11,8 @@ Metrics/CyclomaticComplexity: base_points: 1_000_000 overage_points: 70_000 Metrics/LineLength: - remediation_points: 50_000 + base_points: 50_000 + overage_points: 0 Metrics/MethodLength: base_points: 1_000_000 overage_points: 70_000 diff --git a/lib/cc/engine/issue.rb b/lib/cc/engine/issue.rb index b7bd25db..2eccd032 100644 --- a/lib/cc/engine/issue.rb +++ b/lib/cc/engine/issue.rb @@ -12,6 +12,7 @@ class Issue < SimpleDelegator def initialize(issue, path, cop_list: nil) @path = path @cop_list = cop_list + super(issue) end @@ -73,8 +74,8 @@ def overage_points def multiplier result = message.scan(MULTIPLIER_REGEX) - score, max = result[0] - score.to_i - max.to_i + score, threshold = result[0] + score.to_i - threshold.to_i end def category @@ -94,29 +95,25 @@ def positions } end - # Increment column value as columns are 0-based in parser + # Increments column values as columns are 0-based in parser def columns return @columns if defined?(@columns) - if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange) - @columns = [location.column + 1, location.column + 1] - else - @columns = [location.column + 1, location.last_column + 1] - end + end_column = location.try(:last_column) || location.column + @columns = [location.column + 1, end_column + 1] end def lines return @lines if defined?(@lines) - if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange) - @lines = [location.line, location.line] - else - @lines = [location.first_line, location.last_line] - end + begin_line = location.try(:first_line) || location.line + end_line = location.try(:last_line) || location.line + @lines = [begin_line, end_line] end def content_body return @content_body if defined?(@content_body) + content_path = expand_config_path("contents/#{cop_name.underscore}.md") @content_body = File.exist?(content_path) && File.read(content_path) end diff --git a/spec/cc/engine/issue_spec.rb b/spec/cc/engine/issue_spec.rb index b9d83efd..84c0cb6d 100644 --- a/spec/cc/engine/issue_spec.rb +++ b/spec/cc/engine/issue_spec.rb @@ -3,6 +3,41 @@ module CC::Engine describe Issue do + describe "#to_json" do + let(:issue) do + location = OpenStruct.new + location.first_line = 10 + location.last_line = 10 + location.column = 3 + location.last_column = 99 + + offense = OpenStruct.new + offense.cop_name = "Metrics/LineLength" + offense.message = "Line too long [100/80]" + offense.location = location + + Issue.new(offense, "app/models/user.rb") + end + + it "returns a json issue for a Rubocop offense" do + attributes = JSON.parse(issue.to_json) + + expect(attributes["type"]).to eq("Issue") + expect(attributes["check_name"]).to eq("Rubocop/Metrics/LineLength") + expect(attributes["description"]).to eq("Line too long [100/80]") + expect(attributes["categories"]).to eq(["Style"]) + expect(attributes["remediation_points"]).to eq(50_000) + expect(attributes["location"]["path"]).to eq("app/models/user.rb") + expect(attributes["location"]["positions"]["begin"]["line"]).to eq(10) + expect(attributes["location"]["positions"]["end"]["line"]).to eq(10) + expect(attributes["location"]["positions"]["begin"]["column"]).to eq(4) + expect(attributes["location"]["positions"]["end"]["column"]).to eq(100) + expect(attributes["content"]["body"]).to include( + "This cop checks the length of lines in the source code." + ) + end + end + describe "#remediation points" do describe "cop has configured remediation points" do describe "without a multiplier" do From 562cea6c9687516ffd09ec47efb07374f1819259 Mon Sep 17 00:00:00 2001 From: John Pignata Date: Tue, 9 Feb 2016 10:21:57 -0500 Subject: [PATCH 5/8] Bump to rubocop 0.37.1 --- Gemfile | 2 +- Gemfile.lock | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index e0f80458..1564374d 100644 --- a/Gemfile +++ b/Gemfile @@ -1,7 +1,7 @@ source 'https://rubygems.org' gem "activesupport", require: false -gem "rubocop", "~> 0.36.0", require: false +gem "rubocop", "~> 0.37.1", require: false gem "rubocop-rspec", require: false gem "safe_yaml" gem "pry", require: false diff --git a/Gemfile.lock b/Gemfile.lock index 17828bf8..9d7c5ff5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -14,7 +14,7 @@ GEM json (1.8.3) method_source (0.8.2) minitest (5.8.4) - parser (2.3.0.2) + parser (2.3.0.4) ast (~> 2.2) powerpack (0.1.1) pry (0.10.3) @@ -36,11 +36,12 @@ GEM diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.3.0) rspec-support (3.3.0) - rubocop (0.36.0) - parser (>= 2.3.0.0, < 3.0) + rubocop (0.37.1) + parser (>= 2.3.0.4, < 3.0) powerpack (~> 0.1) rainbow (>= 1.99.1, < 3.0) ruby-progressbar (~> 1.7) + unicode-display_width (~> 0.3) rubocop-rspec (1.3.1) ruby-progressbar (1.7.5) safe_yaml (1.0.4) @@ -48,6 +49,7 @@ GEM thread_safe (0.3.5) tzinfo (1.2.2) thread_safe (~> 0.1) + unicode-display_width (0.3.1) PLATFORMS ruby @@ -57,7 +59,7 @@ DEPENDENCIES pry rake rspec - rubocop (~> 0.36.0) + rubocop (~> 0.37.1) rubocop-rspec safe_yaml From 89f9a05ba838d775386d3ed336e40f1d9391d5f1 Mon Sep 17 00:00:00 2001 From: John Pignata Date: Wed, 10 Feb 2016 22:16:01 -0500 Subject: [PATCH 6/8] Patch RuboCop::Config to warn on obsolete cops Instead of raising on the inclusion of an obsolete cop, write an error to STDERR. This is slightly more invisible than failing a build on an obsolete cop, but preferable as a user won't be blocked when we bump RuboCop on our engine. We'll write a changelog post advising users of the necessary changes to make and deprecate this patch in the coming weeks. --- lib/cc/engine/rubocop.rb | 1 + lib/rubocop/config_patch.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 lib/rubocop/config_patch.rb diff --git a/lib/cc/engine/rubocop.rb b/lib/cc/engine/rubocop.rb index f6b8e2c1..35eacefd 100644 --- a/lib/cc/engine/rubocop.rb +++ b/lib/cc/engine/rubocop.rb @@ -2,6 +2,7 @@ require "delegate" require "pathname" require "rubocop" +require "rubocop/config_patch" require "rubocop/cop/method_complexity_patch" require "rubocop/cop/method_length" require "rubocop/cop/class_length" diff --git a/lib/rubocop/config_patch.rb b/lib/rubocop/config_patch.rb new file mode 100644 index 00000000..92871057 --- /dev/null +++ b/lib/rubocop/config_patch.rb @@ -0,0 +1,10 @@ +RuboCop::Config.class_eval do + def reject_obsolete_cops + RuboCop::Config::OBSOLETE_COPS.each do |cop_name, message| + next unless key?(cop_name) || key?(cop_name.split('/').last) + message += "\n(obsolete configuration found in #{loaded_path}, please" \ + ' update it)' + $stderr.puts message + end + end +end From 25e9a155f575edf4e69b530bf681641bfb0a196c Mon Sep 17 00:00:00 2001 From: John Pignata Date: Wed, 10 Feb 2016 22:45:56 -0500 Subject: [PATCH 7/8] Revert to rubocop 0.36.0 config --- .rubocop.yml | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index fb82bcab..a84782df 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,3 @@ -AllCops: - TargetRubyVersion: 2.2 - Metrics/ModuleLength: Exclude: - 'spec/**/*' @@ -21,14 +18,10 @@ Style/PercentLiteralDelimiters: '%W': '[]' Style/StringLiterals: Enabled: false +Style/TrailingComma: + Enabled: false Style/DotPosition: EnforcedStyle: 'trailing' Enabled: true -Style/MultilineMethodCallIndentation: - Enabled: false -Style/TrailingCommaInArguments: - Enabled: false -Style/TrailingCommaInLiteral: - Enabled: false -Style/ConditionalAssignment: +Style/MultilineOperationIndentation: Enabled: false From 88974fc0ba4d49afdd903051d65398ba719bf85a Mon Sep 17 00:00:00 2001 From: John Pignata Date: Thu, 11 Feb 2016 11:45:35 -0500 Subject: [PATCH 8/8] Bump to rubocop 0.37.2 --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 1564374d..3e553ad9 100644 --- a/Gemfile +++ b/Gemfile @@ -1,7 +1,7 @@ source 'https://rubygems.org' gem "activesupport", require: false -gem "rubocop", "~> 0.37.1", require: false +gem "rubocop", "~> 0.37.2", require: false gem "rubocop-rspec", require: false gem "safe_yaml" gem "pry", require: false diff --git a/Gemfile.lock b/Gemfile.lock index 9d7c5ff5..5a209965 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,7 +36,7 @@ GEM diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.3.0) rspec-support (3.3.0) - rubocop (0.37.1) + rubocop (0.37.2) parser (>= 2.3.0.4, < 3.0) powerpack (~> 0.1) rainbow (>= 1.99.1, < 3.0) @@ -59,7 +59,7 @@ DEPENDENCIES pry rake rspec - rubocop (~> 0.37.1) + rubocop (~> 0.37.2) rubocop-rspec safe_yaml