Change FactoryGirl to FactoryBot in Solidus v2.3 #2835

fastjames · 2018-08-28T03:34:47Z

Change FactoryGirl to FactoryBot

We need to backport this change to maintain the branch, since factory_girl is no longer available as a gem.

I don't know how much interest there is in backporting changes like this, but I figured it would be useful since this branch is still under maintenance until November or so.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

kennyadsl · 2018-09-02T16:31:17Z

Personally, I'm against changing this in old versions branch/es. I know they should be maintained still for some time but the maintenance is more for security updates, or at least bugfix.

Let's wait for some other opinions on this, thanks anyway.

fastjames · 2018-09-02T23:16:10Z

I would prefer to leave it as-is as well, so maybe I need to lock it to a pre-name change version? That would be a less invasive change. My goal here is to help extension authors who run CI against older versions. I had trouble getting versions with factory_girl to pass so I might have been doing something wrong.

kennyadsl · 2018-09-04T09:25:52Z

What about locking factory_bot gem to 4.10.0 in solidus 2.3 instead? It should still have the factory_girl file in /lib which has been removed with this commit. What do you think?

BenMorganIO · 2018-09-10T22:17:50Z

Are we able to kill using FactoryBot explicitly in specs? Using the monkey patched version of create and build are far more succinct, use less code, and uses FactoryBot as it was originally meant to be used.

BenMorganIO · 2018-09-10T22:26:47Z

@kennyadsl the community is providing a valid change to help with backward compatibility. Solidus team members are committed to security patches for older versions. If a community member provides a fix, especially one that may be holding them back, I don't see why the Solidus team cannot merge it if the work has already been paid for.

tvdeyen · 2018-09-11T06:45:03Z

We have a policy of very stable branches. Even small changes may have impact on stores. Since this change does not affect the stability I am fine with it, but I am not sure why this change is needed. Do specs fail without that change? The factory_girl is still available. It hasn’t been removed from rubygems AFAIK. Maybe we need to pin the specific version.

kennyadsl · 2018-09-11T08:35:08Z

@tvdeyen specs fail because they removed the FactoryGirl namespace at all in the latest released versions.

@BenMorganIO I wasn't aware that this was breaking extension specs on older Solidus versions when I said I was against this PR. Now that this is clear I'm open to merge this change, even if we should make the same thing for the other older versions maybe?

tvdeyen · 2018-09-11T10:00:05Z

@tvdeyen specs fail because they removed the FactoryGirl namespace at all in the latest released versions.

@kennyadsl but not if we pin the version of factory_girl to 4.8.1, right? That's the last version that does not have all the deprecation notices in it. This should work well and is the least intrusive change for this branch IMO.

@fastjames would you please revert all the FactoryGirl -> FactoryBot changes and just lock the factory_girl version? We should be fine then.

Thanks for the contribution

kennyadsl · 2018-09-11T10:03:11Z

@tvdeyen I think we could even switch to factory_bot and just use 4.10.0 version, which is the last that contains the old FactoryGirl namespace.

tvdeyen · 2018-09-11T10:08:07Z

@kennyadsl This only produces a lot of deprecation noise, no? This might confuse people and make them think they should upgrade. Are there any notable fixes in 4.10 that we need?

I am not opposed to it, just think we should reduce deprecation notices to a minimum.

kennyadsl · 2018-09-11T10:12:02Z

@tvdeyen Yes, I was just afraid that factory_girl gem one day could be deleted but honestly I don't even know if it's doable and for sure when this happens those Solidus versions will have passed their EOL.

👍 for your proposal

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

fastjames · 2018-09-11T14:20:49Z

Glad I could generate some discussion! To answer an earlier question from @tvdeyen I came to this because I had an extension whose specs were failing, and the errors seemed to point to the uphill factory_girl dependency as the culprit (because it was not pinned). With time comes understanding, and I would be happy to pin to either of the mentioned versions. If 4.10.0 creates deprecation messages and we don't want to fix them, then I would prefer to pin to 4.8.1. I am pushing a new commit with that in it now.

BenMorganIO · 2018-09-11T19:35:50Z

@fastjames are you trying to use factory bot in your current project?

BenMorganIO · 2018-09-11T19:39:31Z

If so, you may be able to cheat with this initializer:

# config/initializers/factory_bot.rb

FactoryBot = FactoryGirl

