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.

Sign up for GitHub

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

Jump to bottom

ActionDispatch::TestProcess needs to access :file_fixture_path on Rails 6.1 #2385

Closed
wants to merge 2 commits into from
Closed

ActionDispatch::TestProcess needs to access :file_fixture_path on Rails 6.1 #2385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d920fe
Failing test: file_fixture_path attribute unreachable by RailsFixture…
sudara Sep 21, 2020
fe2bdb4
Ensure file_fixture_path can be read by ActionDispatch::TestProcess
sudara Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rspec/rails/fixture_file_upload_support.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class RailsFixtureFileWrapper

class << self
attr_accessor :fixture_path
attr_reader :file_fixture_path
Copy link
Member

Choose a reason for hiding this comment

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

If this is only for the next version of rails, we need some kind of feature flag.

Copy link
Author

Choose a reason for hiding this comment

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

Prior versions of Rails do not access file_fixture_path from within ActionDispatch::TestProcess#fixture_file_upload. Is there a benefit to not exposing the reader in those cases? Otherwise it feels a bit heavy handed.

Copy link
Member

Choose a reason for hiding this comment

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

These is because the presence of this reader might be seen by other libraries feature detection and the wrong version of Rails assumed, we don't use it ourselves so theres no point having a defined reader that does nothing but might potentially false flag other libraries.

Copy link
Author

@sudara sudara Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I'm missing some context, but it doesn't feel right to me to add code to support a hypothetical edge case such as a third party library leaning on this particular class in RSpec to determine a Rails version. However, this is your project, so please feel free to merge / modify / close out this PR as you wish.


# Get instance of wrapper
def instance
Expand Down
6 changes: 6 additions & 0 deletions spec/rspec/rails/fixture_file_upload_support_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ module RSpec::Rails
end
end

context 'ActionDispatch::TestProcess on Rails 6.1' do
it 'can read the file_fixture_path attribute' do
expect(FixtureFileUploadSupport::RailsFixtureFileWrapper).to respond_to(:file_fixture_path)
Copy link
Member

Choose a reason for hiding this comment

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

This explains the how, but not the why, any chance of an integration spec for 6.1? Or do some of our existing integration specs fail on 6.1 for this reason?

Copy link
Author

@sudara sudara Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Or do some of our existing integration specs fail on 6.1 for this reason?

Exactly, the wrapped class ActionDispatch::TestProcess requires access to this attribute as of Rails 6.1, otherwise fixture_file_upload bombs out. As to why that class needs to be wrapped in the first place, I'm not sure, that wasn't clear to me from reading through the code / specs.

I'm not sure what the convention is for upcoming releases: is 6.1 running on a branch somewhere? Is it trivial for me to set that up locally?

end
end

def fixture_file_upload_resolved(fixture_name, fixture_path = nil)
RSpec::Core::ExampleGroup.describe do
include RSpec::Rails::FixtureFileUploadSupport
Expand Down