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

Rename of FactoryGirl to FactoryBot has broken my projects #1055

Closed
iHiD opened this Issue Oct 21, 2017 · 18 comments

Comments

Projects
None yet
9 participants
@iHiD

iHiD commented Oct 21, 2017

This is a duplicate of the same issue on factory_bot_rails but I thought it was worth duplicating so others can find it.


Hello. Sorry for the slightly dramatic issue title, but I couldn't find a gentler way to say it :)

Running bundle update and running tests now gives:
NameError: uninitialized constant ActionView::TestCase::FactoryGirl

Renaming FactoryGirl to FactoryBot everywhere in my code fixes the issue, but I think such a breaking change should result in a Major version increment, not a Patch increment. I would suggest reverting the 4.8 branch to factory_girl and release 5.0 as factory_bot.

@S-hack

This comment has been minimized.

Show comment
Hide comment
@S-hack

S-hack Oct 21, 2017

What is the motivation for the change? It's a huge inconvenience and... seems a bit sexist.

S-hack commented Oct 21, 2017

What is the motivation for the change? It's a huge inconvenience and... seems a bit sexist.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Oct 21, 2017

What is the motivation for the change?

The logic for the change is covered in #921 and related discussions linked from there.

It's a huge inconvenience and... seems a bit sexist.

The correctness or not of the change itself is not the point on this issue. This is purely about whether the knock on effect of the fact this was done via a patch version change, rather than as a major release.

iHiD commented Oct 21, 2017

What is the motivation for the change?

The logic for the change is covered in #921 and related discussions linked from there.

It's a huge inconvenience and... seems a bit sexist.

The correctness or not of the change itself is not the point on this issue. This is purely about whether the knock on effect of the fact this was done via a patch version change, rather than as a major release.

@markprzepiora

This comment has been minimized.

Show comment
Hide comment
@markprzepiora

markprzepiora Oct 21, 2017

Definitely should have been a major version change (unless backwards-compatible aliases were included).

But come on guys, this isn't rocket science.

ag -0l FactoryGirl spec | xargs -0 sed -i 's/FactoryGirl/FactoryBot/g'

It's pretty obvious that a lot of people complaining about this change are just on some anti-feminism crusade.

markprzepiora commented Oct 21, 2017

Definitely should have been a major version change (unless backwards-compatible aliases were included).

But come on guys, this isn't rocket science.

ag -0l FactoryGirl spec | xargs -0 sed -i 's/FactoryGirl/FactoryBot/g'

It's pretty obvious that a lot of people complaining about this change are just on some anti-feminism crusade.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Oct 21, 2017

But come on guys, this isn't rocket science.

Totally agree that fixing it is a 2min job.

However, it's totally unexpected to run bundle update and find your test-suite break. It took me as an experienced developer 10mins to work out why FactoryGirl suddenly failed and understand what was going on, etc. Less experienced developers will be totally stumped for significantly longer than that. Therefore, I think this is something that should be reversed on this branch and released onto a major branch.

What is not rocket science for you and me may well feel like rocket science to a beginner who started programming more recently.

It's pretty obvious that a lot of people complaining about this change are just on some anti-feminism crusade.

Agreed.

iHiD commented Oct 21, 2017

But come on guys, this isn't rocket science.

Totally agree that fixing it is a 2min job.

However, it's totally unexpected to run bundle update and find your test-suite break. It took me as an experienced developer 10mins to work out why FactoryGirl suddenly failed and understand what was going on, etc. Less experienced developers will be totally stumped for significantly longer than that. Therefore, I think this is something that should be reversed on this branch and released onto a major branch.

What is not rocket science for you and me may well feel like rocket science to a beginner who started programming more recently.

It's pretty obvious that a lot of people complaining about this change are just on some anti-feminism crusade.

Agreed.

@markprzepiora

This comment has been minimized.

Show comment
Hide comment
@markprzepiora

markprzepiora Oct 21, 2017

However, it's totally unexpected to run bundle update and find your test-suite break. It took me as an experienced developer 10mins to work out why FactoryGirl suddenly failed and understand what was going on, etc. Less experienced developers will be totally stumped for significantly longer than that. Therefore, I think this is something that should be reversed on this branch and released onto a major branch.

Agreed completely. Or add aliases that issue a deprecation warning.

markprzepiora commented Oct 21, 2017

However, it's totally unexpected to run bundle update and find your test-suite break. It took me as an experienced developer 10mins to work out why FactoryGirl suddenly failed and understand what was going on, etc. Less experienced developers will be totally stumped for significantly longer than that. Therefore, I think this is something that should be reversed on this branch and released onto a major branch.

Agreed completely. Or add aliases that issue a deprecation warning.

@Euraldius

This comment has been minimized.

