Skip to content

Conversation

@agrobbin
Copy link
Contributor

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.

@agrobbinagrobbinforce-pushed the rails-7-1-file-fixture-upload branch 3 times, most recently from 8a17e8d to d3e1c4bCompareDecember 3, 2023 02:50
@agrobbin
Copy link
ContributorAuthor

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

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 :)

@agrobbinagrobbinforce-pushed the rails-7-1-file-fixture-upload branch 3 times, most recently from 71f11b4 to bc7d3b1CompareDecember 4, 2023 23:57
@agrobbin
Copy link
ContributorAuthor

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.

moduleFixtureFileUploadSupport
delegate:fixture_file_upload,to: :rails_fixture_file_wrapper
moduleFileFixtureUploadSupport
delegate:file_fixture_upload,:fixture_file_upload,to: :rails_file_fixture_wrapper
Copy link
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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

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
ContributorAuthor

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.
@agrobbinagrobbinforce-pushed the rails-7-1-file-fixture-upload branch from bc7d3b1 to d5c6107CompareDecember 11, 2023 01:53
@agrobbin
Copy link
ContributorAuthor

@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.

@mjankowski
Copy link
Contributor

Curious about state of this one ...

I was attempting to rename some of the (soft deprecated) fixture_file_upload calls to use file_fixture_upload instead. It went fine in places where the call was directly in the params of a request being made in an example, but I saw NoMethodError when calling it from a let, and then found this issue.

Given the drop of rails 7.1 support in latest rspec-rails versions - would it make sense to rebase/rework this and drop all the version conditional stuff, since (I think?) both 7.2 and 8.0 will be ok with this?

Aside from that, any blockers here?

Sign up for freeto 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.

3 participants

@agrobbin@JonRowe@mjankowski