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

Migrate to Rails 7.1's file_fixture_upload naming approach #2718

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agrobbin
Copy link
Contributor

@agrobbin agrobbin commented Dec 3, 2023

rails/rails#48857 renamed fixture_file_upload to file_fixture_upload to reduce "confusion and surprise". With this change, file_fixture_upload now works in RSpec, with fixture_file_upload still functional to maintain backwards compatibility.

@agrobbin agrobbin force-pushed the rails-7-1-file-fixture-upload branch 3 times, most recently from 8a17e8d to d3e1c4b Compare December 3, 2023 02:50
@agrobbin
Copy link
Contributor Author

agrobbin commented Dec 3, 2023

I'm surprised by the test failures here, and am curious if they are due to something recent in another rspec gem (tests haven't been run on main in ~2 weeks). If there is something I've screwed up that's causing these failures, please let me know and I'll look into it!

@JonRowe
Copy link
Member

JonRowe commented Dec 4, 2023

This is actually due to the ameter gem adding a deprecation warning to a matcher we apparently used in our generator specs it looks like its an RSpec problem but its just we check our builds for warnings and so the error is hidden slightly, working on a fix in #2719 and then this can be rebased :)

@agrobbin agrobbin force-pushed the rails-7-1-file-fixture-upload branch 3 times, most recently from 71f11b4 to bc7d3b1 Compare December 4, 2023 23:57
@agrobbin
Copy link
Contributor Author

agrobbin commented Dec 5, 2023

@JonRowe thanks for the quick fix there! I've rebased this and gotten tests passing. It took an extra pass to add the appropriate testing for Rails 7.1+ and < 7.1, but I think things are in good shape now. Let me know if there's anything else I should change.

module FixtureFileUploadSupport
delegate :fixture_file_upload, to: :rails_fixture_file_wrapper
module FileFixtureUploadSupport
delegate :file_fixture_upload, :fixture_file_upload, to: :rails_file_fixture_wrapper
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One minor note... I have included the delegation for file_fixture_upload in all Rails versions, even though it's only available in Rails 7.1.+. It should raise a NoMethodError if called in Rails < 7.1.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused as to what provides the old behaviour with this change, I can see the build passing but I don't understand what now provides fixture_file_upload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JonRowe it's hard to see, since file_fixture_upload and fixture_file_upload look so similar, but fixture_file_upload is on this same delegate line, after file_fixture_upload.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem!

@JonRowe
Copy link
Member

JonRowe commented Dec 10, 2023

Overall this looks fine, but I'm nervous of the support differences for 6.1 and 7.x, especially the change in module. Could I be a pain and ask you split this into two, with the old module for the old behaviour and the new module, and conditionally include the right module? If you don't want to I'm happy to finish this off

@agrobbin
Copy link
Contributor Author

Happy to, @JonRowe! I should be able to do that tonight.

rails/rails#48857 renamed `fixture_file_upload` to `file_fixture_upload` to reduce "confusion and surprise". With this change, `file_fixture_upload` now works in RSpec, with `fixture_file_upload` still functional to maintain backwards compatibility.
@agrobbin agrobbin force-pushed the rails-7-1-file-fixture-upload branch from bc7d3b1 to d5c6107 Compare December 11, 2023 01:53
@agrobbin
Copy link
Contributor Author

@JonRowe when I started looking at this, I actually noticed that FixtureFileUploadSupport already includes some conditional logic for Rails 7.1+ compared other versions:

if ::Rails::VERSION::STRING < "7.1.0"
attr_accessor :fixture_path
else
attr_accessor :fixture_paths
end

With that in mind, I made a change that is more explicit about which methods are supported, but I left it in the same module. I'm curious if you think that's reasonable, or if you'd still prefer to break things apart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants