From 8e98b614e404eee72a4ad87dc700b6acef62063d Mon Sep 17 00:00:00 2001 From: northeastprince <55164724+northeastprince@users.noreply.github.com> Date: Thu, 1 Feb 2024 17:45:49 -0500 Subject: [PATCH 01/12] Setup jemalloc in default Dockerfile --- railties/CHANGELOG.md | 4 ++++ railties/lib/rails/generators/app_base.rb | 3 +++ .../generators/rails/app/templates/docker-entrypoint.tt | 5 +++++ 3 files changed, 12 insertions(+) diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index df5392242a233..b616ab75018fe 100644 --- a/railties/CHANGELOG.md +++ b/railties/CHANGELOG.md @@ -1,3 +1,7 @@ +* Setup jemalloc in the default Dockerfile for memory optimization. + + *Matt Almeida*, *Jean Boussier* + * Commented out lines in .railsrc file should not be treated as arguments when using rails new generator command. Update ARGVScrubber to ignore text after # symbols. diff --git a/railties/lib/rails/generators/app_base.rb b/railties/lib/rails/generators/app_base.rb index 9116579ad0a5c..1e84be276ca10 100644 --- a/railties/lib/rails/generators/app_base.rb +++ b/railties/lib/rails/generators/app_base.rb @@ -595,6 +595,9 @@ def dockerfile_deploy_packages # ActiveStorage preview support packages << "libvips" unless skip_active_storage? + # jemalloc for memory optimization + packages << "libjemalloc2" + packages.compact.sort end diff --git a/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt b/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt index 0749858f6131c..36a1d58ee1a8b 100755 --- a/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt +++ b/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt @@ -1,5 +1,10 @@ #!/bin/bash -e +# Enable jemalloc for reduced memory usage and reduce latency. +if [ -f /usr/lib/*/libjemalloc.so.2 ]; then + LD_PRELOAD="$(echo /usr/lib/*/libjemalloc.so.2) $LD_PRELOAD" +fi + <% unless skip_active_record? -%> # If running the rails server then create or migrate existing database if [ "${1}" == "./bin/rails" ] && [ "${2}" == "server" ]; then From bbc7ec49fbe11001d46d3ec578e6147264536f5f Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 5 Feb 2024 09:37:27 +0100 Subject: [PATCH 02/12] Improve routes source location detection Followup: https://github.com/rails/rails/pull/50923 Instead of stopping on the first frame that isn't in Action Dispatch, we should return the first frame that isn't filtered by the backtrace cleaner. --- actionpack/lib/action_dispatch/railtie.rb | 1 - .../lib/action_dispatch/routing/mapper.rb | 29 ++++++++++++------- railties/lib/rails/backtrace_cleaner.rb | 6 ++-- railties/test/commands/routes_test.rb | 26 +++++++++++++++++ 4 files changed, 48 insertions(+), 14 deletions(-) diff --git a/actionpack/lib/action_dispatch/railtie.rb b/actionpack/lib/action_dispatch/railtie.rb index 46720ca716a37..ac9b89181e51d 100644 --- a/actionpack/lib/action_dispatch/railtie.rb +++ b/actionpack/lib/action_dispatch/railtie.rb @@ -66,7 +66,6 @@ class Railtie < Rails::Railtie # :nodoc: ActionDispatch::Cookies::CookieJar.always_write_cookie = config.action_dispatch.always_write_cookie ActionDispatch::Routing::Mapper.route_source_locations = Rails.env.development? - ActionDispatch::Routing::Mapper.backtrace_cleaner = Rails.backtrace_cleaner ActionDispatch.test_app = app end diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index b93e77fd0090e..79034bd3293b2 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -10,10 +10,19 @@ module ActionDispatch module Routing class Mapper + class BacktraceCleaner < ActiveSupport::BacktraceCleaner # :nodoc: + def initialize + super + remove_silencers! + add_core_silencer + add_stdlib_silencer + end + end + URL_OPTIONS = [:protocol, :subdomain, :domain, :host, :port] cattr_accessor :route_source_locations, instance_accessor: false, default: false - cattr_accessor :backtrace_cleaner, instance_accessor: false, default: ActiveSupport::BacktraceCleaner.new + cattr_accessor :backtrace_cleaner, instance_accessor: false, default: BacktraceCleaner.new class Constraints < Routing::Endpoint # :nodoc: attr_reader :app, :constraints @@ -366,11 +375,10 @@ def route_source_location Thread.each_caller_location do |location| next if location.path.start_with?(action_dispatch_dir) - if cleaned_path = Mapper.backtrace_cleaner.clean_frame(location.path) - return "#{cleaned_path}:#{location.lineno}" - else - return nil - end + cleaned_path = Mapper.backtrace_cleaner.clean_frame(location.path) + next if cleaned_path.nil? + + return "#{cleaned_path}:#{location.lineno}" end nil end @@ -382,11 +390,10 @@ def route_source_location caller_locations.each do |location| next if location.path.start_with?(action_dispatch_dir) - if cleaned_path = Mapper.backtrace_cleaner.clean_frame(location.path) - return "#{cleaned_path}:#{location.lineno}" - else - return nil - end + cleaned_path = Mapper.backtrace_cleaner.clean_frame(location.path) + next if cleaned_path.nil? + + return "#{cleaned_path}:#{location.lineno}" end nil end diff --git a/railties/lib/rails/backtrace_cleaner.rb b/railties/lib/rails/backtrace_cleaner.rb index 54c273cc1a53d..006d44d4c63e4 100644 --- a/railties/lib/rails/backtrace_cleaner.rb +++ b/railties/lib/rails/backtrace_cleaner.rb @@ -10,9 +10,11 @@ class BacktraceCleaner < ActiveSupport::BacktraceCleaner # :nodoc: def initialize super - @root = "#{Rails.root}/" add_filter do |line| - line.start_with?(@root) ? line.from(@root.size) : line + # We may be called before Rails.root is assigned. + # When that happens we fallback to not truncating. + @root ||= Rails.root && "#{Rails.root}/" + @root && line.start_with?(@root) ? line.from(@root.size) : line end add_filter do |line| if RENDER_TEMPLATE_PATTERN.match?(line) diff --git a/railties/test/commands/routes_test.rb b/railties/test/commands/routes_test.rb index 4a6416b75c032..58ccad151681b 100644 --- a/railties/test/commands/routes_test.rb +++ b/railties/test/commands/routes_test.rb @@ -242,127 +242,153 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase run_routes_command([ "--expanded" ]) end + rails_gem_root = File.expand_path("../../../../", __FILE__) + assert_equal <<~MESSAGE, output --[ Route 1 ]-------------- Prefix | cart Verb | GET URI | /cart(.:format) Controller#Action | cart#show + Source Location | #{app_path}/config/routes.rb:2 --[ Route 2 ]-------------- Prefix | rails_postmark_inbound_emails Verb | POST URI | /rails/action_mailbox/postmark/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/postmark/inbound_emails#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:5 --[ Route 3 ]-------------- Prefix | rails_relay_inbound_emails Verb | POST URI | /rails/action_mailbox/relay/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/relay/inbound_emails#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:6 --[ Route 4 ]-------------- Prefix | rails_sendgrid_inbound_emails Verb | POST URI | /rails/action_mailbox/sendgrid/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/sendgrid/inbound_emails#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:7 --[ Route 5 ]-------------- Prefix | rails_mandrill_inbound_health_check Verb | GET URI | /rails/action_mailbox/mandrill/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/mandrill/inbound_emails#health_check + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:10 --[ Route 6 ]-------------- Prefix | rails_mandrill_inbound_emails Verb | POST URI | /rails/action_mailbox/mandrill/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/mandrill/inbound_emails#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:11 --[ Route 7 ]-------------- Prefix | rails_mailgun_inbound_emails Verb | POST URI | /rails/action_mailbox/mailgun/inbound_emails/mime(.:format) Controller#Action | action_mailbox/ingresses/mailgun/inbound_emails#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:14 --[ Route 8 ]-------------- Prefix | rails_conductor_inbound_emails Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#index + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:19 --[ Route 9 ]-------------- Prefix |#{" "} Verb | POST URI | /rails/conductor/action_mailbox/inbound_emails(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:19 --[ Route 10 ]------------- Prefix | new_rails_conductor_inbound_email Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails/new(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#new + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:19 --[ Route 11 ]------------- Prefix | rails_conductor_inbound_email Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails/:id(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#show + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:19 --[ Route 12 ]------------- Prefix | new_rails_conductor_inbound_email_source Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails/sources/new(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails/sources#new + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:20 --[ Route 13 ]------------- Prefix | rails_conductor_inbound_email_sources Verb | POST URI | /rails/conductor/action_mailbox/inbound_emails/sources(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails/sources#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:21 --[ Route 14 ]------------- Prefix | rails_conductor_inbound_email_reroute Verb | POST URI | /rails/conductor/action_mailbox/:inbound_email_id/reroute(.:format) Controller#Action | rails/conductor/action_mailbox/reroutes#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:23 --[ Route 15 ]------------- Prefix | rails_conductor_inbound_email_incinerate Verb | POST URI | /rails/conductor/action_mailbox/:inbound_email_id/incinerate(.:format) Controller#Action | rails/conductor/action_mailbox/incinerates#create + Source Location | #{rails_gem_root}/actionmailbox/config/routes.rb:24 --[ Route 16 ]------------- Prefix | rails_service_blob Verb | GET URI | /rails/active_storage/blobs/redirect/:signed_id/*filename(.:format) Controller#Action | active_storage/blobs/redirect#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:5 --[ Route 17 ]------------- Prefix | rails_service_blob_proxy Verb | GET URI | /rails/active_storage/blobs/proxy/:signed_id/*filename(.:format) Controller#Action | active_storage/blobs/proxy#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:6 --[ Route 18 ]------------- Prefix |#{" "} Verb | GET URI | /rails/active_storage/blobs/:signed_id/*filename(.:format) Controller#Action | active_storage/blobs/redirect#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:7 --[ Route 19 ]------------- Prefix | rails_blob_representation Verb | GET URI | /rails/active_storage/representations/redirect/:signed_blob_id/:variation_key/*filename(.:format) Controller#Action | active_storage/representations/redirect#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:9 --[ Route 20 ]------------- Prefix | rails_blob_representation_proxy Verb | GET URI | /rails/active_storage/representations/proxy/:signed_blob_id/:variation_key/*filename(.:format) Controller#Action | active_storage/representations/proxy#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:10 --[ Route 21 ]------------- Prefix |#{" "} Verb | GET URI | /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) Controller#Action | active_storage/representations/redirect#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:11 --[ Route 22 ]------------- Prefix | rails_disk_service Verb | GET URI | /rails/active_storage/disk/:encoded_key/*filename(.:format) Controller#Action | active_storage/disk#show + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:13 --[ Route 23 ]------------- Prefix | update_rails_disk_service Verb | PUT URI | /rails/active_storage/disk/:encoded_token(.:format) Controller#Action | active_storage/disk#update + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:14 --[ Route 24 ]------------- Prefix | rails_direct_uploads Verb | POST URI | /rails/active_storage/direct_uploads(.:format) Controller#Action | active_storage/direct_uploads#create + Source Location | #{rails_gem_root}/activestorage/config/routes.rb:15 MESSAGE end From e8786601ec6eebc535cb879e0cf4c2f8af3ddb1a Mon Sep 17 00:00:00 2001 From: Akhil G Krishnan Date: Mon, 5 Feb 2024 23:49:11 +0530 Subject: [PATCH 03/12] Remove codespell from contribution guide --- guides/source/contributing_to_ruby_on_rails.md | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/guides/source/contributing_to_ruby_on_rails.md b/guides/source/contributing_to_ruby_on_rails.md index 51870ff84b236..2c40c7b58b963 100644 --- a/guides/source/contributing_to_ruby_on_rails.md +++ b/guides/source/contributing_to_ruby_on_rails.md @@ -287,16 +287,6 @@ Inspecting 1 file 1 file inspected, no offenses detected ``` -#### Spell Checking - -We run [codespell](https://github.com/codespell-project/codespell) with GitHub Actions to check spelling and -[codespell](https://pypi.org/project/codespell/) runs against a [small custom dictionary](https://github.com/rails/rails/blob/main/codespell.txt). -`codespell` is written in [Python](https://www.python.org/) and you can run it with: - -```bash -$ codespell --ignore-words=codespell.txt -``` - ### Benchmark Your Code For changes that might have an impact on performance, please benchmark your From a87668a2388633bffc6c92d46b25b5766776ce4c Mon Sep 17 00:00:00 2001 From: fatkodima Date: Mon, 5 Feb 2024 15:59:14 +0200 Subject: [PATCH 04/12] Support `:source_location` tag option for query log tags --- activerecord/CHANGELOG.md | 12 ++++++++++++ activerecord/lib/active_record/query_logs.rb | 15 +++++++++++++++ activerecord/lib/active_record/railtie.rb | 3 ++- guides/source/configuring.md | 6 +++--- railties/test/application/query_logs_test.rb | 17 +++++++++++++++++ 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 1c385682b69c7..8a7d13801318d 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,15 @@ +* Support `:source_location` tag option for query log tags + + ```ruby + config.active_record.query_log_tags << :source_location + ``` + + Calculating the caller location is a costly operation and should be used primarily in development + (note, there is also a `config.active_record.verbose_query_logs` that serves the same purpose) + or occasionally on production for debugging purposes. + + *fatkodima* + * Add an option to `ActiveRecord::Encryption::Encryptor` to disable compression Allow compression to be disabled by setting `compress: false` diff --git a/activerecord/lib/active_record/query_logs.rb b/activerecord/lib/active_record/query_logs.rb index 6642c822bcdcc..da97e5c746baf 100644 --- a/activerecord/lib/active_record/query_logs.rb +++ b/activerecord/lib/active_record/query_logs.rb @@ -26,6 +26,7 @@ module ActiveRecord # * +socket+ # * +db_host+ # * +database+ + # * +source_location+ # # Action Controller adds default tags when loaded: # @@ -108,6 +109,20 @@ def update_formatter(format) end end + if Thread.respond_to?(:each_caller_location) + def query_source_location # :nodoc: + Thread.each_caller_location do |location| + frame = LogSubscriber.backtrace_cleaner.clean_frame(location.path) + return frame if frame + end + nil + end + else + def query_source_location # :nodoc: + LogSubscriber.backtrace_cleaner.clean(caller_locations(1).each).first + end + end + ActiveSupport::ExecutionContext.after_change { ActiveRecord::QueryLogs.clear_cache } private diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 63caf32fe05d1..dd279a03decf4 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -415,7 +415,8 @@ class Railtie < Rails::Railtie # :nodoc: pid: -> { Process.pid.to_s }, socket: ->(context) { context[:connection].pool.db_config.socket }, db_host: ->(context) { context[:connection].pool.db_config.host }, - database: ->(context) { context[:connection].pool.db_config.database } + database: ->(context) { context[:connection].pool.db_config.database }, + source_location: -> { QueryLogs.query_source_location } ) ActiveRecord.disable_prepared_statements = true diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 145bf2dc577a1..9145fedb9c8a7 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -1411,9 +1411,9 @@ NOTE: When this is set to `true` database prepared statements will be automatica #### `config.active_record.query_log_tags` -Define an `Array` specifying the key/value tags to be inserted in an SQL -comment. Defaults to `[ :application ]`, a predefined tag returning the -application name. +Define an `Array` specifying the key/value tags to be inserted in an SQL comment. Defaults to +`[ :application, :controller, :action, :job ]`. The available tags are: `:application`, `:controller`, +`:namespaced_controller`, `:action`, `:job`, and `:source_location`. #### `config.active_record.query_log_tags_format` diff --git a/railties/test/application/query_logs_test.rb b/railties/test/application/query_logs_test.rb index 0127a95fa39cf..604f0d5f3f9c5 100644 --- a/railties/test/application/query_logs_test.rb +++ b/railties/test/application/query_logs_test.rb @@ -149,6 +149,23 @@ def app assert_equal("/*action='index',controller='users',database='storage%2Fproduction_animals.sqlite3'*/", comment) end + test "source_location information is added if enabled" do + add_to_config <<~RUBY + config.active_record.query_log_tags_enabled = true + config.active_record.query_log_tags = [ :source_location ] + + # Remove silencers, so we won't get all backtrace lines filtered. + Rails.backtrace_cleaner.remove_silencers! + RUBY + + boot_app + + get "/", {}, { "HTTPS" => "on" } + comment = last_response.body.strip + + assert_match(/source_location='.*'/, comment) + end + test "controller tags are not doubled up if already configured" do add_to_config "config.active_record.query_log_tags_enabled = true" add_to_config "config.active_record.query_log_tags = [ :action, :job, :controller, :pid ]" From 0c9329161c122efd435221a706783c90da752ecb Mon Sep 17 00:00:00 2001 From: Yasuo Honda Date: Tue, 6 Feb 2024 09:59:36 +0900 Subject: [PATCH 05/12] Pin minitest version to 5.21 Managed to reproduce CI failure at https://buildkite.com/rails/rails-nightly/builds/133#018d7bb8-8a32-4978-8e36-d7cb9b067813/1196-1204 . It would also reproduce against the released versions of Ruby because this is triggered by minitest v5.22.0 change. https://github.com/minitest/minitest/commit/ebb468c81c0eea069234127bd6a101c84e955872 To avoid all of railties CI failures, pin minitest version to 5.21 tentatively. * Steps to reproduce ```ruby git clone https://github.com/rails/rails cd rails rm Gemfile.lock bundle install cd railties bin/test test/application/test_runner_test.rb -n test_system_tests_are_not_run_with_the_default_test_command ``` * Expected behavior It should pass. * Actual behavior ```ruby $ bin/test test/application/test_runner_test.rb -n test_system_tests_are_not_run_with_the_default_test_command Run options: -n test_system_tests_are_not_run_with_the_default_test_command --seed 14574 F Failure: ApplicationTests::TestRunnerTest#test_system_tests_are_not_run_with_the_default_test_command [test/application/test_runner_test.rb:1191]: Expected /0\ runs,\ 0\ assertions,\ 0\ failures,\ 0\ errors,\ 0\ skips/ to match "Nothing ran for filter: \nRunning 0 tests in a single process (parallelization threshold is 50)\nRun options: --seed 45713\n\n# Running:\n\n". bin/test test/application/test_runner_test.rb:1179 Finished in 9.982314s, 0.1002 runs/s, 0.2004 assertions/s. 1 runs, 2 assertions, 1 failures, 0 errors, 0 skips $ ``` --- Gemfile | 2 +- Gemfile.lock | 4 ++-- activesupport/activesupport.gemspec | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 0b9d8dcff20df..cf2f1b9f4b4e0 100644 --- a/Gemfile +++ b/Gemfile @@ -3,7 +3,7 @@ source "https://rubygems.org" gemspec -gem "minitest", ">= 5.15.0" +gem "minitest", ">= 5.15.0", "< 5.22.0" # We need a newish Rake since Active Job sets its test tasks' descriptions. gem "rake", ">= 13" diff --git a/Gemfile.lock b/Gemfile.lock index 64c97c5242f37..7ca2f07a10104 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -93,7 +93,7 @@ PATH connection_pool (>= 2.2.5) drb i18n (>= 1.6, < 2) - minitest (>= 5.1) + minitest (>= 5.1, < 5.22.0) tzinfo (~> 2.0, >= 2.0.5) rails (7.2.0.alpha) actioncable (= 7.2.0.alpha) @@ -618,7 +618,7 @@ DEPENDENCIES libxml-ruby listen (~> 3.3) mdl (!= 0.13.0) - minitest (>= 5.15.0) + minitest (>= 5.15.0, < 5.22.0) minitest-bisect minitest-ci minitest-retry diff --git a/activesupport/activesupport.gemspec b/activesupport/activesupport.gemspec index 17fb20f2a4c3d..136a0465680b2 100644 --- a/activesupport/activesupport.gemspec +++ b/activesupport/activesupport.gemspec @@ -38,7 +38,7 @@ Gem::Specification.new do |s| s.add_dependency "tzinfo", "~> 2.0", ">= 2.0.5" s.add_dependency "concurrent-ruby", "~> 1.0", ">= 1.0.2" s.add_dependency "connection_pool", ">= 2.2.5" - s.add_dependency "minitest", ">= 5.1" + s.add_dependency "minitest", ">= 5.1", "< 5.22.0" s.add_dependency "base64" s.add_dependency "drb" s.add_dependency "bigdecimal" From 4873347538caaceee38b1835a7ec9b192701f6e1 Mon Sep 17 00:00:00 2001 From: zzak Date: Tue, 6 Feb 2024 08:02:23 +0900 Subject: [PATCH 06/12] Fix Active Storage test configurations for CI This change is an up-port of #50787 which applied a similar fix to 7-1-stable. Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in #49003). This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to #43705. Co-authored-by: Sean Doyle Co-authored-by: Jean byroot Boussier --- activestorage/Rakefile | 8 ++- .../direct_uploads_controller_test.rb | 62 +++++++++---------- .../test/dummy/config/environments/test.rb | 5 +- 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/activestorage/Rakefile b/activestorage/Rakefile index 0b246564bc3ad..0d71f44595bae 100644 --- a/activestorage/Rakefile +++ b/activestorage/Rakefile @@ -13,11 +13,13 @@ Rake::TestTask.new do |t| end if ENV["encrypted_0fb9444d0374_key"] && ENV["encrypted_0fb9444d0374_iv"] - file "test/service/configurations.yml" do - system "openssl aes-256-cbc -K $encrypted_0fb9444d0374_key -iv $encrypted_0fb9444d0374_iv -in test/service/configurations.yml.enc -out test/service/configurations.yml -d" + config_file = "test/service/configurations.yml" + file config_file do + puts "Generating #{config_file} for Active Storage tests..." + system "openssl aes-256-cbc -K $encrypted_0fb9444d0374_key -iv $encrypted_0fb9444d0374_iv -in #{config_file}.enc -out #{config_file} -d" end - task test: "test/service/configurations.yml" + task test: config_file end task :package diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index 23a30f876da73..3e6cee015d876 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -17,13 +17,13 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionDispatch::Integration test "creating new direct upload" do checksum = OpenSSL::Digest::MD5.base64digest("Hello") metadata = { - "foo": "bar", - "my_key_1": "my_value_1", - "my_key_2": "my_value_2", - "platform": "my_platform", - "library_ID": "12345", - "custom": { - "my_key_3": "my_value_3" + "foo" => "bar", + "my_key_1" => "my_value_1", + "my_key_2" => "my_value_2", + "platform" => "my_platform", + "library_ID" => "12345", + "custom" => { + "my_key_3" => "my_value_3" } } @@ -35,7 +35,7 @@ class ActiveStorage::S3DirectUploadsControllerTest < ActionDispatch::Integration assert_equal "hello.txt", details["filename"] assert_equal 6, details["byte_size"] assert_equal checksum, details["checksum"] - assert_equal metadata, details["metadata"].deep_transform_keys(&:to_sym) + assert_equal metadata, details["metadata"] assert_equal "text/plain", details["content_type"] assert_match SERVICE_CONFIGURATIONS[:s3][:bucket], details["direct_upload"]["url"] assert_match(/s3(-[-a-z0-9]+)?\.(\S+)?amazonaws\.com/, details["direct_upload"]["url"]) @@ -63,25 +63,25 @@ class ActiveStorage::GCSDirectUploadsControllerTest < ActionDispatch::Integratio test "creating new direct upload" do checksum = OpenSSL::Digest::MD5.base64digest("Hello") metadata = { - "foo": "bar", - "my_key_1": "my_value_1", - "my_key_2": "my_value_2", - "platform": "my_platform", - "library_ID": "12345", - "custom": { - "my_key_3": "my_value_3" + "foo" => "bar", + "my_key_1" => "my_value_1", + "my_key_2" => "my_value_2", + "platform" => "my_platform", + "library_ID" => "12345", + "custom" => { + "my_key_3" => "my_value_3" } } post rails_direct_uploads_url, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - @response.parsed_body.tap do |details| + response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) assert_equal "hello.txt", details["filename"] assert_equal 6, details["byte_size"] assert_equal checksum, details["checksum"] - assert_equal metadata, details["metadata"].deep_transform_keys(&:to_sym) + assert_equal metadata, details["metadata"] assert_equal "text/plain", details["content_type"] assert_match %r{storage\.googleapis\.com/#{@config[:bucket]}}, details["direct_upload"]["url"] assert_equal({ "Content-MD5" => checksum, "Content-Disposition" => "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", "x-goog-meta-my_key_3" => "my_value_3" }, details["direct_upload"]["headers"]) @@ -108,22 +108,22 @@ class ActiveStorage::AzureStorageDirectUploadsControllerTest < ActionDispatch::I test "creating new direct upload" do checksum = OpenSSL::Digest::MD5.base64digest("Hello") metadata = { - "foo": "bar", - "my_key_1": "my_value_1", - "my_key_2": "my_value_2", - "platform": "my_platform", - "library_ID": "12345" + "foo" => "bar", + "my_key_1" => "my_value_1", + "my_key_2" => "my_value_2", + "platform" => "my_platform", + "library_ID" => "12345" } post rails_direct_uploads_url, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - @response.parsed_body.tap do |details| + response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) assert_equal "hello.txt", details["filename"] assert_equal 6, details["byte_size"] assert_equal checksum, details["checksum"] - assert_equal metadata, details["metadata"].deep_transform_keys(&:to_sym) + assert_equal metadata, details["metadata"] assert_equal "text/plain", details["content_type"] assert_match %r{#{@config[:storage_account_name]}\.blob\.core\.windows\.net/#{@config[:container]}}, details["direct_upload"]["url"] assert_equal({ "Content-Type" => "text/plain", "Content-MD5" => checksum, "x-ms-blob-content-disposition" => "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", "x-ms-blob-type" => "BlockBlob" }, details["direct_upload"]["headers"]) @@ -148,7 +148,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati post rails_direct_uploads_url, params: { blob: { filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } - @response.parsed_body.tap do |details| + response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"]) assert_equal "hello.txt", details["filename"] assert_equal 6, details["byte_size"] @@ -163,11 +163,11 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati test "creating new direct upload does not include root in json" do checksum = OpenSSL::Digest::MD5.base64digest("Hello") metadata = { - "foo": "bar", - "my_key_1": "my_value_1", - "my_key_2": "my_value_2", - "platform": "my_platform", - "library_ID": "12345" + "foo" => "bar", + "my_key_1" => "my_value_1", + "my_key_2" => "my_value_2", + "platform" => "my_platform", + "library_ID" => "12345" } set_include_root_in_json(true) do @@ -175,7 +175,7 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } end - @response.parsed_body.tap do |details| + response.parsed_body.tap do |details| assert_nil details["blob"] assert_not_nil details["id"] end diff --git a/activestorage/test/dummy/config/environments/test.rb b/activestorage/test/dummy/config/environments/test.rb index fe6223e80f56a..7286f46b43d3c 100644 --- a/activestorage/test/dummy/config/environments/test.rb +++ b/activestorage/test/dummy/config/environments/test.rb @@ -37,9 +37,10 @@ config.active_storage.service = :local SERVICE_CONFIGURATIONS = begin - ActiveSupport::ConfigurationFile.parse(File.expand_path("service/configurations.yml", __dir__)).deep_symbolize_keys + config_file = Rails.root.join("../service/configurations.yml") + ActiveSupport::ConfigurationFile.parse(config_file, symbolize_names: true) rescue Errno::ENOENT - puts "Missing service configuration file in test/service/configurations.yml" + puts "Missing service configuration file in #{config_file}" {} end # Azure service tests are currently failing on the main branch. From 54d3934d439ec92c9bcd204ed09b03502244683e Mon Sep 17 00:00:00 2001 From: Jonathan del Strother Date: Tue, 6 Feb 2024 12:30:07 +0000 Subject: [PATCH 07/12] Fix JSON-encoding ActiveStorage::Filename ActiveStorage::Filename was missing quotes when encoded, generating invalid json like this - ``` JSON.generate(foo: ActiveStorage::Filename.new("bar.pdf") # => '{"foo":bar.pdf}' ``` Delete to_json and rely on the implementation from ActiveSupport::ToJsonWithActiveSupportEncoder --- activestorage/CHANGELOG.md | 4 ++++ activestorage/app/models/active_storage/filename.rb | 4 ---- activestorage/test/models/filename_test.rb | 6 ++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 11e4b98327129..38ef126637eb0 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix JSON-encoding of `ActiveStorage::Filename` instances. + + *Jonathan del Strother* + * Fix N+1 query when fetching preview images for non-image assets *Aaron Patterson & Justin Searls* diff --git a/activestorage/app/models/active_storage/filename.rb b/activestorage/app/models/active_storage/filename.rb index a63f5d9510b28..d79eb608ed7d5 100644 --- a/activestorage/app/models/active_storage/filename.rb +++ b/activestorage/app/models/active_storage/filename.rb @@ -69,10 +69,6 @@ def as_json(*) to_s end - def to_json - to_s - end - def <=>(other) to_s.downcase <=> other.to_s.downcase end diff --git a/activestorage/test/models/filename_test.rb b/activestorage/test/models/filename_test.rb index 715116309f911..86521bde2a998 100644 --- a/activestorage/test/models/filename_test.rb +++ b/activestorage/test/models/filename_test.rb @@ -53,4 +53,10 @@ class ActiveStorage::FilenameTest < ActiveSupport::TestCase test "compare sanitized" do assert_operator ActiveStorage::Filename.new("foo-bar.pdf"), :==, ActiveStorage::Filename.new("foo\tbar.pdf") end + + test "encoding to json" do + assert_equal '"foo.pdf"', ActiveStorage::Filename.new("foo.pdf").to_json + assert_equal '{"filename":"foo.pdf"}', { filename: ActiveStorage::Filename.new("foo.pdf") }.to_json + assert_equal '{"filename":"foo.pdf"}', JSON.generate(filename: ActiveStorage::Filename.new("foo.pdf")) + end end From 932af452e865f80c4f76f9525ff1a274d58da1be Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 6 Feb 2024 15:52:22 +0100 Subject: [PATCH 08/12] Implement `Rails::TestUnitReporter#prerecord` This is invoked by Minitest before invoking the test, allowing to print the test name in advance. This is useful to debug slow and stuck tests by turning on verbose mode. This way the stuck test name is printed before the process deadlock. Otherwise you have to resort to dirty tricks to figure out which test is not returning. This is also how the default Minitest reporter works in verbose mode. --- railties/lib/rails/test_unit/reporter.rb | 10 +++- railties/test/test_unit/reporter_test.rb | 65 +++++++++++++----------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/railties/lib/rails/test_unit/reporter.rb b/railties/lib/rails/test_unit/reporter.rb index d6dffc03eecde..4e8636d88d8c0 100644 --- a/railties/lib/rails/test_unit/reporter.rb +++ b/railties/lib/rails/test_unit/reporter.rb @@ -8,6 +8,13 @@ class TestUnitReporter < Minitest::StatisticsReporter class_attribute :app_root class_attribute :executable, default: "bin/rails test" + def prerecord(test_class, test_name) + super + if options[:verbose] + io.print "%s#%s = " % [test_class.name, test_name] + end + end + def record(result) super @@ -69,8 +76,7 @@ def fail_fast? end def format_line(result) - klass = result.respond_to?(:klass) ? result.klass : result.class - "%s#%s = %.2f s = %s" % [klass, result.name, result.time, result.result_code] + "%.2f s = %s" % [result.time, result.result_code] end def format_rerun_snippet(result) diff --git a/railties/test/test_unit/reporter_test.rb b/railties/test/test_unit/reporter_test.rb index 71f841027a6b0..e7451d55b8731 100644 --- a/railties/test/test_unit/reporter_test.rb +++ b/railties/test/test_unit/reporter_test.rb @@ -15,7 +15,7 @@ def woot; end end test "prints rerun snippet to run a single failed test" do - @reporter.record(failed_test) + record(failed_test) @reporter.report assert_match %r{^bin/rails test .*test/test_unit/reporter_test\.rb:\d+$}, @output.string @@ -23,26 +23,26 @@ def woot; end end test "prints rerun snippet for every failed test" do - @reporter.record(failed_test) - @reporter.record(failed_test) - @reporter.record(failed_test) + record(failed_test) + record(failed_test) + record(failed_test) @reporter.report assert_rerun_snippet_count 3 end test "does not print snippet for successful and skipped tests" do - @reporter.record(passing_test) - @reporter.record(skipped_test) + record(passing_test) + record(skipped_test) @reporter.report assert_no_match "Failed tests:", @output.string assert_rerun_snippet_count 0 end test "prints rerun snippet for skipped tests if run in verbose mode" do - verbose = Rails::TestUnitReporter.new @output, verbose: true - verbose.record(skipped_test) - verbose.report + @reporter = Rails::TestUnitReporter.new @output, verbose: true + record(skipped_test) + @reporter.report assert_rerun_snippet_count 1 end @@ -51,7 +51,7 @@ def woot; end original_executable = Rails::TestUnitReporter.executable begin Rails::TestUnitReporter.executable = "bin/test" - @reporter.record(failed_test) + record(failed_test) @reporter.report assert_match %r{^bin/test .*test/test_unit/reporter_test\.rb:\d+$}, @output.string @@ -61,7 +61,7 @@ def woot; end end test "outputs failures inline" do - @reporter.record(failed_test) + record(failed_test) @reporter.report expect = %r{\AF\n\nFailure:\nTestUnitReporterTest::ExampleTest#woot \[[^\]]+\]:\nboo\n\nbin/rails test test/test_unit/reporter_test\.rb:\d+\n\n\z} @@ -69,7 +69,7 @@ def woot; end end test "outputs errors inline" do - @reporter.record(errored_test) + record(errored_test) @reporter.report expect = %r{\AE\n\nError:\nTestUnitReporterTest::ExampleTest#woot:\nArgumentError: wups\n some_test.rb:4\n\nbin/rails test .*test/test_unit/reporter_test\.rb:\d+\n\n\z} @@ -77,28 +77,28 @@ def woot; end end test "outputs skipped tests inline if verbose" do - verbose = Rails::TestUnitReporter.new @output, verbose: true, output_inline: true - verbose.record(skipped_test) - verbose.report + @reporter = Rails::TestUnitReporter.new @output, verbose: true, output_inline: true + record(skipped_test) + @reporter.report expect = %r{\ATestUnitReporterTest::ExampleTest#woot = 10\.00 s = S\n\n\nSkipped:\nTestUnitReporterTest::ExampleTest#woot \[[^\]]+\]:\nskipchurches, misstemples\n\nbin/rails test test/test_unit/reporter_test\.rb:\d+\n\n\z} assert_match expect, @output.string end test "does not output rerun snippets after run" do - @reporter.record(failed_test) + record(failed_test) @reporter.report assert_no_match "Failed tests:", @output.string end test "fail fast interrupts run on failure" do - fail_fast = Rails::TestUnitReporter.new @output, fail_fast: true + @reporter = Rails::TestUnitReporter.new @output, fail_fast: true interrupt_raised = false # Minitest passes through Interrupt, catch it manually. begin - fail_fast.record(failed_test) + record(failed_test) rescue Interrupt interrupt_raised = true ensure @@ -107,12 +107,12 @@ def woot; end end test "fail fast interrupts run on error" do - fail_fast = Rails::TestUnitReporter.new @output, fail_fast: true + @reporter = Rails::TestUnitReporter.new @output, fail_fast: true interrupt_raised = false # Minitest passes through Interrupt, catch it manually. begin - fail_fast.record(errored_test) + record(errored_test) rescue Interrupt interrupt_raised = true ensure @@ -121,16 +121,16 @@ def woot; end end test "fail fast does not interrupt run skips" do - fail_fast = Rails::TestUnitReporter.new @output, fail_fast: true + @reporter = Rails::TestUnitReporter.new @output, fail_fast: true - fail_fast.record(skipped_test) + record(skipped_test) assert_no_match "Failed tests:", @output.string end test "outputs colored passing results" do @output.stub(:tty?, true) do - colored = Rails::TestUnitReporter.new @output, color: true, output_inline: true - colored.record(passing_test) + @reporter = Rails::TestUnitReporter.new @output, color: true, output_inline: true + record(passing_test) expect = %r{\e\[32m\.\e\[0m} assert_match expect, @output.string @@ -139,8 +139,8 @@ def woot; end test "outputs colored skipped results" do @output.stub(:tty?, true) do - colored = Rails::TestUnitReporter.new @output, color: true, output_inline: true - colored.record(skipped_test) + @reporter = Rails::TestUnitReporter.new @output, color: true, output_inline: true + record(skipped_test) expect = %r{\e\[33mS\e\[0m} assert_match expect, @output.string @@ -149,8 +149,8 @@ def woot; end test "outputs colored failed results" do @output.stub(:tty?, true) do - colored = Rails::TestUnitReporter.new @output, color: true, output_inline: true - colored.record(failed_test) + @reporter = Rails::TestUnitReporter.new @output, color: true, output_inline: true + record(failed_test) expected = %r{\e\[31mF\e\[0m\n\n\e\[31mFailure:\nTestUnitReporterTest::ExampleTest#woot \[test/test_unit/reporter_test.rb:\d+\]:\nboo\n\e\[0m\n\nbin/rails test .*test/test_unit/reporter_test.rb:\d+\n\n} assert_match expected, @output.string @@ -159,8 +159,8 @@ def woot; end test "outputs colored error results" do @output.stub(:tty?, true) do - colored = Rails::TestUnitReporter.new @output, color: true, output_inline: true - colored.record(errored_test) + @reporter = Rails::TestUnitReporter.new @output, color: true, output_inline: true + record(errored_test) expected = %r{\e\[31mE\e\[0m\n\n\e\[31mError:\nTestUnitReporterTest::ExampleTest#woot:\nArgumentError: wups\n some_test.rb:4\n\e\[0m} assert_match expected, @output.string @@ -168,6 +168,11 @@ def woot; end end private + def record(test_result) + @reporter.prerecord(test_result.klass.constantize, test_result.name) + @reporter.record(test_result) + end + def assert_rerun_snippet_count(snippet_count) assert_equal snippet_count, @output.string.scan(%r{^bin/rails test }).size end From b324c221e2fd715f8abd563c1c8c7f9ae8a19451 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 6 Feb 2024 16:57:32 +0100 Subject: [PATCH 09/12] SQLite3AdapterTest: Stop leaking files in fixtures directory --- .../adapters/sqlite3/sqlite3_adapter_test.rb | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb index a3324109e42cc..207dfa17f7d5e 100644 --- a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb +++ b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb @@ -224,27 +224,31 @@ def test_overriding_default_journal_mode_pragma assert_match(/nrecognized journal_mode false/, error.message) else # must use a new, separate database file that hasn't been opened in WAL mode before - with_file_connection(database: "fixtures/journal_mode_test.sqlite3", pragmas: { "journal_mode" => "delete" }) do |conn| - assert_equal [{ "journal_mode" => "delete" }], conn.execute("PRAGMA journal_mode") - end + Dir.mktmpdir do |tmpdir| + database_file = File.join(tmpdir, "journal_mode_test.sqlite3") - with_file_connection(database: "fixtures/journal_mode_test.sqlite3", pragmas: { "journal_mode" => :delete }) do |conn| - assert_equal [{ "journal_mode" => "delete" }], conn.execute("PRAGMA journal_mode") - end + with_file_connection(database: database_file, pragmas: { "journal_mode" => "delete" }) do |conn| + assert_equal [{ "journal_mode" => "delete" }], conn.execute("PRAGMA journal_mode") + end - error = assert_raises(ActiveRecord::StatementInvalid) do - with_file_connection(database: "fixtures/journal_mode_test.sqlite3", pragmas: { "journal_mode" => 0 }) do |conn| - conn.execute("PRAGMA journal_mode") + with_file_connection(database: database_file, pragmas: { "journal_mode" => :delete }) do |conn| + assert_equal [{ "journal_mode" => "delete" }], conn.execute("PRAGMA journal_mode") end - end - assert_match(/unrecognized journal_mode 0/, error.message) - error = assert_raises(ActiveRecord::StatementInvalid) do - with_file_connection(database: "fixtures/journal_mode_test.sqlite3", pragmas: { "journal_mode" => false }) do |conn| - conn.execute("PRAGMA journal_mode") + error = assert_raises(ActiveRecord::StatementInvalid) do + with_file_connection(database: database_file, pragmas: { "journal_mode" => 0 }) do |conn| + conn.execute("PRAGMA journal_mode") + end + end + assert_match(/unrecognized journal_mode 0/, error.message) + + error = assert_raises(ActiveRecord::StatementInvalid) do + with_file_connection(database: database_file, pragmas: { "journal_mode" => false }) do |conn| + conn.execute("PRAGMA journal_mode") + end end + assert_match(/unrecognized journal_mode false/, error.message) end - assert_match(/unrecognized journal_mode false/, error.message) end end From 1ca8c8ea3401ec1a88ea5a5adf56b9c65a6746a6 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 6 Feb 2024 16:59:50 +0100 Subject: [PATCH 10/12] Improve the docker-entrypoint comments Co-Authored-By: Joshua Young --- .../rails/generators/rails/app/templates/docker-entrypoint.tt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt b/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt index 36a1d58ee1a8b..296dd159f84e7 100755 --- a/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt +++ b/railties/lib/rails/generators/rails/app/templates/docker-entrypoint.tt @@ -1,6 +1,6 @@ #!/bin/bash -e -# Enable jemalloc for reduced memory usage and reduce latency. +# Enable jemalloc for reduced memory usage and latency. if [ -f /usr/lib/*/libjemalloc.so.2 ]; then LD_PRELOAD="$(echo /usr/lib/*/libjemalloc.so.2) $LD_PRELOAD" fi From d0d74258547b734623b75c47cefaed071d60eae2 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 6 Feb 2024 17:32:44 +0100 Subject: [PATCH 11/12] Speedup ConnectionPoolTests Most of the time was spent waiting on the default 5 seconds checkout timeout. Reducing in for just these tests saves about 1 minute of run time but more importantly saves me from trying to figure out if my refactoring introduced a deadlock of some sort. Before: `real 1m4.448s` After: `real 0m6.395s` --- .../test/cases/connection_pool_test.rb | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index 9566474073b6e..f84a45054062b 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -12,7 +12,15 @@ def setup @previous_isolation_level = ActiveSupport::IsolatedExecutionState.isolation_level # Keep a duplicate pool so we do not bother others - @db_config = ActiveRecord::Base.connection_pool.db_config + config = ActiveRecord::Base.connection_pool.db_config + @db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new( + config.env_name, + config.name, + config.configuration_hash.merge( + checkout_timeout: 0.2, # Reduce checkout_timeout to speedup tests + ) + ) + @pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new(ActiveRecord::Base, @db_config, :writing, :default) @pool = ConnectionPool.new(@pool_config) @@ -436,7 +444,7 @@ def test_checkout_fairness end # this should wake up the waiting threads one by one in order - conns.each { |conn| @pool.checkin(conn); sleep 0.1 } + conns.each { |conn| @pool.checkin(conn); sleep 0.01 } dispose_held_connections.set threads.each(&:join) @@ -768,11 +776,11 @@ def test_connection_pool_stat with_single_connection_pool do |pool| pool.with_connection do |connection| stats = pool.stat - assert_equal({ size: 1, connections: 1, busy: 1, dead: 0, idle: 0, waiting: 0, checkout_timeout: 5 }, stats) + assert_equal({ size: 1, connections: 1, busy: 1, dead: 0, idle: 0, waiting: 0, checkout_timeout: 0.2 }, stats) end stats = pool.stat - assert_equal({ size: 1, connections: 1, busy: 0, dead: 0, idle: 1, waiting: 0, checkout_timeout: 5 }, stats) + assert_equal({ size: 1, connections: 1, busy: 0, dead: 0, idle: 1, waiting: 0, checkout_timeout: 0.2 }, stats) assert_raise(ThreadError) do new_thread do @@ -782,7 +790,7 @@ def test_connection_pool_stat end stats = pool.stat - assert_equal({ size: 1, connections: 1, busy: 0, dead: 1, idle: 0, waiting: 0, checkout_timeout: 5 }, stats) + assert_equal({ size: 1, connections: 1, busy: 0, dead: 1, idle: 0, waiting: 0, checkout_timeout: 0.2 }, stats) ensure Thread.report_on_exception = original_report_on_exception end From 3e7ba58439a434be5e6894993370a8cef403303e Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 6 Feb 2024 17:59:37 +0100 Subject: [PATCH 12/12] Delete `EncryptionPerformanceTest` On my machine, running the whole Active Record test suite takes `88` seconds, and `40` of these are spent in encryption tests. Some of them also happen to flake because of random blips. I appreciate the care that has been put into ensuring the overhead of encrption was reasonable, but I don't think these tests justify their cost. --- Gemfile | 2 - Gemfile.lock | 2 - activerecord/test/cases/encryption/helper.rb | 42 +---------- .../encryption_performance_test.rb | 42 ----------- .../envelope_encryption_performance_test.rb | 53 -------------- ..._deterministic_queries_performance_test.rb | 23 ------ .../performance/storage_performance_test.rb | 70 ------------------- 7 files changed, 1 insertion(+), 233 deletions(-) delete mode 100644 activerecord/test/cases/encryption/performance/encryption_performance_test.rb delete mode 100644 activerecord/test/cases/encryption/performance/envelope_encryption_performance_test.rb delete mode 100644 activerecord/test/cases/encryption/performance/extended_deterministic_queries_performance_test.rb delete mode 100644 activerecord/test/cases/encryption/performance/storage_performance_test.rb diff --git a/Gemfile b/Gemfile index cf2f1b9f4b4e0..f6fc8c1bc91b4 100644 --- a/Gemfile +++ b/Gemfile @@ -143,8 +143,6 @@ group :test do gem "debug", ">= 1.1.0", require: false end - gem "benchmark-ips" - # Needed for Railties tests because it is included in generated apps. gem "brakeman" end diff --git a/Gemfile.lock b/Gemfile.lock index 7ca2f07a10104..f0cc4b9d35365 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -159,7 +159,6 @@ GEM base64 (0.2.0) bcrypt (3.1.20) beaneater (1.1.3) - benchmark-ips (2.13.0) bigdecimal (3.1.5) bindex (0.8.1) bootsnap (1.17.0) @@ -596,7 +595,6 @@ DEPENDENCIES azure-storage-blob (~> 2.0) backburner bcrypt (~> 3.1.11) - benchmark-ips bootsnap (>= 1.4.4) brakeman capybara (>= 3.39) diff --git a/activerecord/test/cases/encryption/helper.rb b/activerecord/test/cases/encryption/helper.rb index 1913a124d610b..1839bff03e77c 100644 --- a/activerecord/test/cases/encryption/helper.rb +++ b/activerecord/test/cases/encryption/helper.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "cases/helper" -require "benchmark/ips" class ActiveRecord::Fixture prepend ActiveRecord::Encryption::EncryptedFixtures @@ -111,45 +110,6 @@ def assert_invalid_key_cant_read_attribute_with_custom_key_provider(model, attri attribute_type.key_provider.keys = original_keys end end - - module PerformanceHelpers - BENCHMARK_DURATION = 1 - BENCHMARK_WARMUP = 1 - BASELINE_LABEL = "Baseline" - CODE_TO_TEST_LABEL = "Code" - - # Usage: - # - # baseline = -> { } - # - # assert_slower_by_at_most 2, baseline: baseline do - # - # end - def assert_slower_by_at_most(threshold_factor, baseline:, baseline_label: BASELINE_LABEL, code_to_test_label: CODE_TO_TEST_LABEL, duration: BENCHMARK_DURATION, quiet: true, &block_to_test) - GC.start - - result = nil - output, error = capture_io do - result = Benchmark.ips do |x| - x.config(time: duration, warmup: BENCHMARK_WARMUP) - x.report(code_to_test_label, &block_to_test) - x.report(baseline_label, &baseline) - x.compare! - end - end - - baseline_result = result.entries.find { |entry| entry.label == baseline_label } - code_to_test_result = result.entries.find { |entry| entry.label == code_to_test_label } - - times_slower = baseline_result.ips / code_to_test_result.ips - - if !quiet || times_slower >= threshold_factor - puts "#{output}#{error}" - end - - assert times_slower < threshold_factor, "Expecting #{threshold_factor} times slower at most, but got #{times_slower} times slower" - end - end end # We eager load encrypted attribute types as they are declared, so that they pick up the @@ -163,7 +123,7 @@ def assert_slower_by_at_most(threshold_factor, baseline:, baseline_label: BASELI end class ActiveRecord::EncryptionTestCase < ActiveRecord::TestCase - include ActiveRecord::Encryption::EncryptionHelpers, ActiveRecord::Encryption::PerformanceHelpers + include ActiveRecord::Encryption::EncryptionHelpers ENCRYPTION_PROPERTIES_TO_RESET = { config: %i[ primary_key deterministic_key key_derivation_salt store_key_references hash_digest_class diff --git a/activerecord/test/cases/encryption/performance/encryption_performance_test.rb b/activerecord/test/cases/encryption/performance/encryption_performance_test.rb deleted file mode 100644 index 54ac5fc8df298..0000000000000 --- a/activerecord/test/cases/encryption/performance/encryption_performance_test.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -require "cases/encryption/helper" -require "models/book_encrypted" -require "models/post_encrypted" - -class ActiveRecord::Encryption::EncryptionPerformanceTest < ActiveRecord::EncryptionTestCase - fixtures :encrypted_books, :posts - - setup do - ActiveRecord::Encryption.config.support_unencrypted_data = true - end - - test "performance when saving records" do - baseline = -> { create_post_without_encryption } - - assert_slower_by_at_most 1.4, baseline: baseline do - create_post_with_encryption - end - end - - test "reading an encrypted attribute multiple times is as fast as reading a regular attribute" do - unencrypted_post = create_post_without_encryption - baseline = -> { unencrypted_post.reload.title } - - encrypted_post = create_post_with_encryption - assert_slower_by_at_most 1.2, baseline: baseline, duration: 3 do - encrypted_post.reload.title - end - end - - private - def create_post_without_encryption - ActiveRecord::Encryption.without_encryption { create_post_with_encryption } - end - - def create_post_with_encryption - EncryptedPost.create!\ - title: "the Starfleet is here!", - body: "