And then as you bump Solidus or try and add factory bot with factory girl (if that's possible :/) it should make the journey somewhat easier on you.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

`factory_girl` 4.8.1 is the last version that supports being called as such; newer versions of the gem (now named `factory_bot`) have dropped the backcompat files.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming

.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.

fastjames · 2018-09-12T14:37:34Z

I can reproduce the poltergeist timeout error, but not the other one; it seems to be related to the time being taken to compile assets.

BenMorganIO · 2018-09-21T17:34:28Z

@fastjames yeah, I've ran into that issue before.

kennyadsl · 2018-10-02T23:23:25Z

I'm trying to locally run specs on an extension (solidus_auth_devise) using this branch and it does not work. 😢

I'm afraid my suggestion was incorrect and locking factory_girl dependency in common_spree_dependencies.rb is not a solution since they are dependency needed for the solidus gem development and they are not considered as extensions dependencies.

I think the only way to handle this is your first attempt which I've wrongly request changes for, sorry!. Do you have that version still available (maybe with git reflog)? If not I can make the change again, please let me know.

tvdeyen · 2018-10-03T06:54:52Z

@kennyadsl what happens if you lock the factory girl version in Solidus Auth Devise? Do they still fail?

tvdeyen · 2018-10-03T06:56:40Z

I'm afraid my suggestion was incorrect and locking factory_girl dependency in common_spree_dependencies.rb is not a solution since they are dependency needed for the solidus gem development and they are not considered as extensions dependencies.

I think this change is still correct. If someone wants to build Solidus 2.3 we want the tests to pass.

For extensions though we need to make sure that the extension is using factory girl 4.8.1 if they run specs for Solidus 2.3.

kennyadsl · 2018-10-03T08:23:18Z

For extensions though we need to make sure that the extension is using factory girl 4.8.1 if they run specs for Solidus 2.3.

Yes but currently almost all extensions are built to work with all solidus versions, adding some extra code or branch in all extension is worst than change Factory Girl to Factory Bot in 2.2, 2.3, 2.4 branches here IMHO.

In terms of the danger of doing that, I think it's just matter of specs so there could not be issues. If stores that still uses Factory Girl (and are importing solidus factories into their specs) updates to latest Solidus patch version (like 2.3.1) they will be forced to update to Factory Bot, but is that a real issue?

tvdeyen · 2018-10-03T09:43:52Z

Yes but currently almost all extensions are built to work with all solidus versions, adding some extra code or branch in all extension is worst than change Factory Girl to Factory Bot in 2.2, 2.3, 2.4 branches here IMHO.

Extensions already have code that distinguish Solidus versions.

See solidus_auth_devise as an example

The change extensions would need to make is not that hard actually.

In solidus_auth_devise it would be to add one line in the Gemfile

# Gemfile
...
group :test do
  ...
  if branch < "v2.5"
    gem 'factory_bot', '4.10.0' # has both `factory_girl` and `factory_bot` in it.
  else
    gem 'factory_bot', '> 4.10.0'
  end
  ...
end

and remove the factory_bot dependency from the gemspec.

We can also update the solidus_cmd Gemfile template, as Solidus 2.4 (the last version that still uses factory_girl) reaches EOL in May 2019 and we need to support it until then.

tvdeyen · 2018-10-03T09:44:43Z

In terms of the danger of doing that, I think it's just matter of specs so there could not be issues. If stores that still uses Factory Girl (and are importing solidus factories into their specs) updates to latest Solidus patch version (like 2.3.1) they will be forced to update to Factory Bot, but is that a real issue?

It think we should avoid breaking the builds of people upgrading a minor (or patch version) of Solidus, if we can.

tvdeyen · 2018-10-03T09:48:02Z

@kennyadsl I will prepare a PR for solidus_auth_devise

kennyadsl · 2018-10-03T09:57:20Z

@tvdeyen ok thanks, let's see how it comes 🙂

tvdeyen · 2018-10-03T10:00:18Z

solidusio/solidus_auth_devise#130

tvdeyen · 2018-10-03T10:12:07Z

Actually the fix is even easier

Just use 'factory_bot', '4.10.0' for all Solidus versions. What FactoryBot feature of > 4.10 do we desperately need that justifies all the hassle?

kennyadsl · 2018-10-03T10:24:04Z

So the plan is to keep all extension locked to 'factory_bot', '4.10.0' until 2.4 reaches EOL. At that time we'll relax factory_bot in all extensions, right?

I'm just concerned on how to explain that to users that creates new extension, maybe a big comment in the extension template in solidus_cmd?

tvdeyen · 2018-10-03T10:26:26Z

So the plan is to keep all extension locked to 'factory_bot', '4.10.0' until 2.4 reaches EOL. At that time we'll relax factory_bot in all extensions, right?

Sounds goo to me

I'm just concerned on how to explain that to users that creates new extension, maybe a big comment in the extension template in solidus_cmd?

If I would start a new extension today, I won't care for old Solidus version anyway. Especially not the ones that are EOL soon.

jacobherrington · 2018-10-03T10:27:46Z

It should be mentioned in writing extensions as well.

@tvdeyen what about someone who works with a store that is on an older Solidus? They might want to build an extension and would care to support the old versions.

tvdeyen · 2018-10-03T10:32:27Z

It should be mentioned in writing extensions as well.

Yes, do it. But keep in mind that Solidus 2.4 will reach EOL in 6 months anyway 🤷‍♂️

@tvdeyen what about someone who works with a store that is on an older Solidus? They might want to build an extension and would care to support the old versions.

Easy. Add factory_bot 4.10.0 to your Gemfile/gemspec. Done

kennyadsl · 2018-10-03T10:44:11Z

Cough, cough #2835 (comment) 🤣 I'm happy if we all agree that this is a good solution.

solidusio/solidus_auth_devise#130 is not needed anymore, right?

tvdeyen · 2018-10-03T10:46:06Z

solidusio/solidus_auth_devise#130 is not needed anymore, right?

It is. Without that change it would load newer versions of factory_bot even for old versions of Solidus and fail. See https://travis-ci.org/solidusio/solidus_auth_devise/jobs/433011396#L789

kennyadsl · 2018-10-03T12:29:50Z

Oh right, I keep forgetting that the Solidus dev dependencies are not reflected into extensions. 👍

RE "the plan":

Right now:

update all extensions with something similar to solidusio/solidus_auth_devise@d902b8e
merge this PR
update template on and documentation to explain why this is needed (I'm working on this)

Once the EOL of 2.4 is reached (2019-05-07):

just revert solidusio/solidus_auth_devise@d902b8e on all extensions

This is needed to aviod incompatibilities between this extension and old versions of solidus (< 2.5). This can be reverted when Solidus 2.4 reaches EOL. See solidusio/solidus#2835

This is needed to avoid incompatibilities between this extension and old versions of solidus (< 2.5). This can be reverted when Solidus 2.4 reaches EOL. See solidusio/solidus#2835

tvdeyen · 2018-10-12T23:21:13Z

This PR is not needed anymore, right?

kennyadsl · 2018-11-26T08:29:16Z

Right, going to close this since we've found a solution that could be applied directly on extensions and there's no need to change core.

fastjames force-pushed the v2.3-factorybot branch from 18aba2d to becedd2 Compare August 28, 2018 03:35

houndci-bot reviewed Aug 28, 2018

View reviewed changes

fastjames changed the title ~~@wip~~ Change FactoryGirl to FactoryBot in Solidus v2.3 Aug 28, 2018

houndci-bot reviewed Aug 28, 2018

View reviewed changes

fixup: make specs check Array instead of CollectionProxy

31107bc

fastjames force-pushed the v2.3-factorybot branch from 8fc4c55 to e742e19 Compare September 11, 2018 14:20

houndci-bot reviewed Sep 11, 2018

View reviewed changes

houndci-bot reviewed Sep 12, 2018

View reviewed changes

Pin factory_girl to 4.8.1

1c0bcab

`factory_girl` 4.8.1 is the last version that supports being called as such; newer versions of the gem (now named `factory_bot`) have dropped the backcompat files.

fastjames force-pushed the v2.3-factorybot branch from d124cab to 1c0bcab Compare September 12, 2018 13:23

houndci-bot reviewed Sep 12, 2018

View reviewed changes

jacobherrington mentioned this pull request Sep 19, 2018

Spec fixes solidusio/solidus_subscriptions#54

Closed

kennyadsl mentioned this pull request Oct 3, 2018

Update new extensions template solidusio-contrib/solidus_cmd#25

Merged

kennyadsl closed this Nov 26, 2018

Change FactoryGirl to FactoryBot in Solidus v2.3 #2835

Change FactoryGirl to FactoryBot in Solidus v2.3 #2835

Conversation

fastjames commented Aug 28, 2018

houndci-bot left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

kennyadsl commented Sep 2, 2018

fastjames commented Sep 2, 2018

kennyadsl commented Sep 4, 2018

BenMorganIO commented Sep 10, 2018

BenMorganIO commented Sep 10, 2018

tvdeyen commented Sep 11, 2018

kennyadsl commented Sep 11, 2018

tvdeyen commented Sep 11, 2018

kennyadsl commented Sep 11, 2018

tvdeyen commented Sep 11, 2018

kennyadsl commented Sep 11, 2018

houndci-bot left a comment

Choose a reason for hiding this comment

fastjames commented Sep 11, 2018

BenMorganIO commented Sep 11, 2018

BenMorganIO commented Sep 11, 2018

houndci-bot left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

fastjames commented Sep 12, 2018

BenMorganIO commented Sep 21, 2018

kennyadsl commented Oct 2, 2018

tvdeyen commented Oct 3, 2018

tvdeyen commented Oct 3, 2018

kennyadsl commented Oct 3, 2018

tvdeyen commented Oct 3, 2018 • edited Loading

tvdeyen commented Oct 3, 2018

tvdeyen commented Oct 3, 2018

kennyadsl commented Oct 3, 2018

tvdeyen commented Oct 3, 2018

tvdeyen commented Oct 3, 2018 • edited Loading

Actually the fix is even easier

kennyadsl commented Oct 3, 2018

tvdeyen commented Oct 3, 2018 via email

jacobherrington commented Oct 3, 2018

tvdeyen commented Oct 3, 2018

kennyadsl commented Oct 3, 2018

tvdeyen commented Oct 3, 2018 • edited Loading

kennyadsl commented Oct 3, 2018

tvdeyen commented Oct 12, 2018

kennyadsl commented Nov 26, 2018

tvdeyen commented Oct 3, 2018 •

edited

Loading

tvdeyen commented Oct 3, 2018 •

edited

Loading

tvdeyen commented Oct 3, 2018 •

edited

Loading