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

ActiveStorage::FixtureSet.blob incompatible with rspec #2637

Closed
WillTaylor22 opened this issue Nov 16, 2022 · 6 comments · Fixed by #2671
Closed

ActiveStorage::FixtureSet.blob incompatible with rspec #2637

WillTaylor22 opened this issue Nov 16, 2022 · 6 comments · Fixed by #2671

Comments

@WillTaylor22
Copy link

What Ruby, Rails and RSpec versions are you using?

Ruby version: 3.1.1
Rails version: 7.0.4
RSpec version:
rspec-core (3.12.0)
rspec-rails (5.0.3)
rspec-support (3.12.0)

Observed behaviour

When setting up file fixtures, and calling e.g. fixtures/active_storage/blobs.yml

my_blob: <%= ActiveStorage::FixtureSet.blob(
   filename: "test.jpg",
) %>

This will call ActiveSupport::Testing::FileFixtures.file_fixture_path, which is nil. This then fails.

The rspec config should be set up correctly.

RSpec.configure do |config||
  config.fixture_path = "#{::Rails.root}/spec/fixtures"
  config.file_fixture_path = "#{::Rails.root}/spec/fixtures/files"
end

Expected behaviour

Expect rspec to find file_fixture_path

@pirj
Copy link
Member

pirj commented Nov 16, 2022

We have a default
Can you please help to pinpoint the issue by setting a breakpoint here? What is the difference if you configure it vs use the default?

@JonRowe
Copy link
Member

JonRowe commented Nov 21, 2022

Can you provide a reproduction snippet? (Preferably not an entire app, see our snippets for examples (https://github.com/rspec/rspec-rails/tree/main/snippets)

@jaywhy
Copy link
Contributor

jaywhy commented Apr 7, 2023

I ran into the same problem today. Here's what I've figured out. And sorry I couldn't get a snippet to work. I tried but ran into ActiveStorage initialization issues.

TL;DR.

Rails expects ActiveStorage::FixtureSet.file_fixture_path to be set, but rspec-rails sets RSpec::ExampleGroups::FooExample.file_fixture_path instead. ActiveStorage::FixtureSet.file_fixture_path is nil and blows up a File.join call.

More information

Calling ActiveStorage::FixtureSet.blob inside a fixture will error with TypeError: no implicit conversion of nil into String.

This is the line that is blowing up.

path = Pathname.new(File.join(file_fixture_path, fixture_name))

https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/activesupport/lib/active_support/testing/file_fixtures.rb#L27

The line errors because file_fixture_path is nil and blows up the File.join call. On that line, self is ActiveStorage::FixtureSet, so Rails expects the ActiveStorage::FixtureSet.file_fixture_path class attribute to be defined.

How Rails and minitest do it

Rails and minitest set up the class attribute in active_storage/engine.rb on line 204.

ActiveSupport.on_load(:active_support_test_case) do
  ActiveStorage::FixtureSet.file_fixture_path = ActiveSupport::TestCase.file_fixture_path  
end

rspec-rails

rspec-rails sets the file_fixture_path class attribute on the wrong class. It should set it on ActiveStorage::FixtureSet, but instead sets it on the current example group.

self.file_fixture_path = RSpec.configuration.file_fixture_path

Fix?

If I change that line to the code below, the error goes away.

ActiveStorage::FixtureSet.file_fixture_path = RSpec.configuration.file_fixture_path

I don't know if that is a good fix, or if I'm 100% on the write track, maybe I'm missing something. I could send a pull request.

@pirj
Copy link
Member

pirj commented Apr 16, 2023

file_fixture is getting the path uses it with no explicit receiver. And file_fixture_path is defined with class_attribute that allows overriding the global setting per instance of a test.
Such usage is confirmed by e.g. this test.

As for blob usage inside fixtures, yes, it appears that we're missing setting the default file fixture path.

But the suggested fix would break non-active storage specs, wouldn't it?

@jaywhy
Copy link
Contributor

jaywhy commented Apr 16, 2023

If I set a breakpoint on the line you mentioned, when it runs, self will be ActiveSupport::TestCase. The user can override that value by calling self.file_fixture_path in their test case because they inherit from ActiveSupport::TestCase.

But the suggested fix would break non-active storage specs, wouldn't it?

Yes, you're right. Good catch. I just tested it. It breaks regular file fixtures with the same error I was getting with blobs.

But, you don't have to replace the line. This makes more sense.

included do
  self.file_fixture_path = RSpec.configuration.file_fixture_path
  if defined? ActiveStorage::FixtureSet
    ActiveStorage::FixtureSet.file_fixture_path = RSpec.configuration.file_fixture_path
  end
end

I don't think Rails < 7.0 has the ActiveStorage::FixtureSet class. So, the if statement is needed.

Oh and a quick note. If anyone comes across this and wants an easy solution, I'm currently setting the ActiveStorage::FixtureSet.file_fixture_path in my rails_helper.rb file. This fixes the problem.

@pirj
Copy link
Member

pirj commented Apr 18, 2023

Nice! A PR is welcome, @jaywhy.

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 a pull request may close this issue.

4 participants