From 27344cff2aac02e13f0a97ab1cb62259b6f03ace Mon Sep 17 00:00:00 2001 From: Vasily Fedoseyev Date: Thu, 19 Dec 2024 22:43:57 +0300 Subject: [PATCH] More robust tests --- spec/connection_adapters_spec.rb | 15 +- spec/seamless_database_pool_adapter_spec.rb | 172 ++++++++++-------- .../connection_adapters/read_only_adapter.rb | 26 ++- 3 files changed, 124 insertions(+), 89 deletions(-) diff --git a/spec/connection_adapters_spec.rb b/spec/connection_adapters_spec.rb index 668d244..297336b 100644 --- a/spec/connection_adapters_spec.rb +++ b/spec/connection_adapters_spec.rb @@ -77,7 +77,7 @@ found = model.find(record_id) expect(found.value).to eq 1 connection.master_connection.update("UPDATE #{model.table_name} SET value = 0 WHERE id = #{record_id}") - pending "rails 7.1 seems to invalidate cache" if (Rails::VERSION::MAJOR*10 + Rails::VERSION::MINOR) >= 71 + pending "rails 7.1 seems to invalidate cache" if ActiveRecord.gem_version >= '7.1' expect(model.find(record_id).value).to eq 1 end end @@ -229,11 +229,20 @@ it 'should properly dump the schema' do with_driver = StringIO.new - ActiveRecord::SchemaDumper.dump(connection, with_driver) + conn_pool, master_pool = if ActiveRecord.gem_version >= '7.2' + [ + double('connection_poool').tap { allow(_1).to receive(:with_connection).and_yield(connection) }, + double('master_poool').tap { allow(_1).to receive(:with_connection).and_yield(master_connection) } + ] + else + [connection, master_connection] + end + ActiveRecord::SchemaDumper.dump(conn_pool, with_driver) without_driver = StringIO.new - ActiveRecord::SchemaDumper.dump(master_connection, without_driver) + ActiveRecord::SchemaDumper.dump(master_pool, without_driver) + expect(with_driver.string).to be_present expect(with_driver.string).to eq without_driver.string end diff --git a/spec/seamless_database_pool_adapter_spec.rb b/spec/seamless_database_pool_adapter_spec.rb index 289e0e8..85aee31 100755 --- a/spec/seamless_database_pool_adapter_spec.rb +++ b/spec/seamless_database_pool_adapter_spec.rb @@ -33,6 +33,26 @@ def execute(sql, name = nil); end def columns(table_name, name = nil); end end end +module ActiveRecord + module ConnectionHandling # :nodoc: + { + writer: SeamlessDatabasePool::MockMasterConnection, + reader: SeamlessDatabasePool::MockConnection, + }.each do |mock_adapter, mock_class| + # legacy + define_method(:"#{mock_adapter}_connection") { |*args| mock_class.new(*args) } + define_method(:"#{mock_adapter}_adapter_class") { mock_class } + # rails 7.2+ + if ConnectionAdapters.respond_to?(:register) + ConnectionAdapters.register( + mock_adapter, mock_class.name, + # this will be required on instantiate, but mocks are already loaded + "active_record/connection_adapters/seamless_database_pool_adapter" + ) + end + end + end +end describe 'SeamlessDatabasePoolAdapter ActiveRecord::Base extension' do it 'should establish the connections in the pool merging global options into the connection options' do @@ -50,36 +70,27 @@ def columns(table_name, name = nil); end { 'host' => 'read_host_3', 'pool_weight' => '0' } ] } - - pool_connection = double(:connection) - master_connection = SeamlessDatabasePool::MockConnection.new('master') - read_connection1 = SeamlessDatabasePool::MockConnection.new('read_1') - read_connection2 = SeamlessDatabasePool::MockConnection.new('read_2') - logger = ActiveRecord::Base.logger - weights = { master_connection => 1, read_connection1 => 1, read_connection2 => 2 } - - expect(ActiveRecord::Base).to receive(:writer_connection).with( + + expect(SeamlessDatabasePool::MockMasterConnection).to receive(:new).with( { 'adapter' => 'writer', 'host' => 'master_host', 'username' => 'user', 'pool_weight' => 1 } - ).and_return(master_connection) - expect(ActiveRecord::Base).to receive(:reader_connection).with( + ).and_call_original + expect(SeamlessDatabasePool::MockConnection).to receive(:new).with( { 'adapter' => 'reader', 'host' => 'read_host_1', 'username' => 'user', 'pool_weight' => 1 } - ).and_return(read_connection1) - expect(ActiveRecord::Base).to receive(:reader_connection).with( + ).and_call_original + expect(SeamlessDatabasePool::MockConnection).to receive(:new).with( { 'adapter' => 'reader', 'host' => 'read_host_2', 'username' => 'user', 'pool_weight' => 2 } - ).and_return(read_connection2) + ).and_call_original - klass = double(:class) - expect( - ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter - ).to receive(:adapter_class).with(master_connection).and_return(klass) - expect(klass).to receive(:new).with(nil, logger, master_connection, [read_connection1, read_connection2], - weights, options).and_return(pool_connection) + expect(ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter).to( + receive(:adapter_class).with(SeamlessDatabasePool::MockMasterConnection).and_call_original + ) expect(ActiveRecord::Base).to receive(:establish_adapter).with('writer') expect(ActiveRecord::Base).to receive(:establish_adapter).with('reader').twice - conn = ActiveRecord::Base.seamless_database_pool_connection(options) - expect(conn).to eq pool_connection + conn = ActiveRecord::Base.seamless_database_pool_connection(options) # TODO: use establish_conne + # expect(conn).to eq seamless_database_pool_connection + expect(conn).to be_a(ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter) end it 'should support urls in config' do @@ -92,40 +103,26 @@ def columns(table_name, name = nil); end }, read_pool: [ { 'host' => 'read_host_1' }, - { 'host' => 'read_host_2', 'pool_weight' => '2' }, + { 'url' => 'reader://read-host-2?pool_weight=2' }, { 'url' => 'reader://read-host-3', 'pool_weight' => '0' } ] } - pool_connection = double(:connection) - master_connection = SeamlessDatabasePool::MockConnection.new('master') - read_connection1 = SeamlessDatabasePool::MockConnection.new('read_1') - read_connection2 = SeamlessDatabasePool::MockConnection.new('read_2') - logger = ActiveRecord::Base.logger - weights = { master_connection => 1, read_connection1 => 1, read_connection2 => 2 } - - expect(ActiveRecord::Base).to receive(:writer_connection).with( + expect(SeamlessDatabasePool::MockMasterConnection).to receive(:new).with( { 'adapter' => 'writer', 'host' => 'master-host', 'username' => 'user', 'pool_weight' => 1 } - ).and_return(master_connection) - expect(ActiveRecord::Base).to receive(:reader_connection).with( + ).and_call_original + expect(SeamlessDatabasePool::MockConnection).to receive(:new).with( { 'adapter' => 'reader', 'host' => 'read_host_1', 'username' => 'user', 'pool_weight' => 1 } - ).and_return(read_connection1) - expect(ActiveRecord::Base).to receive(:reader_connection).with( - { 'adapter' => 'reader', 'host' => 'read_host_2', 'username' => 'user', 'pool_weight' => 2 } - ).and_return(read_connection2) - - klass = double(:class) - expect( - ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter - ).to receive(:adapter_class).with(master_connection).and_return(klass) - expect(klass).to receive(:new).with(nil, logger, master_connection, [read_connection1, read_connection2], - weights, options).and_return(pool_connection) + ).and_call_original + expect(SeamlessDatabasePool::MockConnection).to receive(:new).with( + { 'adapter' => 'reader', 'host' => 'read-host-2', 'username' => 'user', 'pool_weight' => 2 } + ).and_call_original expect(ActiveRecord::Base).to receive(:establish_adapter).with('writer') expect(ActiveRecord::Base).to receive(:establish_adapter).with('reader').twice conn = ActiveRecord::Base.seamless_database_pool_connection(options) - expect(conn).to eq pool_connection + expect(conn).to be_a(ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter) end it 'should raise an error if the adapter would be recursive' do @@ -136,19 +133,36 @@ def columns(table_name, name = nil); end end describe 'SeamlessDatabasePoolAdapter' do - let(:master_connection) { SeamlessDatabasePool::MockMasterConnection.new('master') } - let(:read_connection1) { SeamlessDatabasePool::MockConnection.new('read_1') } - let(:read_connection2) { SeamlessDatabasePool::MockConnection.new('read_2') } - let(:config) { {} } + let!(:master_connection) { SeamlessDatabasePool::MockMasterConnection.new(host: 'master') } + let!(:read_connection1) { SeamlessDatabasePool::MockConnection.new(host: 'read_1') } + let!(:read_connection2) { SeamlessDatabasePool::MockConnection.new(host: 'read_2') } + let(:config) do + { + master: { url: 'writer://master' }, + read_pool: [ + { url: 'reader://read-1' }, + { url: 'reader://read-2?pool_weight=2' }, + ] + } + end let(:pool_connection) do - weights = { master_connection => 1, read_connection1 => 1, read_connection2 => 2 } - connection_class = ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter.adapter_class(master_connection) - connection_class.new(nil, nil, master_connection, [read_connection1, read_connection2], weights, config) + allow(SeamlessDatabasePool::MockMasterConnection).to receive(:new).with( + hash_including({ 'adapter' => 'writer', 'host' => 'master'}) + ).and_return(master_connection) + allow(SeamlessDatabasePool::MockConnection).to receive(:new).with( + hash_including({ 'adapter' => 'reader', 'host' => 'read-1' }) + ).and_return(read_connection1) + allow(SeamlessDatabasePool::MockConnection).to receive(:new).with( + hash_including({ 'adapter' => 'reader', 'host' => 'read-2' }) + ).and_return(read_connection2) + # ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter.new(config) + ActiveRecord::Base.seamless_database_pool_connection(config) end it 'should be able to be converted to a string' do expect(pool_connection.to_s).to match( - /\A#\z/ + # TODO: MockMasterConnection + /\A#\z/ ) expect(pool_connection.inspect).to eq pool_connection.to_s end @@ -181,33 +195,35 @@ def columns(table_name, name = nil); end expect(pool_connection.random_read_connection).to eq master_connection end - it 'should use the master connection in a block' do - connection_class = ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter.adapter_class(master_connection) - connection = connection_class.new(nil, double(:logger), master_connection, [read_connection1], - { read_connection1 => 1 }, config) - expect(connection.random_read_connection).to eq read_connection1 - connection.use_master_connection do - expect(connection.random_read_connection).to eq master_connection + context "when master is not in read pool" do + let(:config) do + { master: { url: 'writer://master', pool_weight: 0 }, read_pool: [{ url: 'reader://read-1' }] } end - expect(connection.random_read_connection).to eq read_connection1 - end - - it 'should use the master connection inside a transaction' do - connection_class = ActiveRecord::ConnectionAdapters::SeamlessDatabasePoolAdapter.adapter_class(master_connection) - connection = connection_class.new(nil, double(:logger), master_connection, [read_connection1], - { read_connection1 => 1 }, config) - expect(master_connection).to receive(:begin_db_transaction) - expect(master_connection).to receive(:commit_db_transaction) - expect(master_connection).to receive(:select).with('Transaction SQL', nil) - expect(read_connection1).to receive(:select).with('SQL 1', nil) - expect(master_connection).to receive(:select).with('SQL 2', nil) - - SeamlessDatabasePool.use_persistent_read_connection do - connection.send(:select, 'SQL 1', nil) - connection.transaction do - connection.send(:select, 'Transaction SQL', nil) + + it 'should use the master connection in a block' do + connection = pool_connection + expect(connection.random_read_connection).to eq read_connection1 + connection.use_master_connection do + expect(connection.random_read_connection).to eq master_connection + end + expect(connection.random_read_connection).to eq read_connection1 + end + + it 'should use the master connection inside a transaction' do + connection = pool_connection + expect(master_connection).to receive(:begin_db_transaction) + expect(master_connection).to receive(:commit_db_transaction) + expect(master_connection).to receive(:select).with('Transaction SQL', nil) + expect(read_connection1).to receive(:select).with('SQL 1', nil) + expect(master_connection).to receive(:select).with('SQL 2', nil) + + SeamlessDatabasePool.use_persistent_read_connection do + connection.send(:select, 'SQL 1', nil) + connection.transaction do + connection.send(:select, 'Transaction SQL', nil) + end + connection.send(:select, 'SQL 2', nil) end - connection.send(:select, 'SQL 2', nil) end end end diff --git a/spec/test_adapter/active_record/connection_adapters/read_only_adapter.rb b/spec/test_adapter/active_record/connection_adapters/read_only_adapter.rb index 323b4e4..f5cb4e3 100644 --- a/spec/test_adapter/active_record/connection_adapters/read_only_adapter.rb +++ b/spec/test_adapter/active_record/connection_adapters/read_only_adapter.rb @@ -1,11 +1,16 @@ # frozen_string_literal: true module ActiveRecord - class Base - def self.read_only_connection(config) - real_adapter = config.delete('real_adapter') - connection = send("#{real_adapter}_connection", config.merge('adapter' => real_adapter)) - ConnectionAdapters::ReadOnlyAdapter.new(connection) + module ConnectionHandling + def read_only_connection(config) + ConnectionAdapters::ReadOnlyAdapter.new(config) + end + + if ConnectionAdapters.respond_to?(:register) + ConnectionAdapters.register( + "read_only", "ActiveRecord::ConnectionAdapters::ReadOnlyAdapter", + "active_record/connection_adapters/read_only_adapter" + ) end end @@ -32,10 +37,15 @@ def #{write_method}(*args, **kwargs, &block) RUBY end - def initialize(connection) - @connection = connection + def initialize(config) + real_adapter = config.delete('real_adapter') + @connection = if ActiveRecord::ConnectionAdapters.respond_to?(:resolve) # rails 7.2 + ActiveRecord::ConnectionAdapters.resolve(real_adapter).new(config.merge('adapter' => real_adapter)) + else + ActiveRecord::Base.send(:"#{real_adapter}_connection", config.merge('adapter' => real_adapter)) + end @connected = true - super + super(@connection, nil, config) end def test_select