Show comment
Hide comment
@Euraldius

Euraldius Oct 21, 2017

Contributor

Hey! Sorry for the trouble this is causing; we didn't expect it to be a breaking change. The intended behavior is that you see a deprecation warning when you run the test suite. We're investigating now.

Contributor

Euraldius commented Oct 21, 2017

Hey! Sorry for the trouble this is causing; we didn't expect it to be a breaking change. The intended behavior is that you see a deprecation warning when you run the test suite. We're investigating now.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Oct 21, 2017

Thanks @Euraldius

The issue is that defining factories or including in test suites uses the FactoryGirl constant so now that constant is missing, all the consuming code breaks. If you want anyone to pair with on it, feel free to email me :)

iHiD commented Oct 21, 2017

Thanks @Euraldius

The issue is that defining factories or including in test suites uses the FactoryGirl constant so now that constant is missing, all the consuming code breaks. If you want anyone to pair with on it, feel free to email me :)

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 21, 2017

Wanted to chime in and say I like the name change. I’ve also written a bunch on versioning and deprecations. In the future I think these might be of some use:

schneems commented Oct 21, 2017

Wanted to chime in and say I like the name change. I’ve also written a bunch on versioning and deprecations. In the future I think these might be of some use:

@joshuaclayton

This comment has been minimized.

Show comment
Hide comment
@joshuaclayton

joshuaclayton Oct 21, 2017

Member

@schneems thanks for the support, and those are a solid reminder. We'd tested this on a number of projects both internal and external to thoughtbot (e.g. client projects) and for each where we'd locked to ~> 4.8.0, the extent of the impact was a couple of deprecation warnings (which we expected!), but a fully green suite.

We'd introduced https://github.com/thoughtbot/factory_bot/blob/master/lib/factory_girl.rb#L3 to effectively alias constants, but that doesn't seem to have been enough.

In the meantime, we've yanked version 4.8.2 of both factory_girl and factory_girl_rails to prevent further issues.

For those who've installed 4.8.2 of the gems, you can uninstall them with:

gem uninstall factory_girl --version 4.8.2
gem uninstall factory_girl_rails --version 4.8.2

You may also want to lock your Gemfile to "4.8.1" for the time being.

Member

joshuaclayton commented Oct 21, 2017

@schneems thanks for the support, and those are a solid reminder. We'd tested this on a number of projects both internal and external to thoughtbot (e.g. client projects) and for each where we'd locked to ~> 4.8.0, the extent of the impact was a couple of deprecation warnings (which we expected!), but a fully green suite.

We'd introduced https://github.com/thoughtbot/factory_bot/blob/master/lib/factory_girl.rb#L3 to effectively alias constants, but that doesn't seem to have been enough.

In the meantime, we've yanked version 4.8.2 of both factory_girl and factory_girl_rails to prevent further issues.

For those who've installed 4.8.2 of the gems, you can uninstall them with:

gem uninstall factory_girl --version 4.8.2
gem uninstall factory_girl_rails --version 4.8.2

You may also want to lock your Gemfile to "4.8.1" for the time being.

@Euraldius

This comment has been minimized.

Show comment
Hide comment
@Euraldius

Euraldius Oct 21, 2017

Contributor

