diff --git a/danger/plugins/rubocop.rb b/danger/plugins/rubocop.rb
index 6645a37041beab0c808028a49ce10e908f5edcc2..d1f63f356440e60fc0907da1200128658a45cb80 100644
--- a/danger/plugins/rubocop.rb
+++ b/danger/plugins/rubocop.rb
@@ -1,12 +1,11 @@
# frozen_string_literal: true
require_relative '../../tooling/danger/rubocop_inline_disable_suggestion'
+require_relative '../../tooling/danger/rubocop_discourage_todo_addition'
+require_relative '../../tooling/danger/rubocop_helper'
module Danger
class Rubocop < ::Danger::Plugin
- # Put the helper code somewhere it can be tested
- def add_suggestions_for(filename)
- Tooling::Danger::RubocopInlineDisableSuggestion.new(filename, context: self).suggest
- end
+ include Tooling::Danger::RubocopHelper
end
end
diff --git a/danger/rubocop/Dangerfile b/danger/rubocop/Dangerfile
index b43a1042bd51174e8d0144fd739d5187bc48a158..8c5a408fc9c343886856d3d8e025fdca64f6a171 100644
--- a/danger/rubocop/Dangerfile
+++ b/danger/rubocop/Dangerfile
@@ -1,14 +1,4 @@
# frozen_string_literal: true
-# This is noisy for draft MRs, so let's ignore this cop in draft mode since we have
-# rubocop watching this as well.
-return if helper.draft_mr?
-
-# Danger should not comment when inline disables are added in the following files.
-no_suggestions_for_extensions = %w[.md]
-
-helper.all_changed_files.each do |filename|
- next if filename.end_with?(*no_suggestions_for_extensions)
-
- rubocop.add_suggestions_for(filename)
-end
+rubocop.execute_inline_disable_suggestor
+rubocop.execute_todo_suggestor
diff --git a/spec/tooling/danger/rubocop_discourage_todo_addition_spec.rb b/spec/tooling/danger/rubocop_discourage_todo_addition_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..1a7f41fb5ee213982df688aabd473e93f729cd35
--- /dev/null
+++ b/spec/tooling/danger/rubocop_discourage_todo_addition_spec.rb
@@ -0,0 +1,113 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'gitlab/dangerfiles/spec_helper'
+
+require_relative '../../../tooling/danger/rubocop_discourage_todo_addition'
+require_relative '../../../tooling/danger/project_helper'
+
+RSpec.describe Tooling::Danger::RubocopDiscourageTodoAddition, feature_category: :tooling do
+ include_context "with dangerfile"
+
+ let(:fake_danger) { DangerSpecHelper.fake_danger }
+ let(:fake_project_helper) { instance_double('Tooling::Danger::ProjectHelper') }
+ let(:filename) { '.rubocop_todo/foo.yml' }
+ let(:template) { Tooling::Danger::RubocopDiscourageTodoAddition::SUGGESTION }
+ let(:file_lines) do
+ <<~YML.split("\n")
+ ---
+ RSpec/AvoidConditionalStatements:
+ Exclude:
+ - 'ee/spec/features/admin/admin_settings_spec.rb'
+ - 'ee/spec/features/analytics/code_analytics_spec.rb'
+ - 'ee/spec/features/foo_spec.rb'
+ - 'ee/spec/features/billings/billing_plans_spec.rb'
+ - 'ee/spec/features/boards/scoped_issue_board_spec.rb'
+ - 'ee/spec/features/boards/user_visits_board_spec.rb'
+ - 'ee/spec/features/ci_shared_runner_warnings_spec.rb'
+ - 'ee/spec/features/bar_spec.rb'
+ - 'ee/spec/features/epic_boards/epic_boards_spec.rb'
+ YML
+ end
+
+ let(:changed_lines) do
+ <<~DIFF.split("\n")
+ + - 'ee/spec/features/foo_spec.rb'
+ + - 'ee/spec/features/bar_spec.rb'
+ DIFF
+ end
+
+ subject(:rubocop) { fake_danger.new(helper: fake_helper) }
+
+ before do
+ allow(rubocop).to receive(:project_helper).and_return(fake_project_helper)
+ allow(rubocop.helper).to receive(:changed_lines).with(filename).and_return(changed_lines)
+ allow(rubocop.project_helper).to receive(:file_lines).and_return(file_lines)
+
+ rubocop.define_singleton_method(:add_todo_suggestion_for) do |filename|
+ Tooling::Danger::RubocopDiscourageTodoAddition.new(filename, context: self).suggest
+ end
+ end
+
+ it 'adds only one comment in the file' do
+ expect(rubocop).to receive(:markdown).with("\n#{template}".chomp, file: filename, line: 6)
+
+ rubocop.add_todo_suggestion_for(filename)
+ end
+
+ context 'with grace period changes' do
+ let(:file_lines) do
+ <<~YML.split("\n")
+ ---
+ RSpec/AvoidConditionalStatements:
+ Details: grace period
+ Exclude:
+ - 'ee/spec/features/analytics/code_analytics_spec.rb'
+ - 'ee/spec/features/foo_spec.rb'
+ YML
+ end
+
+ let(:basic_diff_lines) do
+ <<~DIFF.split("\n")
+ + - 'ee/spec/features/foo_spec.rb'
+ + - 'ee/spec/features/bar_spec.rb'
+ DIFF
+ end
+
+ let(:changed_lines) { basic_diff_lines }
+
+ shared_examples_for 'no_suggestions_for_file' do
+ it 'ignores the file' do
+ expect(rubocop).not_to receive(:markdown)
+
+ rubocop.add_todo_suggestion_for(filename)
+ end
+ end
+
+ context 'when grace period exists and is not part of the change' do
+ it_behaves_like 'no_suggestions_for_file'
+ end
+
+ context 'when grace period exists and is added as part of the change' do
+ let(:changed_lines) { basic_diff_lines.prepend('+ Details: grace period') }
+
+ it_behaves_like 'no_suggestions_for_file'
+ end
+
+ context 'when grace period exists and is removed as part of the change' do
+ let(:file_lines) do
+ <<~YML.split("\n")
+ ---
+ RSpec/AvoidConditionalStatements:
+ Exclude:
+ - 'ee/spec/features/analytics/code_analytics_spec.rb'
+ - 'ee/spec/features/foo_spec.rb'
+ YML
+ end
+
+ let(:changed_lines) { basic_diff_lines.prepend('- Details: grace period') }
+
+ it_behaves_like 'no_suggestions_for_file'
+ end
+ end
+end
diff --git a/spec/tooling/danger/rubocop_helper_spec.rb b/spec/tooling/danger/rubocop_helper_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..868b2312e2d6007e834fbdd6d37675072d36c0af
--- /dev/null
+++ b/spec/tooling/danger/rubocop_helper_spec.rb
@@ -0,0 +1,107 @@
+# frozen_string_literal: true
+
+require 'rspec-parameterized'
+require 'fast_spec_helper'
+require 'gitlab/dangerfiles/spec_helper'
+require_relative '../../../tooling/danger/rubocop_helper'
+require_relative '../../../tooling/danger/rubocop_discourage_todo_addition'
+require_relative '../../../tooling/danger/rubocop_inline_disable_suggestion'
+require_relative '../../../tooling/danger/project_helper'
+
+RSpec.describe Tooling::Danger::RubocopHelper, feature_category: :tooling do
+ include_context 'with dangerfile'
+
+ let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
+ let(:rubocop) { fake_danger.new(helper: fake_helper) }
+
+ before do
+ allow(fake_helper).to receive(:changed_lines).and_return([])
+ end
+
+ describe 'rubocop inline disable suggestor danger' do
+ let(:all_changed_files) { %w[app/models/user.rb something.rb doc.md Gemfile] }
+ let(:draft_mr?) { false }
+
+ before do
+ allow(fake_helper).to receive(:all_changed_files).and_return(all_changed_files)
+ allow(fake_helper).to receive(:draft_mr?).and_return(draft_mr?)
+ end
+
+ it 'processes the right amount of files' do
+ expect(rubocop).to receive(:add_inline_disable_suggestions_for).exactly(3).times.and_call_original
+
+ rubocop.execute_inline_disable_suggestor
+ end
+
+ context 'when it is a draft mr' do
+ let(:draft_mr?) { true }
+
+ it 'does not perform any processing of files' do
+ expect(rubocop).not_to receive(:add_inline_disable_suggestions_for)
+
+ rubocop.execute_inline_disable_suggestor
+ end
+ end
+ end
+
+ describe 'rubocop discourage todo addition danger' do
+ using RSpec::Parameterized::TableSyntax
+
+ let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
+
+ where do
+ {
+ 'with only todo files' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml],
+ modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml],
+ suggest_calls: 0
+ },
+ 'with only todo files and Gemfile.lock' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml Gemfile.lock],
+ modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml Gemfile.lock],
+ suggest_calls: 0
+ },
+ 'with todo files and other files' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml app/models/user.rb],
+ modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml app/models/user.rb],
+ suggest_calls: 2
+ },
+ 'with added or removed todo files and other files' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml app/models/user.rb],
+ modified_files: %w[app/models/user.rb],
+ suggest_calls: 0
+ },
+ 'with todo files and Gemfile' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml Gemfile Gemfile],
+ modified_files: %w[.rubocop_todo/foo/foo.yml Gemfile Gemfile],
+ suggest_calls: 0
+ },
+ 'with todo files and rubocop config file' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop.yml],
+ modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop.yml],
+ suggest_calls: 0
+ },
+ 'with todo files Gemfile and other files' => {
+ all_changed_files: %w[.rubocop_todo/foo/foo.yml Gemfile app/models/user.rb],
+ modified_files: %w[.rubocop_todo/foo/foo.yml Gemfile app/models/user.rb],
+ suggest_calls: 0
+ }
+ }
+ end
+
+ with_them do
+ before do
+ allow(fake_helper).to receive(:all_changed_files).and_return(all_changed_files)
+ allow(fake_helper).to receive(:modified_files).and_return(modified_files)
+ allow(rubocop).to receive(:project_helper).and_return(fake_project_helper)
+ allow(fake_project_helper).to receive(:file_lines).and_return([])
+ end
+
+ specify do
+ expect(rubocop).to receive(:add_todo_suggestion_for).exactly(suggest_calls).times.and_call_original
+
+ rubocop.execute_todo_suggestor
+ end
+ end
+ end
+end
diff --git a/tooling/danger/rubocop_discourage_todo_addition.rb b/tooling/danger/rubocop_discourage_todo_addition.rb
new file mode 100644
index 0000000000000000000000000000000000000000..caca5b51bdf85674dbfcac42a480fe7bb7ad7293
--- /dev/null
+++ b/tooling/danger/rubocop_discourage_todo_addition.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require_relative 'suggestion'
+require_relative '../../rubocop/formatter/graceful_formatter'
+
+module Tooling
+ module Danger
+ class RubocopDiscourageTodoAddition < Suggestion
+ ONCE_PER_FILE = true
+ MATCH = %r{\s*-\s*['"].*['"]\s*}
+ REPLACEMENT = nil
+
+ SUGGESTION = <<~MESSAGE_MARKDOWN
+ Adding exclusions to RuboCop TODO files manually is discouraged.
+
+ If it is not possible to resolve the exception, then
+ [inline disabling](https://docs.gitlab.com/ee/development/rubocop_development_guide.html#disabling-rules-inline)
+ should be used.
+
+ ----
+
+ To reduce noise, this message will only appear once per file.
+ MESSAGE_MARKDOWN
+
+ def suggest
+ return if existing_grace_period? || added_grace_period?
+
+ super
+ end
+
+ private
+
+ def existing_grace_period?
+ project_helper
+ .file_lines(filename).grep(/\A\s*#{::RuboCop::Formatter::GracefulFormatter.grace_period_key_value}/).any?
+ end
+
+ def added_grace_period?
+ helper.changed_lines(filename).grep(/\s*#{::RuboCop::Formatter::GracefulFormatter.grace_period_key_value}/).any?
+ end
+ end
+ end
+end
diff --git a/tooling/danger/rubocop_helper.rb b/tooling/danger/rubocop_helper.rb
new file mode 100644
index 0000000000000000000000000000000000000000..9104e17bf6f4268ddd9107b378a09c0fe6b4f209
--- /dev/null
+++ b/tooling/danger/rubocop_helper.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+module Tooling
+ module Danger
+ module RubocopHelper
+ def execute_inline_disable_suggestor
+ # This is noisy for draft MRs, so let's ignore this cop in draft mode since we have
+ # rubocop watching this as well.
+ return if helper.draft_mr?
+
+ # Danger should not comment when inline disables are added in the following files.
+ no_suggestions_for_extensions = %w[.md]
+
+ helper.all_changed_files.each do |filename|
+ next if filename.end_with?(*no_suggestions_for_extensions)
+
+ add_inline_disable_suggestions_for(filename)
+ end
+ end
+
+ def execute_todo_suggestor
+ # we'll assume a todo file modification only means regeneration/gem update/etc
+ return if contains_rubocop_update_files? || only_rubocop_todo_files?
+
+ modified_rubocop_todo_files.each do |filename|
+ add_todo_suggestion_for(filename)
+ end
+ end
+
+ private
+
+ def contains_rubocop_update_files?
+ # this will help be a signal for valid todo file additions or changes
+ helper.all_changed_files.any? { |path| path =~ %r{\A(Gemfile(\z|.lock\z)|.rubocop.yml\z)} }
+ end
+
+ def only_rubocop_todo_files?
+ # this will help be a signal that this is change only has todo files in it
+ helper.all_changed_files.none? { |path| path !~ %r{\A\.rubocop_todo/.*/\w+.yml\b} }
+ end
+
+ def modified_rubocop_todo_files
+ files = helper.modified_files
+
+ files.select { |path| path =~ %r{\A\.rubocop_todo/.*/\w+.yml\b} }
+ end
+
+ def add_todo_suggestion_for(filename)
+ Tooling::Danger::RubocopDiscourageTodoAddition.new(filename, context: self).suggest
+ end
+
+ def add_inline_disable_suggestions_for(filename)
+ Tooling::Danger::RubocopInlineDisableSuggestion.new(filename, context: self).suggest
+ end
+ end
+ end
+end
diff --git a/tooling/danger/suggestion.rb b/tooling/danger/suggestion.rb
index ff1ca83876deafbe0744e657a31fd92f321672af..83984cb0b0a47a4b1576e58055e801cee7e6f2c0 100644
--- a/tooling/danger/suggestion.rb
+++ b/tooling/danger/suggestion.rb
@@ -15,6 +15,8 @@ module Danger
class Suggestion
include ::Tooling::Danger::Suggestor
+ ONCE_PER_FILE = false
+
attr_reader :filename
def initialize(filename, context:)
@@ -27,7 +29,8 @@ def suggest
filename: filename,
regex: self.class::MATCH,
replacement: self.class::REPLACEMENT,
- comment_text: self.class::SUGGESTION
+ comment_text: self.class::SUGGESTION,
+ once_per_file: self.class::ONCE_PER_FILE
)
end
diff --git a/tooling/danger/suggestor.rb b/tooling/danger/suggestor.rb
index c3f886b0af34795cd97b9c7dc14f2b69cfe35d80..0e61005629d14b3140af8197831e0bd27afc2e32 100644
--- a/tooling/danger/suggestor.rb
+++ b/tooling/danger/suggestor.rb
@@ -4,7 +4,7 @@ module Tooling
module Danger
module Suggestor
# For file lines matching `regex` adds suggestion `replacement` with `comment_text` added.
- def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclude: nil)
+ def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclude: nil, once_per_file: false)
added_lines = added_lines_matching(filename, regex)
return if added_lines.empty?
@@ -12,6 +12,8 @@ def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclu
file_lines = project_helper.file_lines(filename)
added_lines.each_with_object([]) do |added_line, processed_line_numbers|
+ break if once_per_file && processed_line_numbers.any?
+
line_number = find_line_number(file_lines, added_line.delete_prefix('+'),
exclude_indexes: processed_line_numbers)