Support Rails 7.2 in test suite #252
Conversation
| @@ -0,0 +1,9 @@ | |||
| RSpec::Matchers.define :exist do |*expected| | |||
There was a problem hiding this comment.
From https://github.com/alexrothenberg/ammeter/blob/main/lib/ammeter/rspec/generator/matchers/exist.rb
Needs attribution or alternative fix.
There was a problem hiding this comment.
One idea I have in mind is to add String#exists? (only in tests) and rely on the built-in RSpec matcher. This way, we won't need to refactor all the usages.
Or, we can add a custom matcher, say, be_a_file:
expect("some_path.rb").to be_a_fileThere was a problem hiding this comment.
I favored not patching String but can if you'd prefer a smaller diff. Alternatively, I can pull that change to an independent PR if you'd rather.
gemfiles/rails7.gemfile
Outdated
| @@ -1,6 +1,6 @@ | |||
| source 'https://rubygems.org' | |||
|
|
|||
| rails_version = '~> 7.0.0' | |||
| rails_version = '~> 7.0' | |||
There was a problem hiding this comment.
I think, we need to split this gemfile into two, one for ~> 7.0.0 series and one for ~> 7.0, since the differences are significant between 7.0 and 7.2
There was a problem hiding this comment.
Usually, I'd think it unnecessary but since Logidze has additional exposure from using non-public APIs from Rails, it's a good idea!
spec/spec_helper.rb
Outdated
| @@ -39,6 +39,6 @@ | |||
| config.append_after(:each, db: true) do |ex| | |||
| ActiveRecord::Base.connection.rollback_transaction | |||
|
|
|||
| raise "Migrations are pending: #{ex.metadata[:location]}" if ActiveRecord::Base.connection.migration_context.needs_migration? | |||
| raise "Migrations are pending: #{ex.metadata[:location]}" if ActiveRecord::Base.connection_pool.migration_context.needs_migration? | |||
There was a problem hiding this comment.
Looks like this change is causing test failures on CI. Could you please take a look?
There was a problem hiding this comment.
Will do. This does pass locally with 7.2.2 and Ruby 3.3.6. Additionally, this isn't a backward compatible change so I'll fix that too.
There was a problem hiding this comment.
Still investigating this piece.
There was a problem hiding this comment.
The last two commits should address this. See https://github.com/atomaka/logidze/actions/runs/12209635303/job/34064707411 (from atomaka#2) for a passing test suite.
ActiveRecord::MigrationContext was made a public API in rails/rails@eb09f59 so let's prefer that when it's available. Prefered check might be ActiveRecord.const_defined?(:MigrationContext) but MigrationContext existed as a private API with a different interface in Rails 6.
| migration_context = if Rails::VERSION::MAJOR >= 7 | ||
| ActiveRecord::MigrationContext.new("db/migrate") |
There was a problem hiding this comment.
ActiveRecord::MigrationContext was made a public API in rails/rails@eb09f59 released with Rails 7. Prior to that, it was a non-public API (with a different interface)
|
Thanks a lot! |
What is the purpose of this pull request?
What changes did you make? (overview)
Updates the test suite to run the latest Rails 7.2 minor version and corrects test suite issues to allow it to pass.
ActiveRecord::Base.connection.migration_contextno longer existsActiveRecord::MigrationContextis now public API and a good replacementto_sis no longer overridden in Rails. Similar behavior exists asto_fsto_s(format)in favor ofto_formatted_s(format)rails/rails#43772ActiveSupport::Deprecation.warnhas been removedSingletonrails/rails#48395Is there anything you'd like reviewers to focus on?
be_a_fileRails::VERSION::MAJORfor Rails version compatibilityChecklist