Note: you can use factory_bot and factory_bot_rails if you want to - those should be working. (The only change you'll need to make is the rename.) I'll add a note in the README about that

Contributor

Euraldius commented Oct 21, 2017

Note: you can use factory_bot and factory_bot_rails if you want to - those should be working. (The only change you'll need to make is the rename.) I'll add a note in the README about that

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Oct 21, 2017

@joshuaclayton @Euraldius Thanks for the quick action.

I've put a sample repo together for you that shows the problems I'm getting here. It has 3 commits - a boilerplate Rails app. the addition of factory_girl_rails 4.8.0 and then the change to master.

After commit 2 I get:
screen shot 2017-10-21 at 20 02 51

After commit 3 I get:
screen shot 2017-10-21 at 20 02 32

(bert = bundle exec rake test)

These are different errors to the one I get in real projects, which is NameError: uninitialized constant ActionView::TestCase::FactoryGirl - I can't reproduce that in this boilerplate app, it just fails to load the tests instead. So there seem to be a couple of issues in there somewhere. I've noticed that this release also packages previous commits as well as the FactoryGirl rename, so something else might be responsible for this bit.

Thanks again for your hard work on this. Let me know if I can be of help in testing anything.

iHiD commented Oct 21, 2017

@joshuaclayton @Euraldius Thanks for the quick action.

I've put a sample repo together for you that shows the problems I'm getting here. It has 3 commits - a boilerplate Rails app. the addition of factory_girl_rails 4.8.0 and then the change to master.

After commit 2 I get:
screen shot 2017-10-21 at 20 02 51

After commit 3 I get:
screen shot 2017-10-21 at 20 02 32

(bert = bundle exec rake test)

These are different errors to the one I get in real projects, which is NameError: uninitialized constant ActionView::TestCase::FactoryGirl - I can't reproduce that in this boilerplate app, it just fails to load the tests instead. So there seem to be a couple of issues in there somewhere. I've noticed that this release also packages previous commits as well as the FactoryGirl rename, so something else might be responsible for this bit.

Thanks again for your hard work on this. Let me know if I can be of help in testing anything.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Oct 21, 2017

Final note. If I remove factory_girl from being explicit in the Gemfile (and just have factory_girl_rails at either 4.8.2 or master), I get the original uninitialized constant ActionView::TestCase::FactoryGirl. So I wonder if that is to do with how factory_girl_rails is loading factory_girl implicitly, and whether the constant is no longer auto-loaded or something.

iHiD commented Oct 21, 2017

Final note. If I remove factory_girl from being explicit in the Gemfile (and just have factory_girl_rails at either 4.8.2 or master), I get the original uninitialized constant ActionView::TestCase::FactoryGirl. So I wonder if that is to do with how factory_girl_rails is loading factory_girl implicitly, and whether the constant is no longer auto-loaded or something.

@quangv

This comment has been minimized.

Show comment
Hide comment
@quangv

quangv Oct 22, 2017

@tcpj sensing sarcasm in your comment...

Yes! Thank you @tastycode and 👏 ThoughtBot, especially @joshuaclayton and @Euraldius for leading the heroic efforts. 👍

quangv commented Oct 22, 2017

@tcpj sensing sarcasm in your comment...

Yes! Thank you @tastycode and 👏 ThoughtBot, especially @joshuaclayton and @Euraldius for leading the heroic efforts. 👍

@thoughtbot thoughtbot deleted a comment from nukeop Oct 22, 2017

@thoughtbot thoughtbot deleted a comment from tcpj Oct 22, 2017

@thoughtbot thoughtbot deleted a comment from tcpj Oct 22, 2017

@joshuaclayton

This comment has been minimized.

Show comment
Hide comment
@joshuaclayton

joshuaclayton Oct 22, 2017

Member

@iHiD Thanks for your help here! I'll dig into your example repo, and see how it differs from some of the internal projects we've been running this on (without changing to FactoryBot) with success. The alias (

FactoryGirl = FactoryBot
) doesn't seem to "stick" in certain situations, resulting in the uninitialized constant issues you're able to produce.

Member

joshuaclayton commented Oct 22, 2017

@iHiD Thanks for your help here! I'll dig into your example repo, and see how it differs from some of the internal projects we've been running this on (without changing to FactoryBot) with success. The alias (

FactoryGirl = FactoryBot
) doesn't seem to "stick" in certain situations, resulting in the uninitialized constant issues you're able to produce.

@tvw

This comment has been minimized.

Show comment
Hide comment
@tvw

tvw Oct 24, 2017

It is dead simple. When the consuming code needs to be changed, it is major version change.

See semver.org:

"1. MAJOR version when you make incompatible API changes"

Changing the namespace is an incompatible API change.

tvw commented Oct 24, 2017

It is dead simple. When the consuming code needs to be changed, it is major version change.

See semver.org:

"1. MAJOR version when you make incompatible API changes"

Changing the namespace is an incompatible API change.

@mjankowski

This comment has been minimized.

Show comment
Hide comment
@mjankowski

mjankowski Oct 24, 2017

Member

We have yanked version 4.8.2 which broke code for people - we apologize for the issues.

Version 4.9.0 is out with nothing but a deprecation warning.

We will have a blog post explaining the whole change along with upgrade instructions in the near future.

Member

mjankowski commented Oct 24, 2017

We have yanked version 4.8.2 which broke code for people - we apologize for the issues.

Version 4.9.0 is out with nothing but a deprecation warning.

We will have a blog post explaining the whole change along with upgrade instructions in the near future.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Oct 24, 2017

@mjankowski Thank you for dealing with this so promptly.

I'm closing this. Could you lock it once I have please as I suspect the ongoing comments from people hit by this will not be particularly helpful... :)

iHiD commented Oct 24, 2017

@mjankowski Thank you for dealing with this so promptly.

I'm closing this. Could you lock it once I have please as I suspect the ongoing comments from people hit by this will not be particularly helpful... :)

@iHiD iHiD closed this Oct 24, 2017

@thoughtbot thoughtbot locked and limited conversation to collaborators Oct 24, 2017

@mjankowski

This comment has been minimized.

Show comment
Hide comment
@mjankowski
Member

mjankowski commented Oct 24, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.