Remove missing factory spec and all blank factories #16768

bdunne · 2018-01-08T19:53:44Z

Enhance FactoryGirl.create and FactoryGirl.build to super if the factory exists, otherwise call create or new directly.

No longer need to create an empty factory every time a new class is created
No longer need factories for things that shouldn't be factories (SchemaMigrations and join tables)
Many of the factories were incorrect to begin with (not inheriting from the base factory)

cc @jrafanie for the ✂️ 🔥 🎁

Fryguy · 2018-01-08T20:05:35Z

@bdunne Please verify that none of the other pluggable provider repos (or any other repo for that matter) have child factories that might depend on one of these as a parent factory.

chrisarcand · 2018-01-08T20:32:18Z

I generally dislike this as you're breaking [or rather, continuing the MIQ tradition of ignoring] the expectation that when you create the most top-level, generic factory for something it should return for you a valid object with dummy data. By just delegating to create, you can't guarantee that the object is valid at all.

I do realize this argument is against the greater argument of "Do you really need a factory for every single thing?", and that these changes really just remove empty factories that don't work anyway, so... ¯\_(ツ)_/¯ I guess. I'm just sad that it perpetuates bad factory use.

👍

Fryguy · 2018-01-08T23:31:58Z

spec/models/miq_alert_spec.rb

