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

[sudara]

I had a similar problem to @koenpunt in #2349 where fixture_file_upload stopped working.

     NoMethodError:
       undefined method `file_fixture_path' for RSpec::Rails::FixtureFileUploadSupport::RailsFixtureFileWrapper:Class
       Did you mean?  fixture_path
     # /actionpack/lib/action_dispatch/testing/test_process.rb:25:in `fixture_file_upload'
     # /rspec-rails-dd093928f021/lib/rspec/rails/fixture_file_upload_support.rb:5:in `fixture_file_upload'

In my case, it's because I'm on Rails 6.1 alpha. As of these recent changes, ActionController::TestCase now needs access to file_fixture_path within fixture_file_upload.

There's not currently any CI for Rails 6.1 (it's not out yet) but I thought I would get a head start. I added the reader on a separate line, as the fixture_path reader is turning into an accessor on #2370.

[pirj]

Wondering if it makes sense to extend the matrix to run against Rails master. It would be nice to have a failing test along with the fix.

[benoittgt]

Wondering if it makes sense to extend the matrix to run against Rails master.

I think it will add lot's of work. What about follow alpha,beta and first releases when they are published?

It would be nice to have a failing test along with the fix.

+1

[sudara]

I wasn't sure how rspec-rails has historically coordinated Rails releases.

extend the matrix to run against Rails master

Having rspec-rails generally track rails master would be a recipe for constant breakage on main, but this would work well for a 6.1 branch. When 6.1 is released, the test target could be switched from master to 6.1. But again, I'm not sure how rspec-rails usually does things.

It would be nice to have a failing test along with the fix.

There will be failing tests aplenty when testing against Rails 6.1. I added a failing test specific to the file_fixture_path accessor, but I think its usefulness is debatable.

I'm happy with this remaining open or merged into 6.1 branch is supported. My primary intent was to communicate the problem/fix to others!

[JonRowe]

Can you rebase this?

[sudara]

✅ Rebased

Though I'm seeing some sort of Thor failure in the suite, looks like the issue has been on main recently:

      Thor::UndefinedCommandError:
11180        Could not find command "mountable_engine?".

[pirj]

It's flaky, I've restarted the job, should most probably pass.

[JonRowe]

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

[sudara]

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.

[JonRowe]

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.

[sudara]

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.

[JonRowe]

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?

[sudara]

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?

[JonRowe]

Thanks for researching this and bringing it to our attention, there was more work needed to ensure it worked without deprecation warning but this is closed in #2398