Adding factory to ExtManagementSystem with invalid credentials #16781
Conversation
|
@chrisarcand Please review. |
There was a problem hiding this comment.
Instead of a specific separate factory for ems_physical_infra, this should utilize FactoryGirl traits; you can see a couple in use within this factory file.
Just make a trait called :with_invalid_authentication which only has your after block with the authentication(s); then in your spec, use FactoryGirl.create(:ems_physical_infra, :with_invalid_authentication). This has the added benefit of now being able to have invalid authentication for any of the other ems types.
douglasgabriel
force-pushed
the
add_factory_ems_ph_with_invalid_credentials
branch
4 times, most recently
from
de9a376 to
0604e93
Compare
|
Awesome! It's done @chrisarcand |
| after(:create) do |x| | ||
| x.authentications = [FactoryGirl.build(:authentication, :resource => x, :status => "invalid")] | ||
| end | ||
| end |
There was a problem hiding this comment.
Right idea, but why inside the :ems_physical_infra factory? These authentications are the same for all providers, no? If you place it next to the other traits in :ext_management_system, this trait can then be used with all the other child factories in this file (including :ems_physical_infra).
| :parent => :ext_management_system do | ||
| trait :with_invalid_authentication do | ||
| after(:create) do |x| | ||
| x.authentications = [FactoryGirl.build(:authentication, :resource => x, :status => "invalid")] |
There was a problem hiding this comment.
Prefer:
x.authentications << FactoryGirl.build(:authentication, :status => "invalid")
douglasgabriel
force-pushed
the
add_factory_ems_ph_with_invalid_credentials
branch
from
0604e93 to
6d5c25c
Compare
| :parent => :ext_management_system do | ||
| end | ||
|
|
||
| factory :ems_physical_infra_with_invalid_authentication, |
There was a problem hiding this comment.
It looks like something went wrong during a rebase and brought this factory back as well as all of the do / end blocks I deleted :)
There was a problem hiding this comment.
I do think this should be the end of those blocks, indeed.
douglasgabriel
force-pushed
the
add_factory_ems_ph_with_invalid_credentials
branch
2 times, most recently
from
82e7f66 to
988204b
Compare
|
|
||
| trait :with_invalid_authentication do | ||
| after(:create) do |x| | ||
| x.authentications << FactoryGirl.build(:authentication, :resource => x, :status => "invalid") |
There was a problem hiding this comment.
Looking good! Last thing: It's good to reach for build first when you can, but one to many relationships may or may not work with initialized-but-not-persisted objects on them (as often, you're testing something where something else does a lookup on this authentications, for example), depending on what you're doing. As this is an optional trait that won't be used unless people explicitly want it, you likely want to use FactoryGirl.create here.
There was a problem hiding this comment.
I got it! create is really better in this case, thanks for pointing
douglasgabriel
force-pushed
the
add_factory_ems_ph_with_invalid_credentials
branch
from
988204b to
2b23145
Compare
|
Checked commit douglasgabriel@2b23145 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
|
||
| trait :with_invalid_authentication do | ||
| after(:create) do |x| | ||
| x.authentications << FactoryGirl.create(:authentication, :resource => x, :status => "invalid") |
There was a problem hiding this comment.
When you use the << operator on an association, you shouldn't need the :resource => x in create
douglasgabriel
force-pushed
the
add_factory_ems_ph_with_invalid_credentials
branch
from
2b23145 to
49ec4be
Compare
douglasgabriel
deleted the
add_factory_ems_ph_with_invalid_credentials
branch
This PR adds a factory to an ExtManagementSystem with invalid credentials