When asserting queries performed, insert placeholder query for release save-point (#1168)

Author: aidanharan

Diff for: lib/active_record/connection_adapters/sqlserver/savepoints.rb

@@ -16,6 +16,8 @@ def exec_rollback_to_savepoint(name = current_savepoint_name)
           internal_execute("ROLLBACK TRANSACTION #{name}", "TRANSACTION")
         end
 
+        # SQL Server does require save-points to be explicitly released.
+        # See https://stackoverflow.com/questions/3101312/sql-server-2008-no-release-savepoint-for-current-transaction
         def release_savepoint(_name)
         end
       end

Diff for: test/cases/coerced_tests.rb

@@ -243,25 +243,6 @@ def test_update_date_time_attributes_with_default_timezone_local
   end
 end
 
-class HasManyThroughAssociationsTest < ActiveRecord::TestCase
-  # SQL Server does not have query for release_savepoint
-  coerce_tests! :test_associate_existing
-  def test_associate_existing_coerced
-    post   = posts(:thinking)
-    person = people(:david)
-
-    assert_queries_count(2) do
-      post.people << person
-    end
-
-    assert_queries_count(1) do
-      assert_includes post.people, person
-    end
-
-    assert_includes post.reload.people.reload, person
-  end
-end
-
 class BelongsToAssociationsTest < ActiveRecord::TestCase
   # Since @client.firm is a single first/top, and we use FETCH the order clause is used.
   coerce_tests! :test_belongs_to_does_not_use_order_by
@@ -1335,18 +1316,6 @@ def test_registering_new_handlers_for_association_coerced
   end
 end
 
-class PrimaryKeysTest < ActiveRecord::TestCase
-  # SQL Server does not have query for release_savepoint
-  coerce_tests! :test_create_without_primary_key_no_extra_query
-  def test_create_without_primary_key_no_extra_query_coerced
-    klass = Class.new(ActiveRecord::Base) do
-      self.table_name = "dashboards"
-    end
-    klass.create! # warmup schema cache
-    assert_queries_count(2, include_schema: true) { klass.create! }
-  end
-end
-
 require "models/task"
 class QueryCacheTest < ActiveRecord::TestCase
   # SQL Server adapter not in list of supported adapters in original test.
@@ -1633,8 +1602,11 @@ def test_releasing_named_savepoints_coerced
 
       Topic.lease_connection.create_savepoint("another")
       Topic.lease_connection.release_savepoint("another")
-      # We do not have a notion of releasing, so this does nothing vs raise an error.
-      Topic.lease_connection.release_savepoint("another")
+
+      # We do not have a notion of releasing, so this does nothing and doesn't raise an error.
+      assert_nothing_raised do
+        Topic.lease_connection.release_savepoint("another")
+      end
     end
   end
 

Diff for: test/cases/helper_sqlserver.rb

@@ -11,6 +11,7 @@
 require "support/load_schema_sqlserver"
 require "support/coerceable_test_sqlserver"
 require "support/connection_reflection"
+require "support/query_assertions"
 require "mocha/minitest"
 
 module ActiveRecord
@@ -19,7 +20,8 @@ class TestCase < ActiveSupport::TestCase
 
     include ARTest::SQLServer::CoerceableTest,
             ARTest::SQLServer::ConnectionReflection,
-            ActiveSupport::Testing::Stream
+            ActiveSupport::Testing::Stream,
+            ARTest::SQLServer::QueryAssertions
 
     let(:logger) { ActiveRecord::Base.logger }
 

Diff for: test/support/query_assertions.rb

@@ -0,0 +1,29 @@
+module ARTest
+  module SQLServer
+    module QueryAssertions
+      def assert_queries_count(count = nil, include_schema: false, &block)
+        ActiveRecord::Base.lease_connection.materialize_transactions
+
+        counter = ActiveRecord::Assertions::QueryAssertions::SQLCounter.new
+        ActiveSupport::Notifications.subscribed(counter, "sql.active_record") do
+          result = _assert_nothing_raised_or_warn("assert_queries_count", &block)
+          queries = include_schema ? counter.log_all : counter.log
+
+          # Start of monkey-patch
+          # Rails tests expect a save-point to be released at the end of the test. SQL Server does not release
+          # save-points and so the number of queries will be off by one. This monkey patch adds a placeholder query
+          # to the end of the queries array to account for the missing save-point release.
+          queries.append "/* release savepoint placeholder for testing */" if queries.first =~ /SAVE TRANSACTION \S+/
+          # End of monkey-patch
+
+          if count
+            assert_equal count, queries.size, "#{queries.size} instead of #{count} queries were executed. Queries: #{queries.join("\n\n")}"
+          else
+            assert_operator queries.size, :>=, 1, "1 or more queries expected, but none were executed.#{queries.empty? ? '' : "\nQueries:\n#{queries.join("\n")}"}"
+          end
+          result
+        end
+      end
+    end
+  end
+end