From d917896f45ad680f43036d8ff6d48d7e5e198d23 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 4 Oct 2022 11:13:21 +0200 Subject: [PATCH] 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. --- actioncable/test/test_helper.rb | 1 + actionmailer/test/abstract_unit.rb | 1 + actionpack/test/abstract_unit.rb | 2 ++ .../action_view/template/handlers/builder.rb | 3 ++- actionview/test/abstract_unit.rb | 2 ++ activejob/test/helper.rb | 1 + .../active_model/type/serialize_cast_value.rb | 1 + activemodel/test/cases/helper.rb | 1 + .../connection_adapters/postgresql_adapter.rb | 1 + .../postgresql/statement_pool_test.rb | 4 +++ activerecord/test/cases/enum_test.rb | 2 +- activerecord/test/cases/test_case.rb | 1 + activestorage/test/test_helper.rb | 2 ++ .../active_support/testing/strict_warnings.rb | 27 +++++++++++++++++++ activesupport/test/abstract_unit.rb | 2 ++ activesupport/test/deprecation_test.rb | 3 +++ railties/lib/rails/command/base.rb | 1 + railties/test/abstract_unit.rb | 2 ++ .../initializers/frameworks_test.rb | 4 ++- .../application/zeitwerk_integration_test.rb | 2 +- .../middleware_stack_proxy_test.rb | 1 + .../test/generators/argv_scrubber_test.rb | 1 + railties/test/generators/generator_test.rb | 1 + railties/test/isolation/abstract_unit.rb | 1 + 24 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 activesupport/lib/active_support/testing/strict_warnings.rb diff --git a/actioncable/test/test_helper.rb b/actioncable/test/test_helper.rb index 7b1d600eb4..9f90fd1e8c 100644 --- 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 index 75ad14adbe..5abed4b24f 100644 --- 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 index a05adbed3f..f5e1daa751 100644 --- 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 index 2e1371c015..4ae350fbfa 100644 --- 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 index 15e4c02244..18ab12a63c 100644 --- 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 index 2b1aac8875..353e6f6f7d 100644 --- 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 index 57dd97dbf0..d06c26e6d1 100644 --- 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 index 8fdb117da0..c049f9496b 100644 --- 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 index 4060826854..2de5d21e9c 100644 --- 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 index 373c723c6f..f0c5dbc2d2 100644 --- 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 index 2ca1e961ec..8cf862e9b7 100644 --- 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 index 8a6ce32b44..b20231c4a2 100644 --- 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 index 6f92e787c3..5cea4dcfea 100644 --- 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 new file mode 100644 index 0000000000..9063960c1e --- /dev/null +++ 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 index 8cccea0b00..f55b224cf9 100644 --- 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 index 79d6d83cc1..79f80542d3 100644 --- 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 index bb6d0893fe..547eb8c1a0 100644 --- 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 index bb671c4953..1de8fbdc83 100644 --- 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 index efdef657d3..f77223a889 100644 --- 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 index fc1d204a98..dc05689740 100644 --- 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 index 6131cdd690..330af8910a 100644 --- 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 index f72cb353e0..4f6d97b114 100644 --- 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 index 281d5a5572..6d5d89ba23 100644 --- 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 index d6c9849345..2142f1fac3 100644 --- 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"