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
version 5.0.0 association creation #314
Comments
Thanks for opening this issue. This was an intentional change; we changed the default value for |
Just a few failures, I discovered/reported/fixed in under an hour, so it's pretty minimal. Was that change missed in the changelog? We got a dependabot PR today to update us from 4.11 -> 5.0 |
https://github.com/thoughtbot/factory_bot/blob/806ec9f6fe071f3576aa28c804bacfc7eefc790e/NEWS#L3 But maybe it didn't show up in the dependabot PR because it is in the factory_bot NEWS file, not factory_bot_rails? |
Let's leave this issue open, since I'm sure others will run into this as well. Also, I should mention that it is possible to revert to the old behavior by setting |
Yeah, that's probably the reason I missed it. Thanks for the quick response. Annecdotally, (not to bikeshed!) I greatly prefer an additional setup step instead of a rails-specific gem...Feel free to email me if there are discussion points I need to understand there. Maybe I should just throw I agree--this "new" behavior is best! |
factory_bot_rails does include a few extra features beyond automatically loading definitions--mostly notably the generator--but if you are not using those I think using factory_bot and calling |
@composerinteralia totes. +1 for your responsiveness. Thanks for factory bot! (and ex_machina!) |
So if changing the default back is not the recommended way to resolve issues caused by this change then can I ask what is? I can find documentation explaining how this causes behaviour to change, but not how to adapt tests that need to build objects whose associations have been fully created... If it helps to have an example then consider https://github.com/openstreetmap/openstreetmap-website/blob/master/test/models/old_node_test.rb#L9 which creates an object and then tests it's validity to check that the validations are working. We don't want to try and save it because it might not be valid (indeed some tests do use invalid objects) but we do need the associated objects to be save or their ids won't be set and validation will fail because of that. |
@tomhughes the most simple case in my experience:
You just have to be more explicit in the setup. I generally find this to be a better pattern anyway--take more time to do slightly more verbose setup blocks will help serve future me when I come back to the test if it's failing. |
Thanks. I went a slightly different way in the end and made the tests check for the validation they are actually interested in which should be more robust - some of them were effectively silently not doing what was intended as a result of this change combined with them being too generic. |
Just for additional context on this issue: The upgrade is breaking a quarter of spec suites according to meta analysis by depend-a-bot |
@composerinteralia any reason thoughtbot prefers NEWS file over something like CHANGELOG? |
This took us a good couple of hours to figure out, and failed on the linting step. I didn't immediately realise it had to do with a factory_bot upgrade (and we're not using rails), so it took me a while to find the documented issues. |
@pboling thank you for that information. That makes sense, since changing the default value for @chevinbrown I haven't thought much about it, but https://www.gnu.org/prep/standards/standards.html#NEWS-File seems to indicate that NEWS files are for user-visible changes, and change logs include all the details of every internal change. Our NEWS files do focus on user-visible changes. @jrgns thanks for that feedback. I am thinking about maybe writing up a blog post with the details of why we changed the default for |
@tomhughes I really like the changes you made in openstreetmap/openstreetmap-website@f7694a9. Let me know if you run into any other problems. |
Thanks for keeping the issue open and findable! |
Thanks @erikraudsepp. If at some point you do tackle the failing tests, I would be curious which fixes were not straightforward.
I had thought https://github.com/thoughtbot/factory_bot/blob/806ec9f6fe071f3576aa28c804bacfc7eefc790e/NEWS#L3-L4 would be explicit enough. For the future, is there something else that would have helped you? |
@composerinteralia |
@chevinbrown I think dependabot does recognize the NEWS file. I think your updated comment is right--people using factory_bot_rails might not have seen the changes listed in factory_bot's NEWS file. |
@composerinteralia thank you for your response. In the test we were using:
Which in the factory is defined as:
At first I thought that I would just need to change the callback before(:create) to after(:build). But after a few tries, I also noticed that now I need to pass in the owner. Now after being more familiar with how factory_bot works, I think the changelog message is enough. Using factory_bot has been intuitive enough that I haven't had any need so far to familiarize myself with the readme and "use_parent_strategy" didn't click at all. |
It took me about 15 minutes to track down the issue in my app and append |
In FactoryBot 5 there's been a change in how associations are built (new behavior is to use the parent build strategy, old behavior... not sure what is the strategy then). This new behavior broke some tests. For now old behavior is restored. The new behavior is very briefly explained in: https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#associations The new (backwards-incompatible) behavior is also discussed in these issues: thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 FactoryBot dev has mentioned maybe writing a blog post explaining the rationale behind the change and how to adapt tests. In the future the old behavior will likely get deprecated. Whenever I decide to use the new behavior in FeedBunch I'll have to check if there's a post in the FactoryBot blog about this.
I haven't had time to write a blog post about this, but if anybody is curious about the motivation behind this change I tried to dump all of my thoughts into the commit message: thoughtbot/factory_bot@d0208ed |
Thanks, @composerinteralia that's super helpful. |
The default value for `FactoryBot.use_parent_strategy` changed from `false` to `true` at [v5.0.0](thoughtbot/factory_bot_rails#314) This caused some validation errors in role creation in tests because the attached role assignments weren't created correctly due to the new FactoryBot configuration. We could set `FactoryBot.use_parent_strategy = false`, but I think it's better to update the factories themselves.
The default value for `FactoryBot.use_parent_strategy` changed from `false` to `true` at [v5.0.0](thoughtbot/factory_bot_rails#314) This caused some validation errors in role creation in tests because the attached role assignments weren't created correctly due to the new FactoryBot configuration. We could set `FactoryBot.use_parent_strategy = false`, but I think it's better to update the factories themselves.
Ruby 2.6.3 When upgrading to version 5, I am getting the
shoulda-matcher validate_uniqueness_of with scope |
The default value for `FactoryBot.use_parent_strategy` changed from `false` to `true` at [v5.0.0](thoughtbot/factory_bot_rails#314) This caused some validation errors in artefact creation in tests because the attached role assignments weren't created correctly due to the new FactoryBot configuration. We could set `FactoryBot.use_parent_strategy = false`, but I think it's better to update the factories themselves.
Some of these tests were failing because factorybot has changed the default for creating associations. For the validations to work correctly, the association has to have been created and persisted. The default is now to mirror the create/build option that the parent fixture has used. thoughtbot/factory_bot_rails#314
@andreierdoss are you able to provide a reproduction script in this format? That would make it easier to debug what is going on in your specific case. |
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false. thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 These tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false. thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 These tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false. thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 These tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false. thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 These tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false. thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 These tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false. thoughtbot/factory_bot_rails#314 thoughtbot/factory_bot#1255 These tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
This change has been out for 10 months now, so I am going to go ahead and close this issue now. Thanks everyone for your help! |
This is honestly probably the way it should work (I'm assuming it's somewhat related d320202#diff-e79a60dc6b85309ae70a6ea8261eaf95R26)
When building a record with an association, the associations are not created first.
Quick example:
In ~4.11
build :user
would create thedoggie
association.In ~5.0.0
build :user
does not yet create the association.Is this expected?
I prefer it b/c it's more explicit, but it does seem to be breaking (if indeed I'm on the right track). Any insights?
The text was updated successfully, but these errors were encountered: