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

Use the file_fixture_path for fixture_file_upload: #39086

Merged
merged 1 commit into from May 5, 2020

Conversation

Edouard-chin
Copy link
Member

Use the file_fixture_path for fixture_file_upload:

  • We used the fixture_path before file_fixture_path was a thing,
    but now that we have the latter we should use it.

    fixture_path is solely used by Active Record so it seems wrong
    to be using that in ActionPack.

    Also he main advantage being that one doesn't need to go one dir up
    fixture_file_upload('../files/blabla.png').

@rails-bot rails-bot bot added the actionpack label Apr 29, 2020
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Also we should add a changelog for this change 🙏

if self.class.respond_to?(:fixture_path) && self.class.fixture_path &&
!File.exist?(path)
path = File.join(self.class.fixture_path, path)
if self.class.file_fixture_path && !File.exist?(path)
Copy link
Member

@gmcgibbon gmcgibbon Apr 29, 2020

Choose a reason for hiding this comment

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

Can we still use fixture_path and log a deprecation warning so people know how to fix this for existing relative path code?

@Edouard-chin Edouard-chin force-pushed the ec-fixture-file-upload branch 2 times, most recently from 5d4ae22 to d053d04 Compare May 4, 2020 14:44
- We used the `fixture_path` before `file_fixture_path` was a thing,
  but now that we have the latter we should use it.

  `fixture_path` is solely used by Active Record so it seems wrong
  to be using that in ActionPack.
@gmcgibbon gmcgibbon merged commit f016969 into rails:master May 5, 2020
@Edouard-chin Edouard-chin deleted the ec-fixture-file-upload branch May 6, 2020 13:36
sudara added a commit to sudara/rspec-rails that referenced this pull request Sep 20, 2020
…ls 6.1

I had a similar problem to @koenpunt in rspec#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]( rails/rails#39086), `ActionController::TestCase`  now needs access to `fixture_file_path` within `fixture_file_upload`.

There's not currently any CI for Rails 6.1 (as it's not out yet) but I thought I would get a head start on things.
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 21, 2020
…nmailbox/test

https://buildkite.com/rails/rails/builds/71675#2b9c36e5-5504-4a8e-8c4d-af774e96ea16/954-982
https://buildkite.com/rails/rails/builds/71675#2b9c36e5-5504-4a8e-8c4d-af774e96ea16/954-988

```bash
.................DEPRECATION WARNING: Passing a path to `fixture_file_upload`
relative to `fixture_path` is deprecated.
In Rails 6.2, the path needs to be relative to `file_fixture_path`.

Please modify the call from
`fixture_file_upload("files/avatar1.jpeg")` to `fixture_file_upload("avatar1.jpeg")`.
DEPRECATION WARNING: Passing a path to `fixture_file_upload` relative to `fixture_path` is deprecated.
In Rails 6.2, the path needs to be relative to `file_fixture_path`.

Please modify the call from
`fixture_file_upload("files/avatar2.jpeg")` to `fixture_file_upload("avatar2.jpeg")`.
........................................................................
```

`fixture_file_upload` now uses path relative to `file_fixture_path`, see
rails#39086,
https://github.com/rails/rails/blob/61c4be477706b721688e36a3168f86aabc625658/actionmailbox/test/test_helper.rb#L21
@connorshea
Copy link
Contributor

connorshea commented Dec 3, 2020

For anyone trying to debug issues with fixture_file_upload not working correctly with Rails 6.1, you may be running into an issue with rspec-rails: rspec/rspec-rails#2402

Took me a while to figure that out so I figured I'd leave this here for others :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants