Merge pull request #32441 from composerinteralia/refute-not

Add custom RuboCop for `assert_not` over `refute`

.rubocop.yml

@@ -1,3 +1,5 @@
+require: './ci/custom_cops/lib/custom_cops'
+
 AllCops:
   TargetRubyVersion: 2.4
   # RuboCop has a bunch of cops enabled by default. This setting tells RuboCop
@@ -8,6 +10,11 @@ AllCops:
     - '**/vendor/**/*'
     - 'actionpack/lib/action_dispatch/journey/parser.rb'
 
+# Prefer assert_not_x over refute_x
+CustomCops/RefuteNot:
+  Include:
+    - '**/*_test.rb'
+
 # Prefer &&/|| over and/or.
 Style/AndOr:
   Enabled: true

actionpack/test/controller/integration_test.rb

@@ -345,7 +345,7 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
       follow_redirect!
 
       assert_response :ok
-      refute_same previous_html_document, html_document
+      assert_not_same previous_html_document, html_document
     end
   end
 
@@ -375,7 +375,7 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
     a = open_session
     b = open_session
 
-    refute_same(a.integration_session, b.integration_session)
+    assert_not_same(a.integration_session, b.integration_session)
   end
 
   def test_get_with_query_string

actionpack/test/controller/parameters/parameters_permit_test.rb

@@ -309,7 +309,7 @@ class ParametersPermitTest < ActiveSupport::TestCase
     merged_params = @params.reverse_merge(default_params)
 
     assert_equal "1234", merged_params[:id]
-    refute_predicate merged_params[:person], :empty?
+    assert_not_predicate merged_params[:person], :empty?
   end
 
   test "#with_defaults is an alias of reverse_merge" do
@@ -317,11 +317,11 @@ class ParametersPermitTest < ActiveSupport::TestCase
     merged_params = @params.with_defaults(default_params)
 
     assert_equal "1234", merged_params[:id]
-    refute_predicate merged_params[:person], :empty?
+    assert_not_predicate merged_params[:person], :empty?
   end
 
   test "not permitted is sticky beyond reverse_merge" do
-    refute_predicate @params.reverse_merge(a: "b"), :permitted?
+    assert_not_predicate @params.reverse_merge(a: "b"), :permitted?
   end
 
   test "permitted is sticky beyond reverse_merge" do
@@ -334,7 +334,7 @@ class ParametersPermitTest < ActiveSupport::TestCase
     @params.reverse_merge!(default_params)
 
     assert_equal "1234", @params[:id]
-    refute_predicate @params[:person], :empty?
+    assert_not_predicate @params[:person], :empty?
   end
 
   test "#with_defaults! is an alias of reverse_merge!" do
@@ -342,7 +342,7 @@ class ParametersPermitTest < ActiveSupport::TestCase
     @params.with_defaults!(default_params)
 
     assert_equal "1234", @params[:id]
-    refute_predicate @params[:person], :empty?
+    assert_not_predicate @params[:person], :empty?
   end
 
   test "modifying the parameters" do

actionpack/test/dispatch/system_testing/server_test.rb

@@ -17,7 +17,7 @@ class ServerTest < ActiveSupport::TestCase
   test "server is changed from `default` to `puma`" do
     Capybara.server = :default
     ActionDispatch::SystemTesting::Server.new.run
-    refute_equal Capybara.server, Capybara.servers[:default]
+    assert_not_equal Capybara.server, Capybara.servers[:default]
   end
 
   test "server is not changed to `puma` when is different than default" do

activerecord/test/cases/associations/has_one_associations_test.rb

@@ -678,7 +678,7 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     book = SpecialBook.create!(status: "published")
     author.book = book
 
-    refute_equal 0, SpecialAuthor.joins(:book).where(books: { status: "published" }).count
+    assert_not_equal 0, SpecialAuthor.joins(:book).where(books: { status: "published" }).count
   end
 
   def test_association_enum_works_properly_with_nested_join

activerecord/test/cases/associations/join_model_test.rb

@@ -369,7 +369,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
     Tag.has_many :null_taggings, -> { none }, class_name: :Tagging
     Tag.has_many :null_tagged_posts, through: :null_taggings, source: "taggable", source_type: "Post"
     assert_equal [], tags(:general).null_tagged_posts
