From d4a919679a1eb5d3e2aaed4b920e6027d2482971 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Fri, 26 Apr 2019 23:49:19 +0800 Subject: Use transactions in JS feature specs Uses Rails transactional tests instead of DatabaseCleaner transaction strategy because that doesn't work with JS tests --- config/environments/test.rb | 2 +- config/initializers/active_record_query_cache.rb | 3 ++ lib/gitlab/database.rb | 25 ++++++++++---- lib/gitlab/patch/active_record_query_cache.rb | 39 ++++++++++++++++++++++ spec/spec_helper.rb | 12 ++++++- spec/support/database_cleaner.rb | 27 +++++---------- spec/support/db_cleaner.rb | 4 +++ .../shared_contexts/email_shared_context.rb | 2 -- 8 files changed, 85 insertions(+), 29 deletions(-) create mode 100644 config/initializers/active_record_query_cache.rb create mode 100644 lib/gitlab/patch/active_record_query_cache.rb diff --git a/config/environments/test.rb b/config/environments/test.rb index e7166882eea..bd35bdd8d5a 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -40,7 +40,7 @@ Rails.application.configure do # Print deprecation notices to the stderr config.active_support.deprecation = :stderr - config.eager_load = false + config.eager_load = true config.cache_store = :null_store diff --git a/config/initializers/active_record_query_cache.rb b/config/initializers/active_record_query_cache.rb new file mode 100644 index 00000000000..61505a1edd3 --- /dev/null +++ b/config/initializers/active_record_query_cache.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +ActiveRecord::ConnectionAdapters::ConnectionPool.prepend Gitlab::Patch::ActiveRecordQueryCache diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 1177f8ea99e..299a8005f42 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -284,17 +284,28 @@ module Gitlab end # inside_transaction? will return true if the caller is running within a transaction. Handles special cases - # when running inside a test environment, in which the entire test is running with a DatabaseCleaner transaction + # when running inside a test environment, where tests may be wrapped in transactions def self.inside_transaction? - ActiveRecord::Base.connection.open_transactions > open_transactions_baseline + if Rails.env.test? + ActiveRecord::Base.connection.open_transactions > open_transactions_baseline + else + ActiveRecord::Base.connection.open_transactions > 0 + end end - def self.open_transactions_baseline - if ::Rails.env.test? - return DatabaseCleaner.connections.count { |conn| conn.strategy.is_a?(DatabaseCleaner::ActiveRecord::Transaction) } - end + # These methods that access @open_transactions_baseline are not thread-safe. + # These are fine though because we only call these in RSpec's main thread. If we decide to run + # specs multi-threaded, we would need to use something like ThreadGroup to keep track of this value + def self.set_open_transactions_baseline + @open_transactions_baseline = ActiveRecord::Base.connection.open_transactions + end + + def self.reset_open_transactions_baseline + @open_transactions_baseline = 0 + end - 0 + def self.open_transactions_baseline + @open_transactions_baseline ||= 0 end private_class_method :open_transactions_baseline diff --git a/lib/gitlab/patch/active_record_query_cache.rb b/lib/gitlab/patch/active_record_query_cache.rb new file mode 100644 index 00000000000..71d66bdbe02 --- /dev/null +++ b/lib/gitlab/patch/active_record_query_cache.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +# Fixes a bug where the query cache isn't aware of the shared +# ActiveRecord connection used in tests +# https://github.com/rails/rails/issues/36587 + +# To be removed with https://gitlab.com/gitlab-org/gitlab-ce/issues/64413 + +module Gitlab + module Patch + module ActiveRecordQueryCache + # rubocop:disable Gitlab/ModuleWithInstanceVariables + def enable_query_cache! + @query_cache_enabled[connection_cache_key(current_thread)] = true + connection.enable_query_cache! if active_connection? + end + + def disable_query_cache! + @query_cache_enabled.delete connection_cache_key(current_thread) + connection.disable_query_cache! if active_connection? + end + + def query_cache_enabled + @query_cache_enabled[connection_cache_key(current_thread)] + end + + def active_connection? + @thread_cached_conns[connection_cache_key(current_thread)] + end + + private + + def current_thread + @lock_thread || Thread.current + end + # rubocop:enable Gitlab/ModuleWithInstanceVariables + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 62fdc039b5e..0f27616a3a3 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -47,7 +47,7 @@ Dir[Rails.root.join("spec/support/**/*.rb")].each { |f| require f } quality_level = Quality::TestLevel.new RSpec.configure do |config| - config.use_transactional_fixtures = false + config.use_transactional_fixtures = true config.use_instantiated_fixtures = false config.fixture_path = Rails.root @@ -289,6 +289,16 @@ RSpec.configure do |config| config.before(:each, :https_pages_disabled) do |_| allow(Gitlab.config.pages).to receive(:external_https).and_return(false) end + + # We can't use an `around` hook here because the wrapping transaction + # is not yet opened at the time that is triggered + config.prepend_before do + Gitlab::Database.set_open_transactions_baseline + end + + config.append_after do + Gitlab::Database.reset_open_transactions_baseline + end end # add simpler way to match asset paths containing digest strings diff --git a/spec/support/database_cleaner.rb b/spec/support/database_cleaner.rb index edd7de94203..f0dd6c52b74 100644 --- a/spec/support/database_cleaner.rb +++ b/spec/support/database_cleaner.rb @@ -26,31 +26,22 @@ RSpec.configure do |config| end config.append_after(:context) do - DatabaseCleaner.clean_with(:deletion, cache_tables: false) + delete_from_all_tables! end - config.before do - setup_database_cleaner - DatabaseCleaner.strategy = :transaction - end + config.around(:each, :delete) do |example| + self.class.use_transactional_tests = false - config.before(:each, :js) do - DatabaseCleaner.strategy = :deletion, { except: deletion_except_tables, cache_tables: false } - end + example.run - config.before(:each, :delete) do - DatabaseCleaner.strategy = :deletion, { except: deletion_except_tables, cache_tables: false } + delete_from_all_tables!(except: deletion_except_tables) end - config.before(:each, :migration) do - DatabaseCleaner.strategy = :deletion, { cache_tables: false } - end + config.around(:each, :migration) do |example| + self.class.use_transactional_tests = false - config.before do - DatabaseCleaner.start - end + example.run - config.append_after do - DatabaseCleaner.clean + delete_from_all_tables! end end diff --git a/spec/support/db_cleaner.rb b/spec/support/db_cleaner.rb index c69fa322073..08622dff6d9 100644 --- a/spec/support/db_cleaner.rb +++ b/spec/support/db_cleaner.rb @@ -1,4 +1,8 @@ module DbCleaner + def delete_from_all_tables!(except: nil) + DatabaseCleaner.clean_with(:deletion, cache_tables: false, except: except) + end + def deletion_except_tables [] end diff --git a/spec/support/shared_contexts/email_shared_context.rb b/spec/support/shared_contexts/email_shared_context.rb index 9d806fc524d..4f5d53f9317 100644 --- a/spec/support/shared_contexts/email_shared_context.rb +++ b/spec/support/shared_contexts/email_shared_context.rb @@ -1,5 +1,3 @@ -require 'gitlab/email/receiver' - shared_context :email_shared_context do let(:mail_key) { "59d8df8370b7e95c5a49fbf86aeb2c93" } let(:receiver) { Gitlab::Email::Receiver.new(email_raw) } -- cgit v1.2.1