Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deprecation warning (fixture_path -> fixture_paths) #1729

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

alexevanczuk
Copy link
Contributor

Resolves:

ActiveSupport::DeprecationException: DEPRECATION WARNING: TestFixtures.fixture_path= is deprecated and will be removed in Rails 7.2. Use .fixture_paths= instead. (called from use_ui at /Users/alexevanczuk/.rbenv/versions/3.2.0/lib/ruby/site_ruby/3.2.0/rubygems/user_interaction.rb:47) (Parallel::UndumpableException)

Still need to add some tests!

@@ -76,7 +76,12 @@ def fixture_loader
Class.new do
T.unsafe(self).include(ActiveRecord::TestFixtures)

T.unsafe(self).fixture_path = Rails.root.join("test", "fixtures")
if respond_to?(:fixture_paths=)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to export and use a helper like

def rails_version(selector)
::Gem::Requirement.new(selector).satisfied_by?(ActiveSupport.gem_version)
end
with an explicit version requirement. It'd make it easier in the future to drop support for a rails version and clean up the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this, let me know what ya think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought version check would be more consistent but apparently not. Looks like we do things differently for Rails compared to Sorbet. I'll revert and merge. cc @andyw8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks!

@andyw8
Copy link
Contributor

andyw8 commented Dec 12, 2023

👋 Similar to @paracycle's comment here, can we use feature discovery rather than a version check?
I just saw @KaanOzkan's comment, let me check...

@andyw8 andyw8 changed the title Fix deprecation warning Fix deprecation warning (fixture_path -> fixture_paths) Dec 12, 2023
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Existing tests should cover this compiler and we have CI for Rails 6.1 and main. Main isn't fully green but the relevant tests are passing.

@KaanOzkan KaanOzkan merged commit 5327dd1 into Shopify:main Dec 12, 2023
30 checks passed
@alexevanczuk alexevanczuk deleted the ae-fix-deprecation-warning branch December 12, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants