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

feature Add file_fixture helper to syntax methods #1317

Closed
wants to merge 3 commits into from

Conversation

aledustet
Copy link
Contributor

Why:
This is to add simple support to files on factories. This is the
original issue.

What:
It is a common behavior to interact with files when creating factories,
and it is used heavily, as demonstrated with the many examples in the
issue, this is a first draft and I would love to see more use-cases to
add to the acceptance test. For the time being we have at least the base
case added.

@composerinteralia
Copy link
Collaborator

The primary use case for this feature is for use with ActiveStorage-backed models. It doesn't currently work there, so I think we need some tests cases for that.

I have a model with has_one_attached :video, and I was able to create a factory with an attached video using:

after(:build) do |instance|
  instance.video.attach(
    io: File.open(Rails.root.join('spec', 'fixtures', 'files', 'video.mp4')),
    filename: 'video.mp4'
  )
end

or, using this Rack helper:

video { Rack::Test::UploadedFile.new("spec/fixtures/files/video.mp4") }

but I was unable to attach a video using the new method in this PR:

video { file_fixture("spec/fixtures/files/video.mp4") }

This didn't raise any error, but it silently failed to attach a video.


I think it is fine if this works for just the ActiveStorage case for now. We can always extend it other use cases as they emerge. We may even decide to move this into factory_bot_rails if we want to make it clear it is only meant to work with Rails.

That would allow us to be opinionated about expecting the fixture files to live in ActiveSupport::TestCase.file_fixture_path, (defaults to "spec/fixtures/files" in rspec-rails) and so we could specify the filename without the full path:

video { file_fixture_upload("video.mp4") }

This would match the helper method used in the ActiveStorage test suite

# Returns:
# An instance of the file loader class pointing to the file at the
# provided path.
def file_fixture(file_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go on the definition proxy, since it only needs to be available inside the attribute block. Otherwise when people include these syntax methods into their test suite it will end up overwriting the original ActiveSupport file_fixture method.

Choose a reason for hiding this comment

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

Good idea, we don't want to overwrite the original ActiveSupport file_fixture method.

But we also need to be able to call file_fixture from transient attributes and callbacks...

Why:
This is to add simple support to files on factories. This is the
original [issue](#1282).

What:
It is a common behavior to interact with files when creating factories,
and it is used heavily, as demonstrated with the many examples in the
issue, this is a first draft and I would love to see more use-cases to
add to the acceptance test. For the time being we have at least the base
case added.
@aledustet aledustet force-pushed the feature/improve-file-attributes branch from aa5bc6e to 94a10ad Compare August 2, 2019 12:44
@@ -1,3 +1,6 @@
source "https://rubygems.org"

gemspec name: "factory_bot"

gem 'pry'
gem 'activestorage'
Copy link

Choose a reason for hiding this comment

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

Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem activestorage should appear before pry.
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -1,3 +1,6 @@
source "https://rubygems.org"

gemspec name: "factory_bot"

gem 'pry'
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

- I am having a hard time on where to locate the method and have access
to the method name and the instance so I can do something like:
```ruby
file_loader = FileLoader.new()
instance.send(method_name).attach(
  io: file_loader.io, filename: file_loader.filename
)
```
@aledustet aledustet force-pushed the feature/improve-file-attributes branch from d5e37f8 to 67c0f64 Compare August 2, 2019 20:20
@aledustet
Copy link
Contributor Author

Added a new

@aledustet aledustet closed this Aug 2, 2019
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.

3 participants