Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Begin conversion from FactoryGirl to FactoryBot #18279

Merged
merged 5 commits into from
Dec 12, 2018
Merged

Begin conversion from FactoryGirl to FactoryBot #18279

merged 5 commits into from
Dec 12, 2018

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Dec 10, 2018

As you may or may not know, the factory_girl gem was renamed to factory_bot at some point last year. I thought it prudent to update the gem so that any future upgrades will be seamless.

For this PR I have not done a global sub/replace. Instead, this is just a first step that will enable easy transition for both this repository and the other associated repositories. Towards that end I have added a simple constant alias for now, so none of the existing specs will break. The idea is that folks could update the name "organically" through other PR's over time. Once done, the alias can be removed.

Update: on second thought, maybe it's easier to gsub everything up front and force the issue.

@Fryguy
Copy link
Member

Fryguy commented Dec 10, 2018

@miq-bot assign @bdunne

@himdel
Copy link
Contributor

himdel commented Dec 11, 2018

I'm getting..

$ cd manageiq-ui-classic ; bundle exec rake spec
...
/home/himdel/manageiq/spec/support/missing_factory_helper.rb:33:in `<top (required)>': uninitialized constant FactoryGirl (NameError)
	from /home/himdel/manageiq-ui-classic/spec/manageiq/spec/spec_helper.rb:21:in `block in <top (required)>'
	from /home/himdel/manageiq-ui-classic/spec/manageiq/spec/spec_helper.rb:21:in `each'
	from /home/himdel/manageiq-ui-classic/spec/manageiq/spec/spec_helper.rb:21:in `<top (required)>'

Looks like the FactoryGirl = FactoryBot line needs to be somewhere rails autoloader will find.

If I echo "FactoryGirl = FactoryBot" > manageiq/lib/factory_girl.rb it starts passing fine (though I'm not sure that's the right location given this should be spec-only).

EDIT: manageiq/spec/support/factory_girl.rb also works (but not with _helper).

@djberg96
Copy link
Contributor Author

@himdel, Does it go away if you change FactoryGirl to FactoryBot in spec/support/missing_factory_helper.rb?

@himdel
Copy link
Contributor

himdel commented Dec 11, 2018

@djberg96 Well, not really, there are many other files in spec/support mentioning FactoryGirl, and their load order is not defined.

EDIT: actually, it does seem to help sometimes at least. Either I'm testing something wrong, or this depends on file ordering in the filesystem or some phase-of-the-moon thing like that.

@bdunne
Copy link
Member

bdunne commented Dec 11, 2018

I think we should gsub FactoryGirl to FactoryBot in this entire repo in the same PR to avoid conflicts

@djberg96
Copy link
Contributor Author

@bdunne Alright, maybe that would be the better approach. Locally, even after I did that, it still caused problems for the provider repos, but they can catch up.

@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2018

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

@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2018

Checked commits https://github.com/djberg96/manageiq/compare/8f00b6b274ad3a62d104bb78fc49361c9b8589fa~...b9da4050acce3920b78f956e9076bdf24113d5aa with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
576 files checked, 187 offenses detected

spec/factories/chargeback_rate_detail.rb

spec/factories/small_environment.rb

spec/lib/ems_event_helper_spec.rb

spec/lib/extensions/ar_virtual_spec.rb

spec/lib/miq_preloader_spec.rb

spec/lib/rbac/filterer_spec.rb

spec/lib/rbac_spec.rb

spec/lib/services/resource_sharer_spec.rb

spec/lib/tasks/evm_application_spec.rb

spec/models/assigned_server_role_spec.rb

spec/models/chargeback_container_project_spec.rb

spec/models/container_deployment_spec.rb

spec/models/container_label_tag_mapping_spec.rb

spec/models/custom_attribute_spec.rb

spec/models/dialog_field_tag_control_spec.rb

spec/models/dialog_field_text_box_spec.rb

spec/models/disk_spec.rb

spec/models/drift_state/purging_spec.rb

spec/models/ems_refresh/metadata_relats_spec.rb

spec/models/hardware_spec.rb

spec/models/host_spec.rb

spec/models/manageiq/providers/inventory/persister/helpers/spec_mocked_data.rb

spec/models/metric/processing_spec.rb

spec/models/metric/purging_spec.rb

spec/models/miq_action_spec.rb

spec/models/miq_alert_spec.rb

spec/models/miq_enterprise_spec.rb

spec/models/miq_group_spec.rb

spec/models/miq_policy_spec.rb

spec/models/miq_provision_request_spec.rb

spec/models/miq_provision_request_template_spec.rb

spec/models/miq_report_result/purging_spec.rb

spec/models/miq_report_result_spec.rb

spec/models/miq_report_spec.rb

spec/models/miq_request_spec.rb

  • ❗ - Line 197, Col 144 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

spec/models/miq_server/worker_monitor_spec.rb

spec/models/miq_widget_set_spec.rb

spec/models/miq_widget_spec.rb

spec/models/mixins/relationship_mixin_spec.rb

spec/models/notification_spec.rb

spec/models/partition_alignment_spec.rb

spec/models/service_ansible_tower_spec.rb

spec/models/service_template_provision_request_quota_spec.rb

spec/models/service_template_provision_request_spec.rb

spec/models/service_template_transformation_plan_request_spec.rb

spec/models/service_template_transformation_plan_task_spec.rb

spec/models/storage_spec.rb

spec/models/tenant_spec.rb

spec/models/user_spec.rb

spec/models/vim_performance_analysis_spec.rb

spec/models/vim_performance_state_spec.rb

spec/models/vm_or_template_spec.rb

spec/models/vm_scan_spec.rb

spec/models/vmdb_database_spec.rb

spec/models/vmdb_index_spec.rb

spec/models/vmdb_table_evm_spec.rb

spec/models/zone_spec.rb

spec/support/quota_helper.rb

spec/tools/copy_reports_structure_spec.rb

@himdel
Copy link
Contributor

himdel commented Dec 11, 2018

The UI specs are passing for me locally now, thanks! 👍

@himdel
Copy link
Contributor

himdel commented Dec 11, 2018

Will this be backported to hammer?

If not, we should probably add FactoryBot = FactoryGirl to hammer (and gaprindashvili?) to ease backporting..

@bdunne
Copy link
Member

bdunne commented Dec 11, 2018

@djberg96 Can you retest with the provider repos you had failures on earlier?

@djberg96
Copy link
Contributor Author

@bdunne I ran it normally and with 3 of the providers locally (Google, Amazon and Azure). All passed.

@djberg96
Copy link
Contributor Author

@bdunne Want me to try to cleanup these rubocop warnings, too?

@bdunne bdunne added the test label Dec 12, 2018
@bdunne bdunne merged commit cdfb8d6 into ManageIQ:master Dec 12, 2018
@bdunne bdunne added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 12, 2018
@bdunne
Copy link
Member

bdunne commented Dec 12, 2018

@djberg96 Would you mind creating a PR to hammer that only aliases FactoryBot to FactoryGirl? That would make backports much easier for @simaishi and the rest of the team.

@NickLaMuro NickLaMuro mentioned this pull request Dec 8, 2020
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.

5 participants