Use transactions in JS feature specsjs-specs-transactions

Uses Rails transactional tests instead of DatabaseCleaner
transaction strategy because that doesn't work with JS tests

8 files changed, 85 insertions, 29 deletions

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) }