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

Fix deprecated TestFixtures#fixture_path call #2664

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
15 checks passed
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.

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.

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