-    refute_equal [], tags(:general).tagged_posts
+    assert_not_equal [], tags(:general).tagged_posts
   end
 
   def test_eager_has_many_polymorphic_with_source_type

activerecord/test/cases/base_test.rb

@@ -1497,7 +1497,7 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   test "column names are quoted when using #from clause and model has ignored columns" do
-    refute_empty Developer.ignored_columns
+    assert_not_empty Developer.ignored_columns
     query = Developer.from("developers").to_sql
     quoted_id = "#{Developer.quoted_table_name}.#{Developer.quoted_primary_key}"
 

activerecord/test/cases/connection_adapters/connection_handler_test.rb

@@ -328,7 +328,7 @@ module ActiveRecord
 
           pool = klass2.establish_connection(ActiveRecord::Base.connection_pool.spec.config)
           assert_same klass2.connection, pool.connection
-          refute_same klass2.connection, ActiveRecord::Base.connection
+          assert_not_same klass2.connection, ActiveRecord::Base.connection
 
           klass2.remove_connection
 
@@ -347,7 +347,7 @@ module ActiveRecord
         def test_remove_connection_should_not_remove_parent
           klass2 = Class.new(Base) { def self.name; "klass2"; end }
           klass2.remove_connection
-          refute_nil ActiveRecord::Base.connection
+          assert_not_nil ActiveRecord::Base.connection
           assert_same klass2.connection, ActiveRecord::Base.connection
         end
       end

activerecord/test/cases/migration_test.rb

@@ -404,7 +404,7 @@ class MigrationTest < ActiveRecord::TestCase
     ENV["RAILS_ENV"]    = ENV["RACK_ENV"] = "foofoo"
     new_env = ActiveRecord::ConnectionHandling::DEFAULT_ENV.call
 
-    refute_equal current_env, new_env
+    assert_not_equal current_env, new_env
 
     sleep 1 # mysql by default does not store fractional seconds in the database
     migrator.up

activerecord/test/cases/multiparameter_attributes_test.rb

@@ -394,6 +394,6 @@ class MultiParameterAttributeTest < ActiveRecord::TestCase
       "written_on(4i)" => "13",
       "written_on(5i)" => "55",
     )
-    refute_predicate topic, :written_on_came_from_user?
+    assert_not_predicate topic, :written_on_came_from_user?
   end
 end

activerecord/test/cases/schema_dumper_test.rb

@@ -469,7 +469,7 @@ class SchemaDumperTest < ActiveRecord::TestCase
     output = ActiveRecord::SchemaDumper.dump(ActiveRecord::Base.connection, stream).string
 
     assert_match %r{create_table "omg_cats"}, output
-    refute_match %r{create_table "cats"}, output
+    assert_no_match %r{create_table "cats"}, output
   ensure
     migration.migrate(:down)
     ActiveRecord::Base.table_name_prefix = original_table_name_prefix

activerecord/test/cases/statement_cache_test.rb

@@ -102,7 +102,7 @@ module ActiveRecord
         Book.find_by(name: "my other book")
       end
 
-      refute_equal book, other_book
+      assert_not_equal book, other_book
     end
 
     def test_find_by_does_not_use_statement_cache_if_table_name_is_changed

activerecord/test/cases/transactions_test.rb

@@ -323,8 +323,8 @@ class TransactionTest < ActiveRecord::TestCase
       raise ActiveRecord::Rollback
     end
 
-    refute_predicate topic_one, :persisted?
-    refute_predicate topic_two, :persisted?
+    assert_not_predicate topic_one, :persisted?
+    assert_not_predicate topic_two, :persisted?
   end
 
   def test_nested_transaction_without_new_transaction_applies_parent_state_on_rollback
@@ -344,8 +344,8 @@ class TransactionTest < ActiveRecord::TestCase
       raise ActiveRecord::Rollback
     end
 
-    refute_predicate topic_one, :persisted?
-    refute_predicate topic_two, :persisted?
+    assert_not_predicate topic_one, :persisted?
+    assert_not_predicate topic_two, :persisted?
   end
 
   def test_double_nested_transaction_applies_parent_state_on_rollback
