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 undefined method config #440

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

benoittgt
Copy link
Contributor

@benoittgt benoittgt commented Nov 23, 2023

Hello

This PR makes last version of factory_bot_rails working with Rails and other gems like database_cleaner-active_record when ActiveRecord::Base is loaded too early. Also the code is tested on last Rails version with the work of @y-yagi. I choose to cherry pick the commit because of great job.

I added a test that reproduce exactly the error and could behave as a non-regression test. Without the patch:

Screenshot 2023-11-23 at 12 45 29

Fix: #432
Fix: #436
Close: #429

For me the code of this gem is not bad, it's because of other gem that we have errors. See recommendations and discussions on this subject: rails/rails#46567

cc @mike-burns

y-yagi and others added 6 commits November 22, 2023 21:40
Some tests put factory files under the `lib`. But since Rails 7.1,
Rails loads `lib` directory by default in a new application.
https://guides.rubyonrails.org/7_1_release_notes.html#introducing-config-autoload-lib-and-config-autoload-lib-once-for-enhanced-autoloading

But factory files don't follow the naming rule of Zeitwerk. So
Zeitwerk raises `Zeitwerk::NameError`.

To avoid the error, this changed to ignore the directory that puts
factory files.
Some test code loads the route file twice. This wasn't a problem
before Rails 7.0 because we didn't have a route.

But, Rails 7.1 has one route by default. So the test application
raises the following error during the load.

```
Invalid route name, already in use: 'rails_health_check'  (ArgumentError)
You may have defined two routes with the same name using the `:as` option, or you may be overriding a route already defined by a resource with the same naming. For the latter, you can restrict the routes created with `resources` as explained here:
https://guides.rubyonrails.org/routing.html#restricting-the-routes-created
```

The routing isn't a matter of this gem, so just ignore that for
running the test.
`Rails.application` is not available when the railties are collected,
so we move the ActiveRecord configuration into the initializer phase.

```
NoMethodError: undefined method `config' for nil:NilClass (NoMethodError)
      application.config
                 ^^^^^^^
/project/app/vendor/bundle/ruby/3.2.0/gems/railties-7.0.8/lib/rails.rb:47:in `configuration'
/project/app/vendor/bundle/ruby/3.2.0/gems/factory_bot_rails-6.4.0/lib/factory_bot_rails/railtie.rb:25:in `block in <class:Railtie>'
```
Copy link
Member

@neilvcarvalho neilvcarvalho left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for cherry picking the fixes and adding a test. I'm asking for a second pair of eyes to take a look at this, then I'll cut a new release.

Copy link
Member

@matsales28 matsales28 left a comment

Choose a reason for hiding this comment

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

LGTM! I tested it locally, and it's working correctly. Ty for the fix!

@neilvcarvalho neilvcarvalho merged commit 79d1ad2 into thoughtbot:main Nov 23, 2023
13 checks passed
@benoittgt benoittgt deleted the fix-undefined-method-config branch November 23, 2023 20:06
@benoittgt
Copy link
Contributor Author

Thanks a lot @neilvcarvalho and @matsales28. Do you plan to make a release? ✨

@neilvcarvalho
Copy link
Member

@benoittgt Working on it right now, and should be up in a few minutes

@neilvcarvalho
Copy link
Member

Released on https://github.com/thoughtbot/factory_bot_rails/releases/tag/v6.4.2 - thank you for working on this!

This was referenced Nov 23, 2023
@benoittgt
Copy link
Contributor Author

Thank you so much 🙌

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.

5 participants