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

Always execute a reload when Rails triggers one (instead of just on u… #329

Merged
merged 1 commit into from Apr 5, 2019

Conversation

jakeonfire
Copy link

…pdate) to prevent doubly-defined constants errors in factories when using Spring

@jakeonfire
Copy link
Author

jakeonfire commented Mar 26, 2019

@composerinteralia there are 3 failing specs where FactoryBot.reload gets called twice instead of once. i'm not sure why that is though.

@composerinteralia
Copy link
Collaborator

Maybe because the to_prepare hook gets invoke a variable number of times: rails/rails#28108

@jakeonfire
Copy link
Author

jakeonfire commented Mar 26, 2019

that is indeed what is happening. given this comment rails/rails#28111 (comment) should the specs assert that #reload is called at least once?

@composerinteralia
Copy link
Collaborator

Yeah, that makes sense to me

…pdate) to prevent doubly-defined constants errors in factories when using Spring

- #323
@jakeonfire
Copy link
Author

@composerinteralia if you feel like adding that spec to prove Spring and #execute_if_updated are incompatible, feel free!

@composerinteralia
Copy link
Collaborator

Thanks! I'll try to find some time on Friday to look at this.

composerinteralia added a commit that referenced this pull request 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
Copy link
Collaborator

composerinteralia commented Apr 4, 2019

@jakeonfire I added a failing test in #330

Can you give that a review when you have a chance? It looks like it does indeed pass with your PR. 🎉

composerinteralia added a commit that referenced this pull request 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 pull request 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 pull request 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 composerinteralia merged commit dcfc9f6 into thoughtbot:master Apr 5, 2019
@composerinteralia
Copy link
Collaborator

Thanks again!

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.

None yet

2 participants