Skip to content

Replace machinist with FactoryBot for test data setup#5121

Open
philippthun wants to merge 1 commit into
cloudfoundry:mainfrom
sap-contributions:factory-bot-migration
Open

Replace machinist with FactoryBot for test data setup#5121
philippthun wants to merge 1 commit into
cloudfoundry:mainfrom
sap-contributions:factory-bot-migration

Conversation

@philippthun

@philippthun philippthun commented May 23, 2026

Copy link
Copy Markdown
Member

Per ADR 0015. machinist 1.0.6 has had no upstream activity since 2013 and machinist 2.0 is not a viable upgrade (no Sequel adapter, no Sham, non-persisting make). FactoryBot is actively maintained, has trivial Sequel integration, and provides equivalents for every pattern used by the existing blueprints.

Convert ~11k call sites from Model.make / Model.make_unsaved to create(:foo) / build(:foo), replace ~130 blueprints with factory definitions under spec/support/factory_definitions/, and remove the machinist gem along with spec/support/fakes/blueprints.rb and spec/support/machinist_monkey_patch.rb.

Key technical decisions (see ADR 0015):

  • Global to_create { |i| i.save; i.refresh } in spec/support/factories.rb. This matches machinist's Sequel adapter which both saved and refreshed; the refresh prevents stale-cache bugs whenever a factory's after-create hook mutates associations.
  • Sham is preserved as a thin shim (spec/support/sham_shim.rb) that delegates Sham.<name> to FactoryBot.generate(:sham_<name>) sequences defined in spec/support/factories.rb. The shim mirrors the original Sham.define block 1:1 so existing call sites need no edits.
  • Dynamic class -> factory-name conversion via klass.name.demodulize.underscore.to_sym is used in matchers and shared examples, so generic helpers continue to look up the right factory when given any model class.
  • Named blueprints become traits: Foo.blueprint(:bar) turns into a trait :bar on the :foo factory; Foo.make(:bar, x: 1) becomes create(:foo, :bar, x: 1).
  • build replaces make_unsaved for the (rare) cases that wanted an unsaved instance.
  • :droplet_model only auto-sets itself as the app's current droplet when no app: override is supplied (set_as_current_droplet { app == :unset }), matching machinist's blueprint where the default app block only ran in that case. Specs that previously relied on the auto-set side effect when passing app: explicitly call app.update(droplet:) just as the pre-migration versions did.
  • :revision_sidecar_process_type_model builds its parent with the :no_process_types trait so FactoryBot.lint does not collide with the parent's after-create web row on the (revision_sidecar_guid, type) unique constraint.
  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@philippthun philippthun force-pushed the factory-bot-migration branch from a98ad52 to 29d0f02 Compare May 23, 2026 08:39
@philippthun philippthun force-pushed the factory-bot-migration branch 2 times, most recently from 0d64166 to f390170 Compare May 23, 2026 22:29
Comment thread Gemfile
group :test do
gem 'codeclimate-test-reporter', '>= 1.0.8', require: false
gem 'machinist', '~> 1.0.6'
gem 'factory_bot', '~> 6.5'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the reason for pinning the version?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also pin other test gems (rspec, rubocop). Dependabot will update it accordingly.

Comment thread spec/support/factories.rb Outdated
Comment thread spec/support/factories.rb Outdated
Comment on lines +46 to +48
# Sequences that mirror the original Sham.define block from
# spec/support/fakes/blueprints.rb byte-for-byte. The Sham compatibility
# shim in spec/support/sham_shim.rb delegates to these.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept a single line commit.

Comment thread spec/support/factories.rb Outdated
Comment thread spec/support/factories.rb Outdated
Comment thread spec/support/factories.rb Outdated
Comment thread spec/support/factory_definitions/user.rb Outdated
@philippthun philippthun force-pushed the factory-bot-migration branch from f390170 to 87aae6e Compare June 16, 2026 10:37
Per ADR 0015. machinist 1.0.6 has had no upstream activity since 2013
and machinist 2.0 is not a viable upgrade (no Sequel adapter, no Sham,
non-persisting make). FactoryBot is actively maintained, has trivial
Sequel integration, and provides equivalents for every pattern used
by the existing blueprints.

Convert ~11k call sites from Model.make / Model.make_unsaved to
create(:foo) / build(:foo), replace ~130 blueprints with factory
definitions under spec/support/factory_definitions/, and remove the
machinist gem along with spec/support/fakes/blueprints.rb and
spec/support/machinist_monkey_patch.rb.

Key technical decisions (see ADR 0015):

- Global to_create { |i| i.save; i.refresh } in
  spec/support/factories.rb. This matches machinist's Sequel adapter
  which both saved and refreshed; the refresh prevents stale-cache
  bugs whenever a factory's after-create hook mutates associations.

- Sham is preserved as a thin shim (spec/support/sham_shim.rb) that
  delegates Sham.<name> to FactoryBot.generate(:sham_<name>) sequences
  defined in spec/support/factories.rb. The shim mirrors the original
  Sham.define block 1:1 so existing call sites need no edits.

- Dynamic class -> factory-name conversion via
  klass.name.demodulize.underscore.to_sym is used in matchers and
  shared examples, so generic helpers continue to look up the right
  factory when given any model class.

- Named blueprints become traits: Foo.blueprint(:bar) turns into a
  trait :bar on the :foo factory; Foo.make(:bar, x: 1) becomes
  create(:foo, :bar, x: 1).

- build replaces make_unsaved for the (rare) cases that wanted an
  unsaved instance.

- :droplet_model only auto-sets itself as the app's current droplet
  when no app: override is supplied (set_as_current_droplet
  { app == :unset }), matching machinist's blueprint where the default
  app block only ran in that case. Specs that previously relied on the
  auto-set side effect when passing app: explicitly call
  app.update(droplet:) just as the pre-migration versions did.

- :revision_sidecar_process_type_model builds its parent with the
  :no_process_types trait so FactoryBot.lint does not collide with the
  parent's after-create web row on the (revision_sidecar_guid, type)
  unique constraint.
@philippthun philippthun force-pushed the factory-bot-migration branch from 87aae6e to da85ce8 Compare June 16, 2026 13:48
@philippthun philippthun changed the title Factory bot migration Replace machinist with FactoryBot for test data setup Jun 16, 2026
@philippthun philippthun marked this pull request as ready for review June 16, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants