From 7af242029e4c111510c37bf32911e3749414a51c Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Thu, 12 Dec 2024 15:43:57 -0500 Subject: [PATCH 1/2] Allow the rails method to be called, don't expect it In rails 7.0, it's called 10 times plus 1 more on the next line. In rails 7.1+, it's called only once. From a test perspective, we don't care if it's even called. We only care that the SQL query is executed with the scramsha password or not. To change the username/password only in calls from the migration itself and not from rails, we hack around the caller_locations to make the change only for the migration callers. With this, I believe we can expect test success on rails 7.1 and 7.2. It was added in 742841cf8954be1cb5843dca33945b5fc001cf8d but I think this change emphasizes what we really care about testing and removes an assertion on internals that we don't really have interest in testing. Fixes #768 --- .github/workflows/ci.yaml | 1 - ...0241017013023_reencrypt_password_scramsha_spec.rb | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 951e0b7b..2fbd5e8f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -48,7 +48,6 @@ jobs: run: bin/setup - name: Run tests run: bundle exec rake - continue-on-error: ${{ matrix.rails-version == '7.2' || matrix.rails-version == '7.1'}} - name: Report code coverage if: ${{ github.ref == 'refs/heads/master' && matrix.ruby-version == '3.3' && matrix.rails-version == '7.0' }} continue-on-error: true diff --git a/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb b/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb index 5bacb2f7..eac7c2db 100644 --- a/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb +++ b/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb @@ -9,9 +9,9 @@ username = ActiveRecord::Base.connection_db_config.configuration_hash[:username] - expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).exactly(10).times.and_call_original - expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| - original_method.call(*args, &block).dup.tap { |i| i[:password] ||= "abc" } + allow(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| + # set a value for any calls originating from the migration file, not from rails itself + original_method.call(*args, &block).dup.tap {|i| i[:password] ||= "abc" if caller_locations.any? {|loc| loc.path.include?("db/migrate/20241017013023_reencrypt_password_scramsha.rb")} } end expect(ActiveRecord::Base.connection).to receive(:execute).with(a_string_matching(/ALTER ROLE #{username} WITH PASSWORD \'SCRAM-SHA-256.*\'\;/)) @@ -23,9 +23,9 @@ username = ActiveRecord::Base.connection_db_config.configuration_hash[:username] - expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).exactly(10).times.and_call_original - expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| - original_method.call(*args, &block).dup.tap { |i| i.delete(:password) } + allow(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| + # set a value for any calls originating from the migration file, not from rails itself + original_method.call(*args, &block).dup.tap { |i| i.delete(:password) if caller_locations.any? {|loc| loc.path.include?("db/migrate/20241017013023_reencrypt_password_scramsha.rb")} } end expect(ActiveRecord::Base.connection).not_to receive(:execute).with(a_string_matching(/ALTER ROLE.*\'\;/)) From 0f2cd766271ee860ab0619b740bca937a230360d Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Fri, 3 Jan 2025 15:18:10 -0500 Subject: [PATCH 2/2] Extract migration_path method Changed the behavior as require_migration expected to be called from the spec, but this method could be called from the helper or elsewhere. Therefore, we detect the first caller location with the spec instead of assuming it's the first one. --- .../20241017013023_reencrypt_password_scramsha_spec.rb | 4 ++-- spec/support/migration_name_helper.rb | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb b/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb index eac7c2db..e11f2363 100644 --- a/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb +++ b/spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb @@ -11,7 +11,7 @@ allow(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| # set a value for any calls originating from the migration file, not from rails itself - original_method.call(*args, &block).dup.tap {|i| i[:password] ||= "abc" if caller_locations.any? {|loc| loc.path.include?("db/migrate/20241017013023_reencrypt_password_scramsha.rb")} } + original_method.call(*args, &block).dup.tap { |i| i[:password] ||= "abc" if caller_locations.any? {|loc| loc.path.include?(migration_path)} } end expect(ActiveRecord::Base.connection).to receive(:execute).with(a_string_matching(/ALTER ROLE #{username} WITH PASSWORD \'SCRAM-SHA-256.*\'\;/)) @@ -25,7 +25,7 @@ allow(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block| # set a value for any calls originating from the migration file, not from rails itself - original_method.call(*args, &block).dup.tap { |i| i.delete(:password) if caller_locations.any? {|loc| loc.path.include?("db/migrate/20241017013023_reencrypt_password_scramsha.rb")} } + original_method.call(*args, &block).dup.tap { |i| i.delete(:password) if caller_locations.any? {|loc| loc.path.include?(migration_path)} } end expect(ActiveRecord::Base.connection).not_to receive(:execute).with(a_string_matching(/ALTER ROLE.*\'\;/)) diff --git a/spec/support/migration_name_helper.rb b/spec/support/migration_name_helper.rb index 6e264dd8..a701b882 100644 --- a/spec/support/migration_name_helper.rb +++ b/spec/support/migration_name_helper.rb @@ -1,6 +1,8 @@ def require_migration - spec_name = caller_locations.first.path - migration_name = spec_name.sub("spec/migrations", "db/migrate").sub("_spec.rb", ".rb") + require migration_path +end - require migration_name +def migration_path + spec_name = caller_locations.detect {|loc| loc.path.end_with?("_spec.rb")}.path + spec_name.sub("spec/migrations", "db/migrate").sub("_spec.rb", ".rb") end