@@ -371,9 +371,9 @@ class TransactionTest < ActiveRecord::TestCase
       raise ActiveRecord::Rollback
     end
 
-    refute_predicate topic_one, :persisted?
-    refute_predicate topic_two, :persisted?
-    refute_predicate topic_three, :persisted?
+    assert_not_predicate topic_one, :persisted?
+    assert_not_predicate topic_two, :persisted?
+    assert_not_predicate topic_three, :persisted?
   end
 
   def test_manually_rolling_back_a_transaction

activesupport/test/dependencies_test.rb

@@ -223,7 +223,7 @@ class DependenciesTest < ActiveSupport::TestCase
         Timeout.timeout(0.1) do
           # Remove the constant, as if Rails development middleware is reloading changed files:
           ActiveSupport::Dependencies.remove_unloadable_constants!
-          refute defined?(AnotherConstant::ReloadError)
+          assert_not defined?(AnotherConstant::ReloadError)
         end
 
         # Change the file, so that it is **correct** this time:
@@ -231,7 +231,7 @@ class DependenciesTest < ActiveSupport::TestCase
 
         # Again: Remove the constant, as if Rails development middleware is reloading changed files:
         ActiveSupport::Dependencies.remove_unloadable_constants!
-        refute defined?(AnotherConstant::ReloadError)
+        assert_not defined?(AnotherConstant::ReloadError)
 
         # Now, reload the _fixed_ constant:
         assert ConstantReloadError

ci/custom_cops/bin/test

@@ -0,0 +1,5 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+COMPONENT_ROOT = File.expand_path("..", __dir__)
+require_relative "../../../tools/test"

ci/custom_cops/lib/custom_cops.rb

@@ -0,0 +1,3 @@
+# frozen_string_literal: true
+
+require_relative "custom_cops/refute_not"

ci/custom_cops/lib/custom_cops/refute_not.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+module CustomCops
+  # Enforces the use of `#assert_not` methods over `#refute` methods.
+  #
+  # @example
+  #   # bad
+  #   refute false
+  #   refute_empty [1, 2, 3]
+  #   refute_equal true, false
+  #
+  #   # good
+  #   assert_not false
+  #   assert_not_empty [1, 2, 3]
+  #   assert_not_equal true, false
+  #
+  class RefuteNot < RuboCop::Cop::Cop
+    MSG = "Prefer `%<assert_method>s` over `%<refute_method>s`"
+
+    CORRECTIONS = {
+      refute:             "assert_not",
+      refute_empty:       "assert_not_empty",
+      refute_equal:       "assert_not_equal",
+      refute_in_delta:    "assert_not_in_delta",
+      refute_in_epsilon:  "assert_not_in_epsilon",
+      refute_includes:    "assert_not_includes",
+      refute_instance_of: "assert_not_instance_of",
+      refute_kind_of:     "assert_not_kind_of",
+      refute_nil:         "assert_not_nil",
+      refute_operator:    "assert_not_operator",
+      refute_predicate:   "assert_not_predicate",
+      refute_respond_to:  "assert_not_respond_to",
+      refute_same:        "assert_not_same",
+      refute_match:       "assert_no_match"
+    }.freeze
+
+    OFFENSIVE_METHODS = CORRECTIONS.keys.freeze
+
+    def_node_matcher :offensive?, "(send nil? #offensive_method? ...)"
+
+    def on_send(node)
+      return unless offensive?(node)
+
+      message = offense_message(node.method_name)
+      add_offense(node, location: :selector, message: message)
+    end
+
+    def autocorrect(node)
+      ->(corrector) do
+        corrector.replace(
+          node.loc.selector,
+          CORRECTIONS[node.method_name]
+        )
+      end
+    end
+
+    private
+
+      def offensive_method?(method_name)
+        OFFENSIVE_METHODS.include?(method_name)
+      end
+
+      def offense_message(method_name)
+        format(
+          MSG,
+          refute_method: method_name,
+          assert_method: CORRECTIONS[method_name]
+        )
+      end
+  end
+end