@@ -159,7 +159,7 @@
 it "miq_alert_status.description = miq_alert.description event if overriden by ems_event.description" do
 @alert.evaluate(
 [@vm.class.base_class.name, @vm.id],
- :ems_event => FactoryGirl.create(:ems_event, :message => "oh no!", :type => 'WhateverEvent')
+ :ems_event => FactoryGirl.create(:ems_event, :message => "oh no!")


I think the rationale was that we should never create an event that's of the base type because the table uses STI

EmsEvent appears to be the leaf subclass.

EmsEvent.descendants => [] EventStream.descendants.collect(&:name) => ["EmsEvent", "MiqEvent", "RequestEvent", "CustomEvent"]

Fryguy · 2018-01-08T23:33:38Z

Should it be delegating to create! ? I can't remember if FactoryGirl.create blows up on invalid things or not.

bdunne · 2018-01-09T13:58:19Z

Yeah, on master...

manageiq (master)$ RAILS_ENV=test rails c
irb(main):001:0> require './spec/spec_helper'
=> true
irb(main):002:0> FactoryGirl.create(:customization_template_sysprep)
ActiveRecord::RecordInvalid: Validation failed: Name has already been taken
	from /home/bdunne/.gem/ruby/2.3.3/gems/activerecord-5.0.6/lib/active_record/validations.rb:78:in `raise_validation_error'
	from /home/bdunne/.gem/ruby/2.3.3/gems/activerecord-5.0.6/lib/active_record/validations.rb:50:in `save!'
	from /home/bdunne/.gem/ruby/2.3.3/gems/activerecord-5.0.6/lib/active_record/attribute_methods/dirty.rb:30:in `save!'

bdunne · 2018-01-09T14:45:19Z

Okay @Fryguy I reviewed all of the plugins and added back the factory for :container_template

bdunne · 2018-01-09T16:04:07Z

@chrisarcand For this PR, I'm intending to clean up what we have by removing what shouldn't exist (SchemaMigrations factory), stuff that's wrong, and factories that don't serve a purpose (because they don't set any default values). Also, based on the factories that exist and the ways that they are used, there seems to be a lot of confusion about the purpose and usage of FactoryGirl.

Bigger picture: I'm generally questioning our need for FactoryGirl.
In my opinion, FactoryGirl doesn't provide a lot of value to us (which may be our fault). With the FactoryGirl -> FactoryBot rename, it made me question whether we should update all of our callers or remove it. My hope is that this set of PR's will expose what we are actually using and help us determine whether or not we want to keep the dependency.

himdel · 2018-01-15T19:07:48Z

Heads up from ui-classic:

Failures:

  1) EmsCloudController::EmsCommon #button when Retire Button is pressed for an Orchestration Stack
     Failure/Error: ems = FactoryGirl.create("ems_amazon")
     
     NameError:
       uninitialized constant EmsAmazon
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/ems_common_controller_spec.rb:42:in `block (4 levels) in <top (required)>'

  2) EmsCloudController::EmsCommon #button when the Tagging Button is pressed for a Cloud provider Instance
     Failure/Error: ems = FactoryGirl.create("ems_vmware")
     
     NameError:
       uninitialized constant EmsVmware
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/ems_common_controller_spec.rb:51:in `block (4 levels) in <top (required)>'

  3) EmsCloudController::EmsCommon #button when Retire Button is pressed for a Cloud provider Instance
     Failure/Error: ems = FactoryGirl.create("ems_vmware")
     
     NameError:
       uninitialized constant EmsVmware
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/ems_common_controller_spec.rb:30:in `block (4 levels) in <top (required)>'

  4) PhysicalServerController#button when timelines button is pressed
     Failure/Error: asset_details = FactoryGirl.create(:asset_details)
     
     NameError:
       uninitialized constant AssetDetail
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  5) PhysicalServerController#show_list 
     Failure/Error: asset_details = FactoryGirl.create(:asset_details)
     
     NameError:
       uninitialized constant AssetDetail
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  6) PhysicalServerController#button when Physical Server Manage Policies is pressed
     Failure/Error: asset_details = FactoryGirl.create(:asset_details)
     
     NameError:
       uninitialized constant AssetDetail
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  7) PhysicalServerController#button when Physical Server Tag is pressed
     Failure/Error: asset_details = FactoryGirl.create(:asset_details)
     
     NameError:
       uninitialized constant AssetDetail
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  8) PhysicalServerController#show with invalid id should redirect to #show_list
     Failure/Error: asset_details = FactoryGirl.create(:asset_details)
     
     NameError:
       uninitialized constant AssetDetail
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  9) PhysicalServerController#show with valid id should respond to show
     Failure/Error: asset_details = FactoryGirl.create(:asset_details)
     
     NameError:
       uninitialized constant AssetDetail
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
     # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
     # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  10) PhysicalServerController#show display=timeline 
      Failure/Error: asset_details = FactoryGirl.create(:asset_details)
      
      NameError:
        uninitialized constant AssetDetail
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/controllers/physical_server_controller_spec.rb:14:in `block (2 levels) in <top (required)>'

  11) ReportController#upload_widget_import_file when an upload file is given when the widget importer does not raise an error returns with an import file upload id
      Failure/Error: let(:ret) { FactoryGirl.build_stubbed(:import_file_upload, :id => '123') }
      
      ArgumentError:
        Factory not registered: import_file_upload
      # ./spec/controllers/report_controller_spec.rb:944:in `block (5 levels) in <top (required)>'
      # ./spec/controllers/report_controller_spec.rb:947:in `block (5 levels) in <top (required)>'

  12) ReportController#upload_widget_import_file when an upload file is given when the widget importer does not raise an error imports the widgets
      Failure/Error: let(:ret) { FactoryGirl.build_stubbed(:import_file_upload, :id => '123') }
      
      ArgumentError:
        Factory not registered: import_file_upload
      # ./spec/controllers/report_controller_spec.rb:944:in `block (5 levels) in <top (required)>'
      # ./spec/controllers/report_controller_spec.rb:947:in `block (5 levels) in <top (required)>'

  13) QuadiconHelper#render_quadicon when service related objects renders a quadicon for service
      Failure/Error: item = FactoryGirl.create(obj)
      
      ActiveRecord::RecordInvalid:
        Validation failed: Name can't be blank
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/helpers/quadicon_helper_spec.rb:149:in `block (5 levels) in <top (required)>'

  14) QuadiconHelper#render_quadicon when service related objects renders a quadicon for service_ansible_tower
      Failure/Error: item = FactoryGirl.create(obj)
      
      ActiveRecord::RecordInvalid:
        Validation failed: Name can't be blank
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/helpers/quadicon_helper_spec.rb:149:in `block (5 levels) in <top (required)>'

  15) SecurityGroupController#show render 
      Failure/Error: firewall_rules { build_list :firewall_rule, 3 }
      
      ArgumentError:
        Factory not registered: firewall_rule
      # ./spec/manageiq/spec/factories/security_group.rb:7:in `block (3 levels) in <top (required)>'
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/controllers/security_group_controller_spec.rb:56:in `block (3 levels) in <top (required)>'

  16) SecurityGroupController#edit #edit queues the update action
      Failure/Error: firewall_rules { build_list :firewall_rule, 3 }
      
      ArgumentError:
        Factory not registered: firewall_rule
      # ./spec/manageiq/spec/factories/security_group.rb:7:in `block (3 levels) in <top (required)>'
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/controllers/security_group_controller_spec.rb:119:in `block (3 levels) in <top (required)>'

  17) SecurityGroupController#edit #edit builds edit screen
      Failure/Error: firewall_rules { build_list :firewall_rule, 3 }
      
      ArgumentError:
        Factory not registered: firewall_rule
      # ./spec/manageiq/spec/factories/security_group.rb:7:in `block (3 levels) in <top (required)>'
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/controllers/security_group_controller_spec.rb:119:in `block (3 levels) in <top (required)>'

  18) EmsInfraController breadcrumbs path on a 'show' page of an Infrastructure Provider accessed from Dashboard maintab when previous breadcrumbs path contained 'Cloud Providers' shows 'Infrastructure Providers -> (Summary)' breadcrumb path
      Failure/Error: ems = FactoryGirl.create("ems_vmware")
      
      NameError:
        uninitialized constant EmsVmware
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:15:in `class_from_symbol'
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/controllers/ems_infra_controller_spec.rb:359:in `block (4 levels) in <top (required)>'

  19) EmsPhysicalInfraController breadcrumbs path on a 'show' page of an Physical Infrastructure Provider accessed from Dashboard maintab when previous breadcrumbs path contained 'Cloud Providers' shows 'Physical Infrastructure Providers -> (Summary)' breadcrumb path
      Failure/Error: ems = FactoryGirl.create("ems_physical_infra")
      
      NoMethodError:
        undefined method `ipaddress?' for nil:NilClass
      # /home/himdel/manageiq/app/models/ext_management_system.rb:95:in `hostname_format_valid?'
      # /home/himdel/manageiq/spec/support/missing_factory_helper.rb:9:in `create'
      # ./spec/controllers/ems_physical_infra_controller_spec.rb:99:in `block (4 levels) in <top (required)>'