the Starfleet is here, we are safe now!

" - end -end diff --git a/activerecord/test/cases/encryption/performance/envelope_encryption_performance_test.rb b/activerecord/test/cases/encryption/performance/envelope_encryption_performance_test.rb deleted file mode 100644 index 85b7f0abf7754..0000000000000 --- a/activerecord/test/cases/encryption/performance/envelope_encryption_performance_test.rb +++ /dev/null @@ -1,53 +0,0 @@ -# frozen_string_literal: true - -require "cases/encryption/helper" -require "models/book_encrypted" - -class ActiveRecord::Encryption::EnvelopeEncryptionPerformanceTest < ActiveRecord::EncryptionTestCase - fixtures :encrypted_books - - setup do - ActiveRecord::Encryption.config.support_unencrypted_data = true - @envelope_encryption_key_provider = ActiveRecord::Encryption::EnvelopeEncryptionKeyProvider.new - end - - test "performance when saving records" do - baseline = -> { create_book_without_encryption } - - assert_slower_by_at_most 1.4, baseline: baseline do - with_envelope_encryption do - create_book - end - end - end - - test "reading an encrypted attribute multiple times is as fast as reading a regular attribute" do - with_envelope_encryption do - baseline = -> { encrypted_books(:awdr).created_at } - book = create_book - assert_slower_by_at_most 1.05, baseline: baseline, duration: 3 do - book.name - end - end - end - - private - def create_book_without_encryption - ActiveRecord::Encryption.without_encryption { create_book } - end - - def create_book - EncryptedBook.create! name: "Dune" - end - - def encrypt_unencrypted_book - book = create_book_without_encryption - with_envelope_encryption do - book.encrypt - end - end - - def with_envelope_encryption(&block) - with_key_provider @envelope_encryption_key_provider, &block - end -end diff --git a/activerecord/test/cases/encryption/performance/extended_deterministic_queries_performance_test.rb b/activerecord/test/cases/encryption/performance/extended_deterministic_queries_performance_test.rb deleted file mode 100644 index 2f853c65656ac..0000000000000 --- a/activerecord/test/cases/encryption/performance/extended_deterministic_queries_performance_test.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -require "cases/encryption/helper" -require "models/book_encrypted" - -class ActiveRecord::Encryption::ExtendedDeterministicQueriesPerformanceTest < ActiveRecord::EncryptionTestCase - test "finding with prepared statement caching by deterministically encrypted columns" do - baseline = -> { EncryptedBook.find_by(format: "paperback") } # not encrypted - - assert_slower_by_at_most 1.7, baseline: baseline, duration: 2 do - EncryptedBook.find_by(name: "Agile Web Development with Rails") # encrypted, deterministic - end - end - - test "finding without prepared statement caching by encrypted columns (deterministic)" do - baseline = -> { EncryptedBook.where("id > 0").find_by(format: "paperback") } # not encrypted - - # Overhead is 1.1 with SQL - assert_slower_by_at_most 1.3, baseline: baseline, duration: 2 do - EncryptedBook.where("id > 0").find_by(name: "Agile Web Development with Rails") # encrypted, deterministic - end - end -end diff --git a/activerecord/test/cases/encryption/performance/storage_performance_test.rb b/activerecord/test/cases/encryption/performance/storage_performance_test.rb deleted file mode 100644 index d777ec9f15464..0000000000000 --- a/activerecord/test/cases/encryption/performance/storage_performance_test.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -require "cases/encryption/helper" - -class ActiveRecord::Encryption::StoragePerformanceTest < ActiveRecord::EncryptionTestCase - test "storage overload without storing keys is acceptable" do - assert_storage_performance size: 2, overload_less_than: 47 - assert_storage_performance size: 50, overload_less_than: 4 - assert_storage_performance size: 255, overload_less_than: 1.6 - assert_storage_performance size: 1.kilobyte, overload_less_than: 1.15 - - [500.kilobytes, 1.megabyte, 10.megabyte].each do |size| - assert_storage_performance size: size, overload_less_than: 1.03 - end - end - - test "storage overload storing keys is acceptable for DerivedSecretKeyProvider" do - ActiveRecord::Encryption.config.store_key_references = true - - ActiveRecord::Encryption.with_encryption_context key_provider: ActiveRecord::Encryption::DerivedSecretKeyProvider.new("some secret") do - assert_storage_performance size: 2, overload_less_than: 54 - assert_storage_performance size: 50, overload_less_than: 3.5 - assert_storage_performance size: 255, overload_less_than: 1.64 - assert_storage_performance size: 1.kilobyte, overload_less_than: 1.18 - - [500.kilobytes, 1.megabyte, 10.megabyte].each do |size| - assert_storage_performance size: size, overload_less_than: 1.03 - end - end - end - - test "storage overload storing keys is acceptable for EnvelopeEncryptionKeyProvider" do - ActiveRecord::Encryption.config.store_key_references = true - - with_envelope_encryption do - assert_storage_performance size: 2, overload_less_than: 126 - assert_storage_performance size: 50, overload_less_than: 6.28 - assert_storage_performance size: 255, overload_less_than: 2.3 - assert_storage_performance size: 1.kilobyte, overload_less_than: 1.3 - - [500.kilobytes, 1.megabyte, 10.megabyte].each do |size| - assert_storage_performance size: size, overload_less_than: 1.015 - end - end - end - - private - def assert_storage_performance(size:, overload_less_than:, quiet: true) - clear_content = SecureRandom.urlsafe_base64(size).first(size) # .alphanumeric is very slow for large sizes - encrypted_content = encryptor.encrypt(clear_content) - - overload_factor = encrypted_content.bytesize.to_f / clear_content.bytesize - - if !quiet || overload_factor > overload_less_than - puts "#{clear_content.bytesize}; #{encrypted_content.bytesize}; #{(encrypted_content.bytesize / clear_content.bytesize.to_f)}" - end - - assert\ - overload_factor <= overload_less_than, - "Expecting a storage overload of #{overload_less_than} at most for #{size} bytes, but got #{overload_factor} instead" - end - - def encryptor - @encryptor ||= ActiveRecord::Encryption::Encryptor.new - end - - def cipher - @cipher ||= ActiveRecord::Encryption::Cipher.new - end -end