ci/custom_cops/test/custom_cops/refute_not_test.rb

@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+require "support/cop_helper"
+require "./lib/custom_cops/refute_not"
+
+class RefuteNotTest < ActiveSupport::TestCase
+  include CopHelper
+
+  setup do
+    @cop = CustomCops::RefuteNot.new
+  end
+
+  {
+    refute:             :assert_not,
+    refute_empty:       :assert_not_empty,
+    refute_equal:       :assert_not_equal,
+    refute_in_delta:    :assert_not_in_delta,
+    refute_in_epsilon:  :assert_not_in_epsilon,
+    refute_includes:    :assert_not_includes,
+    refute_instance_of: :assert_not_instance_of,
+    refute_kind_of:     :assert_not_kind_of,
+    refute_nil:         :assert_not_nil,
+    refute_operator:    :assert_not_operator,
+    refute_predicate:   :assert_not_predicate,
+    refute_respond_to:  :assert_not_respond_to,
+    refute_same:        :assert_not_same,
+    refute_match:       :assert_no_match
+  }.each do |refute_method, assert_method|
+    test "rejects `#{refute_method}` with a single argument" do
+      inspect_source(@cop, "#{refute_method} a")
+      assert_offense @cop, offense_message(refute_method, assert_method)
+    end
+
+    test "rejects `#{refute_method}` with multiple arguments" do
+      inspect_source(@cop, "#{refute_method} a, b, c")
+      assert_offense @cop, offense_message(refute_method, assert_method)
+    end
+
+    test "autocorrects `#{refute_method}` with a single argument" do
+      corrected = autocorrect_source(@cop, "#{refute_method} a")
+      assert_equal "#{assert_method} a", corrected
+    end
+
+    test "autocorrects `#{refute_method}` with multiple arguments" do
+      corrected = autocorrect_source(@cop, "#{refute_method} a, b, c")
+      assert_equal "#{assert_method} a, b, c", corrected
+    end
+
+    test "accepts `#{assert_method}` with a single argument" do
+      inspect_source(@cop, "#{assert_method} a")
+      assert_empty @cop.offenses
+    end
+
+    test "accepts `#{assert_method}` with multiple arguments" do
+      inspect_source(@cop, "#{assert_method} a, b, c")
+      assert_empty @cop.offenses
+    end
+  end
+
+  private
+
+    def assert_offense(cop, expected_message)
+      assert_not_empty cop.offenses
+
+      offense = cop.offenses.first
+      carets = "^" * offense.column_length
+
+      assert_equal expected_message, "#{carets} #{offense.message}"
+    end
+
+    def offense_message(refute_method, assert_method)
+      carets = "^" * refute_method.to_s.length
+      "#{carets} Prefer `#{assert_method}` over `#{refute_method}`"
+    end
+end

ci/custom_cops/test/support/cop_helper.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+require "rubocop"
+
+module CopHelper
+  def inspect_source(cop, source)
+    processed_source = parse_source(source)
+    raise "Error parsing example code" unless processed_source.valid_syntax?
+    investigate(cop, processed_source)
+    processed_source
+  end
+
+  def autocorrect_source(cop, source)
+    cop.instance_variable_get(:@options)[:auto_correct] = true
+    processed_source = inspect_source(cop, source)
+    rewrite(cop, processed_source)
+  end
+
+  private
+    TARGET_RUBY_VERSION = 2.4
+
+    def parse_source(source)
+      RuboCop::ProcessedSource.new(source, TARGET_RUBY_VERSION)
+    end
+
+    def rewrite(cop, processed_source)
+      RuboCop::Cop::Corrector.new(processed_source.buffer, cop.corrections)
+        .rewrite
+    end
+
+    def investigate(cop, processed_source)
+      RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
+        .investigate(processed_source)
+    end
+end

railties/test/application/rake_test.rb

@@ -41,7 +41,7 @@ module ApplicationTests
         rails "db:create", "db:migrate"
         output = rails("db:test:prepare", "test")
 
-        refute_match(/ActiveRecord::ProtectedEnvironmentError/, output)
+        assert_no_match(/ActiveRecord::ProtectedEnvironmentError/, output)
       end
     end