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

new reload behavior breaks normal code reloading with Spring #323

Closed
jakeonfire opened this issue Mar 4, 2019 · 9 comments
Closed

new reload behavior breaks normal code reloading with Spring #323

jakeonfire opened this issue Mar 4, 2019 · 9 comments

Comments

@jakeonfire
Copy link

jakeonfire commented Mar 4, 2019

Whenever I change code inside the Rails app/ dir, FactoryBot no longer reloads (per the decisions in this commit: 02a0f58). This causes a myriad of strange errors, like having multiple class objects for models for which there exist factories (in other words, MyModel.object_id, and my_model_factory_instance.class.object_id drift apart somehow), ActiveModel methods seem to be forgotten (like ActiveRecord#store_accessor not defined errors), and other weirdness.

Adding back

if defined?(Spring)
  Spring.after_fork { FactoryBot.reload }
end

fixes this issue (for example, in a config.after_initalize block in config/environments/test.rb). But this doesn't feel right at all.

I'm not sure if this has something to do with how our app is configured, but it makes using spring and factory_bot_rails 5.x (and Rails 5.2?) together a non-option (without the workaround mentioned above).

@composerinteralia
Copy link
Collaborator

I had a feeling this was going to be a problem when I saw thoughtbot/factory_bot#1029 (comment).

We may need to swap out reloader.execute_if_updated in

reloader.execute_if_updated
with reloader.execute, so any time anything changes in the application factory_bot will reload too. This matches what the rails routes reloader does: https://github.com/rails/rails/blob/2420c44d4b1dfbffea7b624cb8a8c68beb05936f/railties/lib/rails/application/finisher.rb#L144-L154

Thanks for the issue!

@jakeonfire
Copy link
Author

jakeonfire commented Mar 5, 2019

I had the same thought, but doing so seems to conflict with some other aspect of the loading architecture, yielding (for me):
Factory already registered: adapter (FactoryBot::DuplicateDefinitionError)
...which is the first factory alphabetically.

The change to reloader.execute plus removing these three lines seems to fix it though:

config.after_initialize do
FactoryBot.find_definitions
end

If that sounds like a legit fix I'd be happy to open a PR. I don't understand the gem well enough to know if there's a better path.

@composerinteralia
Copy link
Collaborator

Yup, that seems like the way to go. I would love to see a failing spec for this, but I haven't actually been able to recreate it myself.

@jakeonfire
Copy link
Author

jakeonfire commented Mar 5, 2019

@composerinteralia
Copy link
Collaborator

@jakeonfire are you working on this, or are you still interested in working on this? Let me know if I can help.

@jakeonfire
Copy link
Author

jakeonfire commented Mar 25, 2019

@composerinteralia i am not - we are using a work-around. happy to submit a PR if you'd like, but i won't be able to figure out a failing spec for it.

@composerinteralia
Copy link
Collaborator

@jakeonfire Feel free to submit a PR with whatever you have (although no pressure at all), and I can try and add the test coverage.

composerinteralia added a commit that referenced this issue Apr 4, 2019
This test captures what I think is going on in
#323

Currently the factory_bot_rails reloader only reloads factory
definitions when the definition files have changes (using
`execute_if_updated`).

This can cause problems when editing an application file without also
editing a factory_bot definition file. Rails will reload the entire
application, but will not reload the definitions. This causes
factory_bot to have references to classes that were unloaded from the
Rails application and replaced with new classes.

#329 replaces
`execute_if_updated` with `execute` so that factory_bot_rails will
reload the definition files whenever anything in the application is
changed. Hopefully that PR will get this test to pass!
composerinteralia added a commit that referenced this issue Apr 4, 2019
This test captures what I think is going on in
#323

Currently the factory_bot_rails reloader only reloads factory
definitions when the definition files have changes (using
`execute_if_updated`).

This can cause problems when editing an application file without also
editing a factory_bot definition file. Rails will reload the entire
application, but will not reload the definitions. This causes
factory_bot to have references to classes that were unloaded from the
Rails application and replaced with new classes.

#329 replaces
`execute_if_updated` with `execute` so that factory_bot_rails will
reload the definition files whenever anything in the application is
changed. Hopefully that PR will get this test to pass!
composerinteralia added a commit that referenced this issue Apr 5, 2019
This test captures what I think is going on in
#323

Currently the factory_bot_rails reloader only reloads factory
definitions when the definition files have changes (using
`execute_if_updated`).

This can cause problems when editing an application file without also
editing a factory_bot definition file. Rails will reload the entire
application, but will not reload the definitions. This causes
factory_bot to have references to classes that were unloaded from the
Rails application and replaced with new classes.

#329 replaces
`execute_if_updated` with `execute` so that factory_bot_rails will
reload the definition files whenever anything in the application is
changed. Hopefully that PR will get this test to pass!
composerinteralia added a commit that referenced this issue Apr 5, 2019
* Write a failing test for reloading with Spring

This test captures what I think is going on in
#323

Currently the factory_bot_rails reloader only reloads factory
definitions when the definition files have changes (using
`execute_if_updated`).

This can cause problems when editing an application file without also
editing a factory_bot definition file. Rails will reload the entire
application, but will not reload the definitions. This causes
factory_bot to have references to classes that were unloaded from the
Rails application and replaced with new classes.

#329 replaces
`execute_if_updated` with `execute` so that factory_bot_rails will
reload the definition files whenever anything in the application is
changed. Hopefully that PR will get this test to pass!
composerinteralia pushed a commit that referenced this issue Apr 5, 2019
…pdate) to prevent doubly-defined constants errors in factories when using Spring

- #323
@composerinteralia
Copy link
Collaborator

Fixed with #330

@jakeonfire
Copy link
Author

jakeonfire commented Apr 9, 2019

thanks for your help with this!
dancing_panda

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

No branches or pull requests

2 participants