Enable verbose mode in test and report warnings as errors

We recently let a few very easy to avoid warnings get merged. The root cause is that locally the test suite doesn't run in verbose mode unless you explictly pass `-w`. On CI warnings are enabled, but there is no reason to look at the build output unless something is failing. And even if one wanted to do that, that would be particularly work intensive since warnings may be specific to a Ruby version etc. Because of this I believe we should: - Always run the test suite with warnings enabled. - Raise an error if a warning is unexpected. We've been using this pattern for a long time at Shopify both in private and public repositories.
skipkayhil · Oct 11, 2022 · d917896 · d917896
1 parent ca11431
commit d917896
Show file tree

Hide file tree

Showing 24 changed files with 63 additions and 4 deletions.
diff --git a/actioncable/test/test_helper.rb b/actioncable/test/test_helper.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "action_cable"
 require "active_support/testing/autorun"
 require "active_support/testing/method_call_assertions"

diff --git a/actionmailer/test/abstract_unit.rb b/actionmailer/test/abstract_unit.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_support/core_ext/kernel/reporting"
 
 # These are the normal settings that will be set up by Railties

diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
+
 $:.unshift File.expand_path("lib", __dir__)
 
 require "active_support/core_ext/kernel/reporting"

diff --git a/actionview/lib/action_view/template/handlers/builder.rb b/actionview/lib/action_view/template/handlers/builder.rb
@@ -7,7 +7,8 @@ class Builder
 
       def call(template, source)
         require_engine
-        "xml = ::Builder::XmlMarkup.new(indent: 2, target: output_buffer.raw);" \
+        # the double assignment is to silence "assigned but unused variable" warnings
+        "xml = xml = ::Builder::XmlMarkup.new(indent: 2, target: output_buffer.raw);" \
           "#{source};" \
           "output_buffer.to_s"
       end

diff --git a/actionview/test/abstract_unit.rb b/actionview/test/abstract_unit.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
+
 $:.unshift File.expand_path("lib", __dir__)
 
 ENV["TMPDIR"] = File.expand_path("tmp", __dir__)

diff --git a/activejob/test/helper.rb b/activejob/test/helper.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_job"
 require "support/job_buffer"
 

diff --git a/activemodel/lib/active_model/type/serialize_cast_value.rb b/activemodel/lib/active_model/type/serialize_cast_value.rb
@@ -6,6 +6,7 @@ module SerializeCastValue # :nodoc:
       def self.included(klass)
         unless klass.respond_to?(:included_serialize_cast_value)
           klass.singleton_class.attr_accessor :included_serialize_cast_value
+          klass.silence_redefinition_of_method(:itself_if_class_included_serialize_cast_value)
           klass.attr_reader :itself_if_class_included_serialize_cast_value
         end
         klass.included_serialize_cast_value = true

diff --git a/activemodel/test/cases/helper.rb b/activemodel/test/cases/helper.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_model"
 
 # Show backtraces for deprecated behavior for quicker cleanup.

diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -311,6 +311,7 @@ def initialize(...)
 
         @max_identifier_length = nil
         @type_map = nil
+        @raw_connection = nil
 
         @use_insert_returning = @config.key?(:insert_returning) ? self.class.type_cast_config_to_boolean(@config[:insert_returning]) : true
       end

diff --git a/activerecord/test/cases/adapters/postgresql/statement_pool_test.rb b/activerecord/test/cases/adapters/postgresql/statement_pool_test.rb
@@ -8,6 +8,10 @@ module ActiveRecord
   module ConnectionAdapters
     class PostgreSQLAdapter < AbstractAdapter
       class InactivePgConnection
+        def initialize
+          @raw_connection = nil
+        end
+
         def query(*args)
           raise PG::Error
         end

diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb
@@ -413,7 +413,7 @@ class EnumTest < ActiveRecord::TestCase
     e = assert_raises(ArgumentError) do
       Class.new(ActiveRecord::Base) do
         self.table_name = "books"
-        enum :status, {}
+        enum(:status, {}, **{})
       end
     end
 

diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_support"
 require "active_support/testing/autorun"
 require "active_support/testing/method_call_assertions"

diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
+
 ENV["RAILS_ENV"] ||= "test"
 require_relative "dummy/config/environment.rb"
 

