Skip to content

Fix deprecated TestFixtures#fixture_path call #2664

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Fix deprecated TestFixtures#fixture_path call #2664

merged 2 commits into from
Apr 19, 2023

Conversation

nsimmons
Copy link
Contributor

@nsimmons nsimmons commented Apr 6, 2023

Fixes this error message from showing up when running Rails edge:

DEPRECATION WARNING: TestFixtures#fixture_path is deprecated and will be removed in Rails 7.2. Use #fixture_paths instead. (called from block in <module:FixtureSupport> at /Users/nsimmons/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/rspec-rails-6.0.1/lib/rspec/rails/fixture_support.rb:24)

Closes #2661

Velisarius

This comment was marked as outdated.

@pirj
Copy link
Member

pirj commented Apr 6, 2023

Rails: main is deceivingly green, but actually has this:

DEPRECATION WARNING: TestFixtures#fixture_path is deprecated and will be removed in Rails 7.2. Use #fixture_paths instead.
If multiple fixture paths have been configured with #fixture_paths, then #fixture_path will just return
the first path.
 (called from block (3 levels) in <top (required)> at /home/runner/work/rspec-rails/rspec-rails/spec/rspec/rails/configuration_spec.rb:168)

Where is it coming from?

I wonder if we should also add a setting that would allow multiple fixture paths.

@jguecaimburu
Copy link
Contributor

@pirj the group variable defined in the example inherits from ActiveRecord::TestFixtures, so in this line https://github.com/rspec/rspec-rails/blob/main/spec/rspec/rails/configuration_spec.rb#L168 the deprecated method gets called:

expect(group).to respond_to(:fixture_path)
expect(group.fixture_path).to eq("custom/path")

Maybe it could be tested like this:

if ActiveRecord::TestFixtures.respond_to?(:fixture_paths=)
  expect(group).to respond_to(:fixture_paths)
  expect(group.fixture_paths).to include("custom/path")
else
  expect(group).to respond_to(:fixture_path)
  expect(group.fixture_path).to eq("custom/path")
end

Not sure if there's a better way to get around it if we wanna keep the respond_to expectation.

I wonder if we should also add a setting that would allow multiple fixture paths.

It makes sense to me. I also wonder if we should remove Rails default fixture folder from the array. With this change we'll have something like:

if self.respond_to?(:fixture_paths=)
  self.fixture_paths << RSpec.configuration.fixture_path
  # ["test/fixtures", "rspec/custom/path"]
else
  self.fixture_path = RSpec.configuration.fixture_path
  # "rspec/custom/path"
end

I would prefer to replicate the Rails idea of having a default "spec/fixtures" folder, then the developer could add extra paths in the config. But we could force ["spec/fixtures"] into the config generator as well.

Here's the commit where fixture_path was deprecated: rails/rails@6902cbc

@pirj
Copy link
Member

pirj commented Apr 14, 2023

I also wonder if we should remove Rails default fixture folder from the array.

Probably not.

replicate the Rails idea of having a default "spec/fixtures" folder

We have that in rails_helper.rb generator.
It is done differently for file_fixture_path though.

better way to get around

I'd wrap the existing example in

if ::Rails::VERSION::MAJOR < 7

and would add another example inside else.

I suggest reducing the scope of this PR to just fixing the deprecation. We can implement fixture_paths= in our configuration at a later stage.

Apologies for the red ci, fixing in #2666

@jguecaimburu
Copy link
Contributor

I suggest reducing the scope of this PR to just fixing the deprecation. We can implement fixture_paths= in our configuration at a later stage.

Yeah, makes sense. I'll open a PR after this one is merged.

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

This needs to add a spec / fix the existing spec that still emits a deprecation, I hope to merge rspec/rspec-support#575 at some point which will handle this better as currently these deprecations don't trip our warning detection but just fail outright.

@JonRowe
Copy link
Member

JonRowe commented Apr 14, 2023

This also triggers a rubocop rule because self.fixture_paths << doesn't need a self like self.fixture_path =:

lib/rspec/rails/fixture_support.rb:25:16: C: [Correctable] Style/RedundantSelf: Redundant self detected.
            if self.respond_to?(:fixture_paths=)
               ^^^^
lib/rspec/rails/fixture_support.rb:26:15: C: [Correctable] Style/RedundantSelf: Redundant self detected.
              self.fixture_paths << RSpec.configuration.fixture_path
              ^^^^

@nsimmons
Copy link
Contributor Author

Corrected Rubocop issues and fixed the spec that was emitting the deprecation message.

@pirj
Copy link
Member

pirj commented Apr 18, 2023

Re-running jobs failed with "marshal data too short" after merging the fix in rspec/rspec-support#575

@pirj pirj requested a review from JonRowe April 18, 2023 22:33
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you, @nsimmons !

@pirj
Copy link
Member

pirj commented Apr 19, 2023

This needs to add a spec / fix the existing spec that still emits a deprecation

@JonRowe This change fixed those multiple Rails: main failures:


  299) RSpec::Rails::FixtureFileUploadSupport with fixture path set in config resolves fixture file
       Failure/Error: example.run

       RuntimeError:
         Warnings were generated: DEPRECATION WARNING: TestFixtures#fixture_path is deprecated and will be removed in Rails 7.2. Use #fixture_paths instead. (called from block in <module:FixtureSupport> at /home/runner/work/rspec-rails/rspec-rails/lib/rspec/rails/fixture_support.rb:24)

There's just one unrelated spec failure left on Rails: main:

Failures:

  1) be_valid matcher includes the error messages in the failure message
     Failure/Error:
       expect {
         expect(post).to be_valid
       }.to raise_exception(/Title can't be blank/)

that I've fixed in #2669

Is this good to merge?

@JonRowe JonRowe merged commit 234677d into rspec:main Apr 19, 2023
JonRowe added a commit that referenced this pull request Apr 19, 2023
@nsimmons nsimmons deleted the fix-deprecated-fixture_path branch April 20, 2023 00:35
JonRowe added a commit that referenced this pull request May 4, 2023
Fix deprecated TestFixtures#fixture_path call
JonRowe added a commit that referenced this pull request May 4, 2023
@JonRowe
Copy link
Member

JonRowe commented May 4, 2023

Released in 6.0.2

@jasnow
Copy link
Contributor

jasnow commented May 4, 2023

@JonRowe - Thanks for the release.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPRECATION WARNING: TestFixtures#fixture_path is deprecated and will be removed in Rails 7.2
7 participants