Solidus: 2.11.4 breaks modified factories in default paths

[tmtrademarked]

In 2.11.4, attempting to modify a factory defined in the spec/factories/ directory of an application ends up failing with a Factory not registered error. This same factory worked just fine in 2.11.3, but fails in 2.11.4.

For context, here's an example of one of our factories which extends a built in Solidus factory:

# file: spec/factories/option_types.rb

# frozen_string_literal: true
require 'spree/testing_support/factories'

FactoryBot.modify do
  factory :option_type do
    description { 'Testing testing testing' }
  end
end

Solidus Version:
2.11.4

To Reproduce
1) Author a factory that modifies a built-in factory as described above.
2) Run any spec of your application.

Current behavior
Fails to run with Factory not registered error.

Expected behavior
Specs should run and find the modified factories.

Context

It looks like the problem is this line here: https://github.com/solidusio/solidus/blob/master/core/lib/spree/testing_support.rb#L36

Assuming I'm understanding the behavior of the FactoryBot railtie correctly, I believe this puts "spec/factories/_factory.rb" into the FactoryBot.definition_file_paths list first, which means it attempts to load these factories before it loads the default Spree factories.

It seems like this may be fixable either by reordering the traversal of the file definitions for FactoryBot, or perhaps by documenting a better naming convention/location for overrides of Solidus default factories.

[jarednorman]

@elia This seems to relate to your work on #3814. Do you have any thoughts here?

[elia]

Assuming I'm understanding the behavior of the FactoryBot railtie correctly, I believe this puts "spec/factories/_factory.rb" into the FactoryBot.definition_file_paths list _first_, which means it attempts to load these factories _before_ it loads the default Spree factories.

It seems like this may be fixable either by reordering the traversal of the file definitions for FactoryBot, or perhaps by documenting a better naming convention/location for overrides of Solidus default factories.

This is correct, unfortunately all the different workarounds that were employed to make factories work with solidus in all the applications out there are going to pose an issue, and the loading sequence was not addressed in the original fix.

Anyway the solution seems to involve unshifting the solidus factory path instead of appending them, I'm going to send a PR to fix that briefly, in the meantime you can try this in your app:

Add this into config/application.rb (not inside an initializer because of the loading sequence):

# config/application.rb

module MyApp
  class Application < Rails::Application
    # …
    require "spree/testing_support"
    config.factory_bot.definition_file_paths.unshift *Spree::TestingSupport.factory_bot_paths
  end
end

and remove the require from spec/factories/option_types.rb.

With that I'm assuming you have factory_bot_rails in your bundle with the default rails configuration for factories.

Please let me know if the fix works 👍

[elia]

update

if you have an older version of FactoryBot like ~> 4.8 the code in application.rb should look like this:

    if defined? FactoryBotRails
      initializer after: "factory_bot.set_factory_paths" do
        require "spree/testing_support"
        FactoryBot.definition_file_paths.unshift *Spree::TestingSupport.factory_bot_paths
      end
    end

BTW it's advisable to use ~> 4.8 since Solidus factories have some corner cases that will brake on newer versions.

[tmtrademarked]

Yup, we're using ~> 4.8 for factory_bot_rails as you suggested in your original PR. I used your suggested workaround, and it appears to have worked - thanks for the suggestion! Once the fix is backported to a release, I can test out removing the workaround in our code.

[kennyadsl]

I'll leave some documentation here about our findings in improving factories loading. I hope we'll be able to document this process properly (maybe we can use a GitHub discussion) once everything is clear.

After we merge #3907 (and backport it to 2.11), here's the correct setup for stores to configure their specs factories using factory_bot_rails:

You probably have something like this in your spec/spec_helper.rb or spec/support/factory_bot.rb:

# frozen_string_literal: true

require 'spree/testing_support/factories'
require 'solidus_subscriptions/testing_support/factories'

RSpec.configure do |config|
  config.include FactoryBot::Syntax::Methods
end

FactoryBot.definition_file_paths = [Rails.root.join('spec/factories')]
FactoryBot.find_definitions

While this code should still work, it is now deprecated because spree/testing_support/factories is still cherry-picking all factories without using the new way of loading factories of factory_bot_rails. You should see deprecation messages similar to:

DEPRECATION WARNING: Please do not cherry-pick factories, this is not well supported by FactoryBot. Use `require "spree/testing_support/factories"` instead. (called from <top (required)> at ~/solidus/core/lib/spree/testing_support/factories/variant_property_rule_factory.rb:4)
DEPRECATION WARNING: Please do not cherry-pick factories, this is not well supported by FactoryBot. Use `require "spree/testing_support/factories"` instead. (called from <top (required)> at ~/solidus/core/lib/spree/testing_support/factories/variant_property_rule_value_factory.rb:4)
DEPRECATION WARNING: Please do not cherry-pick factories, this is not well supported by FactoryBot. Use `require "spree/testing_support/factories"` instead. (called from <top (required)> at ~/solidus/core/lib/spree/testing_support/factories/zone_factory.rb:4)

To remove these messages you can change your code to make it safely rely on the new factories loading mechanism provided by FactoryBot. Remove all the code related to factories loading in your spec/spec_helper.rb or spec/support/factory_bot.rb and add the following code in your application.rb:

if defined?(FactoryBotRails)
  initializer "my_app.set_factory_paths", after: "spree.factory_bot.set_factory_paths" do
    require "spree/testing_support/factory_bot"
    Spree::TestingSupport::FactoryBot.check_version

    FactoryBot.definition_file_paths = [
      Spree::TestingSupport::FactoryBot.definition_file_paths,
      SolidusSubscriptions::Engine.root.glob('lib/solidus_subscriptions/testing_support/factories/**/*_factory.rb').map { |path| path.sub(/.rb\z/, '') },
      Rails.root.join('spec/factories')
    ].flatten
  end
end

This code will add the following pieces to the FactoryBot definition_file_paths array:

Spree::TestingSupport::FactoryBot.definition_file_paths

This will load all solidus factories. It should be added as the first element of the array because next items could depend on the definitions in Solidus.

The line

SolidusSubscriptions::Engine.root.glob('lib/solidus_subscriptions/testing_support/factories/**/*_factory.rb').map { |path| path.sub(/.rb\z/, '') },

tells FactoryBot to also consider the factories of the solidus_subscription extension. You can use any extension you need here of course.

The last item is

Rails.root.join('spec/factories')

and just load the factories of the project.

The next piece of the puzzle to improve this code is providing a standard way for extensions to declare their factories so that we avoid that hacky code to load their factories. A nice API could be:

    FactoryBot.definition_file_paths = [
      Spree::TestingSupport::FactoryBot.definition_file_paths,
      SolidusSubscriptions::FactoryBot.definition_file_paths,
      Rails.root.join('spec/factories')
    ].flatten
  end

We should try to do this in solidus_support directly, so that it is done one time and works for the extensions.

[kennyadsl]

Closing in favor of https://github.com/solidusio/solidus/discussions/4014