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

Expose a generic fixture accessor for fixture names that may conflict with Minitest #51213

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

casperisfine
Copy link
Contributor

assert_equal "Ruby on Rails", web_sites(:rubyonrails).name
assert_equal "Ruby on Rails", fixture(:web_sites, :rubyonrails).name

This was brought to me by someone with a Metadata model. The fixtures accessor being metadata which conflicts with the metadata method recently added in Minitest.

cc @zk-1

… with Minitest

```ruby
assert_equal "Ruby on Rails", web_sites(:rubyonrails).name
assert_equal "Ruby on Rails", fixture(:web_sites, :rubyonrails).name
```

This was brought to me by someone with a `Metadata` model. The fixtures
accessor being `metadata` which conflicts with the `metadata` method
recently added in `Minitest`.
@byroot byroot merged commit 3ecc269 into rails:main Feb 28, 2024
3 of 4 checks passed
@ghiculescu
Copy link
Member

ghiculescu commented Mar 18, 2024

This is clashing with Minitest::Spec. Here is an example:

class MyTest < ActiveSupport::TestCase
  let(:user) { users(:alex) }
  let(:fixture) { SomeFixtureObject.new } 

  it "passes" { assert_not_nil user }
end

fixture will be redefined here as a method that takes no arguments. Here it will get called when you access users(:alex). And the test will fail with:

ArgumentError: wrong number of arguments (given 2, expected 0)
    minitest (5.17.0) lib/minitest/spec.rb:256:in `block in let'
    rails (5411787a155e) activerecord/lib/active_record/test_fixtures.rb:258:in `method_missing'

A workaround would be to not ever have a let(:fixture), but I wonder if there's a way to avoid having to do that. (My lazy suggestion would be to rename the method added in this PR to _fixture 😆 )

@byroot
Copy link
Member

byroot commented Mar 18, 2024

My lazy suggestion would be to rename the method added in this PR to _fixture 😆

I'd rather not because it's meant to be public API, so a leading underscore would be wrong.

However what we can do:

  • Alias it as _active_record_fixture.
  • Use that alias internally.

You want to PR that?

@byroot
Copy link
Member

byroot commented Mar 18, 2024

NB: that alias should be :nodoc: and private.

@ghiculescu
Copy link
Member

👍 can do

@ghiculescu
Copy link
Member

I'd rather not because it's meant to be public API, so a leading underscore would be wrong.

Hmm, should it be showing in https://edgeapi.rubyonrails.org/classes/ActiveRecord/TestFixtures.html ?

ghiculescu added a commit to ghiculescu/rails that referenced this pull request Mar 19, 2024
ghiculescu added a commit to ghiculescu/rails that referenced this pull request Mar 19, 2024
@byroot
Copy link
Member

byroot commented Mar 19, 2024

Yeah, it should but apparently it doesn't. Not sure how we can make a private method documented.

@ghiculescu
Copy link
Member

I assume you'd have to make all the methods public and then :nodoc: most of them. Not much of that file is public atm.

viralpraxis pushed a commit to viralpraxis/rails that referenced this pull request Mar 24, 2024
fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024
@justinko
Copy link
Contributor

justinko commented Jun 8, 2024

Yeah, it should but apparently it doesn't. Not sure how we can make a private method documented.

#52054

@justinko justinko mentioned this pull request Jun 9, 2024
4 tasks
xjunior pushed a commit to xjunior/rails that referenced this pull request Jun 9, 2024
HeyNonster pushed a commit to HeyNonster/rails that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants