From 62c07b88e409ef4cfae31bf7d45e614639ff08b1 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 5 Jul 2024 15:23:23 +0200 Subject: [PATCH 1/5] fix(tests): Various fixes - Remove `database_exists?` method from `CockroachDBAdapter`, the original method is fine. - Exclude a test that will be fixed in Rails 7.2 --- .../connection_adapters/cockroachdb_adapter.rb | 6 ------ test/excludes/AttributeMethodsTest.rb | 2 ++ 2 files changed, 2 insertions(+), 6 deletions(-) create mode 100644 test/excludes/AttributeMethodsTest.rb diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 3f6137e6..7a5a3582 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -243,12 +243,6 @@ def supports_deferrable_constraints? # @crdb_version = version_num.to_i # end - def self.database_exists?(config) - !!ActiveRecord::Base.cockroachdb_connection(config) - rescue ActiveRecord::NoDatabaseError - false - end - def initialize(...) super diff --git a/test/excludes/AttributeMethodsTest.rb b/test/excludes/AttributeMethodsTest.rb new file mode 100644 index 00000000..fec54a47 --- /dev/null +++ b/test/excludes/AttributeMethodsTest.rb @@ -0,0 +1,2 @@ +# TODO: Rails 7.2 remove this exclusion +exclude "test_#undefine_attribute_methods_undefines_alias_attribute_methods", "The test will be fixed in 7.2 (https://github.com/rails/rails/commit/a0993f81d0450a191da1ee35282f60fc2899135c)" From dba0c06f274f58a28568e651ac9c41305eae6cf5 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 5 Jul 2024 15:24:29 +0200 Subject: [PATCH 2/5] temp --- test/cases/helper_cockroachdb.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index d0d9602d..9ec21e5a 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -200,3 +200,16 @@ def header(stream) end ActiveRecord::SchemaDumper.prepend(NoHeaderExt) + +def bugmsg(header, stdout) + bugged = begin; print; rescue IOError; "💥 "; end + STDOUT.puts "🤞#{header.rjust(17)}: stdout=#{stdout.inspect} " \ + "#{bugged}(at #{caller_locations(2, 1).first})" +end + +trace_var :$stdout, (proc { bugmsg("$stdout=", _1) }) + +TracePoint.trace(:a_return) do |tp| + next unless tp.method_id == :reopen && tp.self == $stdout + bugmsg("$stdout#reopen", $stdout) +end From 4952e5dc625c2057dd5953b1ecf4f14ff2a9baad Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 10 Jul 2024 16:02:15 +0200 Subject: [PATCH 3/5] verbose --- .github/workflows/ci.yml | 2 +- test/cases/helper_cockroachdb.rb | 13 ------------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8974277..aa459188 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -88,4 +88,4 @@ jobs: done cat ${{ github.workspace }}/setup.sql | cockroach sql --insecure - name: Test - run: bundle exec rake test TESTOPTS='--profile=3' + run: bundle exec rake test TESTOPTS='--profile=3 --verbose' diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index 9ec21e5a..d0d9602d 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -200,16 +200,3 @@ def header(stream) end ActiveRecord::SchemaDumper.prepend(NoHeaderExt) - -def bugmsg(header, stdout) - bugged = begin; print; rescue IOError; "💥 "; end - STDOUT.puts "🤞#{header.rjust(17)}: stdout=#{stdout.inspect} " \ - "#{bugged}(at #{caller_locations(2, 1).first})" -end - -trace_var :$stdout, (proc { bugmsg("$stdout=", _1) }) - -TracePoint.trace(:a_return) do |tp| - next unless tp.method_id == :reopen && tp.self == $stdout - bugmsg("$stdout#reopen", $stdout) -end From 2007de455dd430ecb4c705612fd313727f33d06f Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Fri, 19 Jul 2024 12:49:24 +0200 Subject: [PATCH 4/5] CI tests --- .github/workflows/ci.yml | 2 +- Gemfile | 1 - .../cockroachdb/transaction_manager.rb | 4 ++++ test/cases/connection_adapters/type_test.rb | 9 ++++----- .../PostgresqlTransactionNestedTest.rb | 16 ++++++++++++++-- 5 files changed, 23 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aa459188..31f339c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -88,4 +88,4 @@ jobs: done cat ${{ github.workspace }}/setup.sql | cockroach sql --insecure - name: Test - run: bundle exec rake test TESTOPTS='--profile=3 --verbose' + run: bundle exec rake test TESTOPTS='--profile=3 --verbose' TEST_FILES_AR=test/cases/adapters/postgresql/transaction_nested_test.rb diff --git a/Gemfile b/Gemfile index bb239f29..6ddda894 100644 --- a/Gemfile +++ b/Gemfile @@ -62,7 +62,6 @@ group :development, :test do # Gems used by the ActiveRecord test suite gem "bcrypt", "~> 3.1.18" - gem "mocha", "~> 1.14.0" gem "sqlite3", "~> 1.4.4" gem "minitest", "~> 5.15.0" diff --git a/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb b/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb index 8591f027..63f3e894 100644 --- a/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb +++ b/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb @@ -25,6 +25,7 @@ module TransactionManagerMonkeyPatch def within_new_transaction(isolation: nil, joinable: true, attempts: 0) super(isolation: isolation, joinable: joinable) rescue ActiveRecord::ConnectionNotEstablished => error + puts "IN RETRY CASE ! " * 5 raise unless retryable? error raise if attempts >= @connection.max_transaction_retries @@ -37,6 +38,7 @@ def within_new_transaction(isolation: nil, joinable: true, attempts: 0) within_new_transaction(isolation: isolation, joinable: joinable, attempts: attempts + 1) { yield } rescue ActiveRecord::StatementInvalid => error + puts "IN RETRY CASE ! " * 5 raise unless retryable? error raise if attempts >= @connection.max_transaction_retries @@ -46,6 +48,8 @@ def within_new_transaction(isolation: nil, joinable: true, attempts: 0) end def retryable?(error) + puts "========> In a serialization failure" if error.is_a? ActiveRecord::SerializationFailure + puts "========> In a serialization error" if serialization_error?(error) return true if serialization_error?(error) return true if error.is_a? ActiveRecord::SerializationFailure return retryable? error.cause if error.cause diff --git a/test/cases/connection_adapters/type_test.rb b/test/cases/connection_adapters/type_test.rb index 8b47004a..8aca51cf 100644 --- a/test/cases/connection_adapters/type_test.rb +++ b/test/cases/connection_adapters/type_test.rb @@ -7,10 +7,9 @@ module CockroachDB module ConnectionAdapters class TypeTest < ActiveRecord::TestCase fixtures :accounts - class SqliteModel < ActiveRecord::Base + class FakeModel < ActiveRecord::Base establish_connection( - adapter: "sqlite3", - database: "tmp/some" + adapter: "fake" ) end def test_type_can_be_used_with_various_db @@ -19,8 +18,8 @@ def test_type_can_be_used_with_various_db ActiveRecord::Type.adapter_name_from(Account) ) assert_equal( - :sqlite3, - ActiveRecord::Type.adapter_name_from(SqliteModel) + :fake, + ActiveRecord::Type.adapter_name_from(FakeModel) ) end end diff --git a/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb b/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb index 22838c71..35ae977f 100644 --- a/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb +++ b/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb @@ -1,2 +1,14 @@ -exclude :test_deadlock_raises_Deadlocked_inside_nested_SavepointTransaction, "Causes CI to hand. Skip while debugging." -exclude :test_unserializable_transaction_raises_SerializationFailure_inside_nested_SavepointTransaction, "Causes CI to hand. Skip while debugging." + +# > In CRDB SERIALIZABLE, reads block on in-progress writes for +# > as long as those writes are in progress. However, PG does +# > not have this "read block on write" behavior, and so rather +# > than allowing the left-hand-side to execute, it must instead +# > abort that transaction. Both are valid ways to implement SERIALIZABLE. +# +# See discussion: https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/333 +message = "SERIALIZABLE transactions are different in CockroachDB." + +# exclude :test_deadlock_raises_Deadlocked_inside_nested_SavepointTransaction, message +# exclude :test_unserializable_transaction_raises_SerializationFailure_inside_nested_SavepointTransaction, message +exclude :test_SerializationFailure_inside_nested_SavepointTransaction_is_recoverable, message +exclude :test_deadlock_inside_nested_SavepointTransaction_is_recoverable, message From 2fbb5fc6ac9b6b42f5d495455452390593b7f5fa Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Tue, 27 Aug 2024 10:15:39 +0200 Subject: [PATCH 5/5] cleanup --- .github/workflows/ci.yml | 2 +- .../connection_adapters/cockroachdb/transaction_manager.rb | 4 ---- test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 31f339c2..e8974277 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -88,4 +88,4 @@ jobs: done cat ${{ github.workspace }}/setup.sql | cockroach sql --insecure - name: Test - run: bundle exec rake test TESTOPTS='--profile=3 --verbose' TEST_FILES_AR=test/cases/adapters/postgresql/transaction_nested_test.rb + run: bundle exec rake test TESTOPTS='--profile=3' diff --git a/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb b/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb index 63f3e894..8591f027 100644 --- a/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb +++ b/lib/active_record/connection_adapters/cockroachdb/transaction_manager.rb @@ -25,7 +25,6 @@ module TransactionManagerMonkeyPatch def within_new_transaction(isolation: nil, joinable: true, attempts: 0) super(isolation: isolation, joinable: joinable) rescue ActiveRecord::ConnectionNotEstablished => error - puts "IN RETRY CASE ! " * 5 raise unless retryable? error raise if attempts >= @connection.max_transaction_retries @@ -38,7 +37,6 @@ def within_new_transaction(isolation: nil, joinable: true, attempts: 0) within_new_transaction(isolation: isolation, joinable: joinable, attempts: attempts + 1) { yield } rescue ActiveRecord::StatementInvalid => error - puts "IN RETRY CASE ! " * 5 raise unless retryable? error raise if attempts >= @connection.max_transaction_retries @@ -48,8 +46,6 @@ def within_new_transaction(isolation: nil, joinable: true, attempts: 0) end def retryable?(error) - puts "========> In a serialization failure" if error.is_a? ActiveRecord::SerializationFailure - puts "========> In a serialization error" if serialization_error?(error) return true if serialization_error?(error) return true if error.is_a? ActiveRecord::SerializationFailure return retryable? error.cause if error.cause diff --git a/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb b/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb index 35ae977f..b94764e3 100644 --- a/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb +++ b/test/excludes/ActiveRecord/PostgresqlTransactionNestedTest.rb @@ -8,7 +8,7 @@ # See discussion: https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/333 message = "SERIALIZABLE transactions are different in CockroachDB." -# exclude :test_deadlock_raises_Deadlocked_inside_nested_SavepointTransaction, message -# exclude :test_unserializable_transaction_raises_SerializationFailure_inside_nested_SavepointTransaction, message +exclude :test_deadlock_raises_Deadlocked_inside_nested_SavepointTransaction, message +exclude :test_unserializable_transaction_raises_SerializationFailure_inside_nested_SavepointTransaction, message exclude :test_SerializationFailure_inside_nested_SavepointTransaction_is_recoverable, message exclude :test_deadlock_inside_nested_SavepointTransaction_is_recoverable, message