Support factory bot rails #23613

jrafanie · 2025-10-08T16:11:57Z

Extracted from #23600

TODO:

Green cross repo

Factory bot rails' railtie finds factories during rails initialization: https://github.com/thoughtbot/factory_bot_rails/blob/dfdef4c70a76d36058cf77fc1ccd9904a2bf0beb/lib/factory_bot_rails/railtie.rb#L42
- Avoid hitting the database at factory definition time
- Reload after changing file paths to play nice with FactoryBotRails
Use the Rails app path and not a general relative path
Use pathnames to follow factory bot rails conventions

jrafanie · 2025-10-08T16:12:24Z

@miq-bot cross-repo-tests /all

From Pull Request: ManageIQ/manageiq#23613

kbrock

This looks nice.

I like getting region_remote our of the definition up top. (think that becomes a global)

The use cases that seem different are core, ui-classic, and providers. This tests them all

Fryguy · 2025-10-08T16:49:44Z

spec/factories/miq_region.rb

+    sequence(:region) do |region|
+      region_remote = MiqRegion.my_region_number


This is a subtle breaking change. If I recall correctly we did it outside the block on purpose to get the actual database region number independent of what the test wants. This is so that when we stub MiqRegion it builds numbers in the correct region.

It was introduced in #17522

haha, I don't remember merging that. 🤣

I'll try to recreate that error with this change.

note that the original was sporadic though I don't know why

I've run it hundreds of times so far, no failure yet. 🤞

Oh I think I know how to make it fail - @jrafanie Force your database to be a low region number, like 1.

Ok, I ran it another few hundred times with no failure. I'm leaning towards keeping it as is and we'll have to see if it causes sporadic failures. Maybe there is another test that will recreate it easier.

What I think the old code did was to play nice with MiqRegion.seed, I think

MiqRegion.seed will create a region record in the current region, say 10000000001. Then FactoryBot.create(:miq_region) will create a region record with a sequence number. If we happen to be in the same region that defines the database, it will also create a sequenced number like 10000000001 and it collides, so in that case, the code does +1.

I still don't think the previous code is "correct", but i can see how it can avoid most collisions.

Actually the more I think of it, this can only happen in region 0, unless we just so happen to create 1 trillion regions in tests :D

The only real remote region tests I remember are with classifications.

It was very tricky since you can claim any region you want in your specs, the real region is from the database.

But to find errors like this, we seeded the test database with different regions. Don't remember seeing any issues with regions in a long time.

(and now pods only do region 0 so... :) )

spec/support/factory_bot_helper.rb

Factory bot rails's railtie tries to find factory definitions during the rails initialization hooks at rails startup. Previously, the miq_region.rb factory was running MiqRegion.my_region_number early to avoid tests that might stub my_region_number later. This caused a failure in our bin/test_database_connectivity test as we were now connecting to the database during our initializers. To fix this, we can lazily evaluate the underlying method, region_number_from_sequence, called by my_region_number when we create the region. Here is the backtrace of the failure that was occurring in the test: ``` /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/activerecord-id_regions-0.5.0/lib/active_record/id_regions.rb:128:in `region_number_from_sequence' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/activerecord-id_regions-0.5.0/lib/active_record/id_regions.rb:139:in `discover_my_region_number' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/activerecord-id_regions-0.5.0/lib/active_record/id_regions.rb:18:in `my_region_number' /home/runner/work/manageiq/manageiq/spec/factories/miq_region.rb:2:in `block in <main>' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/syntax/default.rb:37:in `instance_eval' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/syntax/default.rb:37:in `run' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/syntax/default.rb:7:in `define' /home/runner/work/manageiq/manageiq/spec/factories/miq_region.rb:1:in `<main>' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/find_definitions.rb:20:in `load' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/find_definitions.rb:20:in `block (2 levels) in find_definitions' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/find_definitions.rb:19:in `each' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/find_definitions.rb:19:in `block in find_definitions' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/find_definitions.rb:15:in `each' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot-6.5.5/lib/factory_bot/find_definitions.rb:15:in `find_definitions' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/factory_bot_rails-6.5.1/lib/factory_bot_rails/railtie.rb:42:in `block in <class:Railtie>' /home/runner/work/manageiq/manageiq/vendor/bundle/ruby/3.3.0/gems/activesupport-7.2.2.2/lib/active_support/lazy_load_hooks.rb:94:in `block in execute_hook' ```

FactoryBotRails calls find_definitions in it's railtie as rails initializes and we can't assume we're the first one to call. Instead we call reload, which will reset things and then call find_definitions. find_definitions isn't reentrant so if it's already run, a second call will try to register the same factories from the same files and lead to the following error: ``` FactoryBot::DuplicateDefinitionError: Factory already registered: account ```

factory bot rails converts the default factory bot directory paths to pathnames based on rails root: https://github.com/thoughtbot/factory_bot_rails/blob/dfdef4c70a76d36058cf77fc1ccd9904a2bf0beb/lib/factory_bot_rails/railtie.rb#L50-L52 * Ensure any duplicate pathnames are removed. * Clarify the rationale for using definition_path_files and reload.

Fryguy · 2025-10-08T19:40:53Z

spec/factories/miq_region.rb

  factory :miq_region do
-    sequence(:region) { |region| region == region_remote ? region + 1 : region }
+    sequence(:region) do |region|
+      region == ApplicationRecord.region_number_from_sequence ? region + 1 : region


worked this out with Joe, and this is effectively the same as before, but done lazily, so this should work.

jrafanie requested a review from Fryguy as a code owner October 8, 2025 16:11

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Oct 8, 2025

Running tests for jrafanie

7005937

From Pull Request: ManageIQ/manageiq#23613

miq-bot mentioned this pull request Oct 8, 2025

[BOT] Cross repo test for ManageIQ/manageiq#23613 ManageIQ/manageiq-cross_repo-tests#993

Closed

jrafanie assigned Fryguy Oct 8, 2025

jrafanie changed the title ~~[WIP] Support factory bot rails~~ Support factory bot rails Oct 8, 2025

kbrock added the test label Oct 8, 2025

kbrock approved these changes Oct 8, 2025

View reviewed changes

Fryguy reviewed Oct 8, 2025

View reviewed changes

spec/support/factory_bot_helper.rb Show resolved Hide resolved

jrafanie force-pushed the support-factory-bot-rails branch from 8f90ab8 to 2ca94e9 Compare October 8, 2025 17:43

jrafanie added 4 commits October 8, 2025 15:22

Use the Rails app path and not a general relative path

78ca4b1

jrafanie force-pushed the support-factory-bot-rails branch from 2ca94e9 to 028e3d0 Compare October 8, 2025 19:26

Fryguy reviewed Oct 8, 2025

View reviewed changes

Fryguy approved these changes Oct 8, 2025

View reviewed changes

Fryguy merged commit cd61157 into ManageIQ:master Oct 8, 2025
9 checks passed

Fryguy mentioned this pull request Oct 8, 2025

Add cypress-on-rails support #23600

Merged

jrafanie deleted the support-factory-bot-rails branch October 14, 2025 15:36

		sequence(:region) do \|region\|
		region_remote = MiqRegion.my_region_number

Support factory bot rails #23613

Support factory bot rails #23613

Conversation

jrafanie commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrafanie commented Oct 8, 2025

Uh oh!

kbrock left a comment

Choose a reason for hiding this comment

Uh oh!

Fryguy Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbrock Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jrafanie commented Oct 8, 2025 •

edited

Loading

Fryguy Oct 8, 2025 •

edited

Loading

kbrock Oct 8, 2025 •

edited

Loading