Added Lenovo vendor to physical server factory #15129
Conversation
|
@MaysaMacedo can you link to where/why this is needed? Are you adding a validation to PhysicalServer on this attribute? |
|
This is needed for PR(#1173) |
|
@MaysaMacedo I don't see it being used anywhere? In general, it's best only to add defaults to factories if there's a validation that requires them. If that value is important to the thing you're testing, it's better expressed in the body of the test when you actually build the object. |
|
@MaysaMacedo ah I see now it's being added as part of #15128 - can you link to that in your description? |
|
Sure! |
|
@juliancheal - looks like there are a few PRs that are interdependent, can you review them collectively, thanks. |
|
@miq-bot assign @juliancheal |
spec/factories/physical_server.rb
Outdated
| @@ -1,4 +1,5 @@ | |||
| FactoryGirl.define do | |||
| factory :physical_server do | |||
| vendor "Lenovo" | |||
There was a problem hiding this comment.
Shouldn't this be lower case because of https://github.com/ManageIQ/manageiq/pull/15128/files#diff-43874eb28d9d57e685f11f1c9ec137c6R10 ?
|
Also, I would add this change to the PR that requires it, I don't think it's enough to stand on its own since it doesn't fix a broken test or add any new tests. |
|
Checked commits MaysaMacedo/manageiq@4a68444~...fea5a63 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
This PR added:
Related to #15128