jrafanie · 2018-01-15T19:57:53Z

Heads up from ui-classic:

@himdel are those errors related to this PR? I don't see those factories being removed here and this PR isn't even merged. 😕

miq-bot · 2018-01-15T20:52:47Z

This pull request is not mergeable. Please rebase and repush.

himdel · 2018-01-16T16:34:35Z

@jrafanie Yes they are, I figured I'd give this a try, to catch the failures before this is merged for once :)

This is from running with ui-classic on master (yesterday) and manageig on this PR.
(I didn't do a second master-master run for comparison, but travis was green at that point, and those failures look suspiciously related.)

bdunne · 2018-01-16T18:34:43Z

That's interesting, because I didn't change anything with :ems_amazon. I'll look into it.

Fryguy · 2018-02-09T15:56:39Z

spec/support/missing_factory_helper.rb

+ end
+
+ def registered_factory_symbols
+ @registered_factory_symbols ||= FactoryGirl.factories.collect { |i| i.name.to_sym }


Since the .include? in this could be called over and over, might be better to put it in a Set.

miq-bot · 2018-02-16T20:42:52Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2018-02-20T01:58:01Z

This pull request is not mergeable. Please rebase and repush.

Enhance FactoryGirl.create and FactoryGirl.build to super if the factory exists, otherwise call create or new directly. - No longer need to create an empty factory every time a new class is created - No longer need factories for things that shouldn't have factories (SchemaMigrations and join tables) - Many of the factories were incorrect to begin with (not inheriting from the base factory)

miq-bot · 2018-02-20T15:32:48Z

Checked commit bdunne@c79a3b4 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

kbrock · 2018-03-14T16:14:49Z

This looks neat.

👍 cuts down on a lot of boiler plate.
👍 We'll need to s/FactoryGirl/FactoryBot/g - so any deletes here are very good.
👎 meta programming tends to be confusing and not be very grep friendly.
👎 this strays from 99% of the projects that use FactoryGirl.

Can we just use Class.create and skip FactoryGirl? (think this is what @chrisarcand also asked)

bdunne · 2018-03-15T18:40:17Z

This also solves the problem on #16728 (comment)

bdunne · 2018-03-15T18:43:25Z

Also, @kbrock I'd love to use Class.create rather than FactoryGirl in most cases, but it sounds like others are not as open to that. I'd also be happy to make this a helper that will wrap FactoryGirl.create / Class.create if that will help move this along.

Fixes test failures after merging ManageIQ/manageiq#16768

bdunne added technical debt test gaprindashvili/yes labels Jan 8, 2018

bdunne requested review from Fryguy and chrisarcand January 8, 2018 19:53

bdunne force-pushed the make_factories_not_required branch 2 times, most recently from 00cfbb4 to 6a5e90a Compare January 8, 2018 23:06

Fryguy reviewed Jan 8, 2018

View reviewed changes

bdunne force-pushed the make_factories_not_required branch from 6a5e90a to a000567 Compare January 9, 2018 14:39

bdunne mentioned this pull request Jan 9, 2018

Remove unnecessary block syntax in factory definition #16776

Merged

bdunne mentioned this pull request Jan 9, 2018

Do we need FactoryGirl? #16778

Closed

chessbyte assigned Fryguy Jan 9, 2018

miq-bot added the unmergeable label Jan 15, 2018

bdunne force-pushed the make_factories_not_required branch from a000567 to 7a79bf3 Compare January 16, 2018 22:53

miq-bot removed the unmergeable label Jan 16, 2018

This was referenced Jan 16, 2018

Factories should be referenced by symbol ManageIQ/manageiq-ui-classic#3259

Merged

Singularize plural model #16833

Merged

Prefer FactoryGirl.build over .build_stubbed ManageIQ/manageiq-ui-classic#3265

Merged

bdunne force-pushed the make_factories_not_required branch from 7a79bf3 to ff96bdc Compare January 17, 2018 21:58

bdunne force-pushed the make_factories_not_required branch from ff96bdc to b6f8d72 Compare February 9, 2018 15:53

Fryguy reviewed Feb 9, 2018

View reviewed changes

bdunne force-pushed the make_factories_not_required branch from b6f8d72 to 1aebba0 Compare February 9, 2018 16:26

miq-bot added the unmergeable label Feb 16, 2018

bdunne force-pushed the make_factories_not_required branch from 1aebba0 to 183f0d6 Compare February 19, 2018 16:58

miq-bot removed the unmergeable label Feb 19, 2018

bdunne force-pushed the make_factories_not_required branch from 183f0d6 to bec4a2b Compare February 19, 2018 18:26

miq-bot added the unmergeable label Feb 20, 2018

bdunne force-pushed the make_factories_not_required branch from bec4a2b to c79a3b4 Compare February 20, 2018 15:26

miq-bot removed the unmergeable label Feb 20, 2018

bdunne mentioned this pull request Mar 15, 2018

Make VimPerformanceTagValue no longer backed by a DB table #16728

Merged

Fryguy merged commit 825a676 into ManageIQ:master Mar 15, 2018

Fryguy added this to the Sprint 82 Ending Mar 26, 2018 milestone Mar 15, 2018

Fryguy deleted the make_factories_not_required branch March 15, 2018 21:52

Fryguy added gaprindashvili/no and removed gaprindashvili/yes labels Mar 15, 2018

bdunne added a commit to bdunne/manageiq-automation_engine that referenced this pull request Mar 16, 2018

The factories are defined as symbols, update callers to use symbols

6184b5c

Fixes test failures after merging ManageIQ/manageiq#16768

This was referenced Mar 16, 2018

The factories are defined as symbols, update callers to use symbols ManageIQ/manageiq-automation_engine#164

Merged

Allow usage of string factories #17165

Merged

NickLaMuro mentioned this pull request Apr 24, 2019

Remove "HACK" MissingFactoryHelper #18680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove missing factory spec and all blank factories #16768

Remove missing factory spec and all blank factories #16768

bdunne commented Jan 8, 2018

Fryguy commented Jan 8, 2018

chrisarcand commented Jan 8, 2018 •

edited

Loading

Fryguy Jan 8, 2018

bdunne Jan 9, 2018

Fryguy commented Jan 8, 2018

bdunne commented Jan 9, 2018 •

edited

Loading

bdunne commented Jan 9, 2018

bdunne commented Jan 9, 2018 •

edited

Loading

himdel commented Jan 15, 2018

jrafanie commented Jan 15, 2018

miq-bot commented Jan 15, 2018

himdel commented Jan 16, 2018 •

edited

Loading

bdunne commented Jan 16, 2018

Fryguy Feb 9, 2018

miq-bot commented Feb 16, 2018

miq-bot commented Feb 20, 2018

miq-bot commented Feb 20, 2018

kbrock commented Mar 14, 2018 •

edited

Loading

bdunne commented Mar 15, 2018

bdunne commented Mar 15, 2018

Remove missing factory spec and all blank factories #16768

Remove missing factory spec and all blank factories #16768

Conversation

bdunne commented Jan 8, 2018

Fryguy commented Jan 8, 2018

chrisarcand commented Jan 8, 2018 • edited Loading

Fryguy Jan 8, 2018

Choose a reason for hiding this comment

bdunne Jan 9, 2018

Choose a reason for hiding this comment

Fryguy commented Jan 8, 2018

bdunne commented Jan 9, 2018 • edited Loading

bdunne commented Jan 9, 2018

bdunne commented Jan 9, 2018 • edited Loading

himdel commented Jan 15, 2018

jrafanie commented Jan 15, 2018

miq-bot commented Jan 15, 2018

himdel commented Jan 16, 2018 • edited Loading

bdunne commented Jan 16, 2018

Fryguy Feb 9, 2018

Choose a reason for hiding this comment

miq-bot commented Feb 16, 2018

miq-bot commented Feb 20, 2018

miq-bot commented Feb 20, 2018

kbrock commented Mar 14, 2018 • edited Loading

bdunne commented Mar 15, 2018

bdunne commented Mar 15, 2018

chrisarcand commented Jan 8, 2018 •

edited

Loading

bdunne commented Jan 9, 2018 •

edited

Loading

bdunne commented Jan 9, 2018 •

edited

Loading

himdel commented Jan 16, 2018 •

edited

Loading

kbrock commented Mar 14, 2018 •

edited

Loading