-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix deprecated TestFixtures#fixture_path call #2664
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
Conversation
Where is it coming from? I wonder if we should also add a setting that would allow multiple fixture paths. |
@pirj the 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
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 Here's the commit where |
Probably not.
We have that in
I'd wrap the existing example in if ::Rails::VERSION::MAJOR < 7 and would add another example inside I suggest reducing the scope of this PR to just fixing the deprecation. We can implement Apologies for the red ci, fixing in #2666 |
Yeah, makes sense. I'll open a PR after this one is merged. |
There was a problem hiding this 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.
This also triggers a rubocop rule because
|
Corrected Rubocop issues and fixed the spec that was emitting the deprecation message. |
Re-running jobs failed with "marshal data too short" after merging the fix in rspec/rspec-support#575 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @nsimmons !
@JonRowe This change fixed those multiple
There's just one unrelated spec failure left on
that I've fixed in #2669 Is this good to merge? |
Fix deprecated TestFixtures#fixture_path call
Released in 6.0.2 |
@JonRowe - Thanks for the release. |
Fixes this error message from showing up when running Rails edge:
Closes #2661