diff --git a/activesupport/lib/active_support/testing/strict_warnings.rb b/activesupport/lib/active_support/testing/strict_warnings.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+$VERBOSE = true
+Warning[:deprecated] = true
+
+module RaiseWarnings
+  PROJECT_ROOT = File.expand_path("../../../../", __dir__)
+  ALLOWED_WARNINGS = Regexp.union(
+    /circular require considered harmful.*delayed_job/, # Bug in delayed job.
+
+    # Expected non-verbose warning emitted by Rails.
+    /Ignoring .*\.yml because it has expired/,
+    /Failed to validate the schema cache because/,
+  )
+  def warn(message, *)
+    super
+
+    return unless message.include?(PROJECT_ROOT)
+    return if ALLOWED_WARNINGS.match?(message)
+    return unless ENV["RAILS_STRICT_WARNINGS"] || ENV["CI"]
+
+    raise message
+  end
+  ruby2_keywords :warn if respond_to?(:ruby2_keywords, true)
+end
+
+Warning.singleton_class.prepend(RaiseWarnings)
diff --git a/activesupport/test/abstract_unit.rb b/activesupport/test/abstract_unit.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
+
 ORIG_ARGV = ARGV.dup
 
 require "bundler/setup"

diff --git a/activesupport/test/deprecation_test.rb b/activesupport/test/deprecation_test.rb
@@ -779,16 +779,19 @@ def assert_callbacks_called_with(matchers = {})
         lambda do |*args|
           message, callstack, deprecator = args
           bindings << binding
+          [message, callstack, deprecator]
         end,
 
         lambda do |message, *other|
           callstack, deprecator = other
           bindings << binding
+          [callstack, deprecator]
         end,
 
         lambda do |message, callstack, *details|
           deprecation_horizon, gem_name = details
           bindings << binding
+          [deprecation_horizon, gem_name]
         end,
       ]
 

diff --git a/railties/lib/rails/command/base.rb b/railties/lib/rails/command/base.rb
@@ -181,6 +181,7 @@ def namespaced_commands
         attr_reader :current_subcommand
 
         def invoke_command(command, *) # :nodoc:
+          @current_subcommand ||= nil
           original_subcommand, @current_subcommand = @current_subcommand, command.name
           super
         ensure

diff --git a/railties/test/abstract_unit.rb b/railties/test/abstract_unit.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
+
 ENV["RAILS_ENV"] ||= "test"
 
 require "stringio"

diff --git a/railties/test/application/initializers/frameworks_test.rb b/railties/test/application/initializers/frameworks_test.rb
@@ -247,7 +247,9 @@ def show
         config.eager_load = true
       RUBY
 
-      require "#{app_path}/config/environment"
+      silence_warnings do
+        require "#{app_path}/config/environment"
+      end
       assert_not ActiveRecord::Base.connection.schema_cache.data_sources("posts")
     end
 

diff --git a/railties/test/application/zeitwerk_integration_test.rb b/railties/test/application/zeitwerk_integration_test.rb
@@ -78,7 +78,7 @@ class RESTfulController < ApplicationController
     end
     RUBY
 
-    app_file "config/initializers/autoload_Y.rb", "Y"
+    app_file "config/initializers/autoload_Y.rb", "Y.succ"
 
     # Preconditions.
     assert_not Object.const_defined?(:X)

diff --git a/railties/test/configuration/middleware_stack_proxy_test.rb b/railties/test/configuration/middleware_stack_proxy_test.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_support"
 require "active_support/testing/autorun"
 require "rails/configuration"

diff --git a/railties/test/generators/argv_scrubber_test.rb b/railties/test/generators/argv_scrubber_test.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_support/test_case"
 require "active_support/testing/autorun"
 require "rails/generators/rails/app/app_generator"

diff --git a/railties/test/generators/generator_test.rb b/railties/test/generators/generator_test.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "active_support/testing/strict_warnings"
 require "active_support/test_case"
 require "active_support/testing/autorun"
 require "rails/generators/app_base"

diff --git a/railties/test/isolation/abstract_unit.rb b/railties/test/isolation/abstract_unit.rb
@@ -8,6 +8,7 @@
 #
 # It is also good to know what is the bare minimum to get
 # Rails booted up.
+require "active_support/testing/strict_warnings"
 require "fileutils"
 require "shellwords"