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

Add several ext_management_system factories #14017

Merged
merged 1 commit into from Feb 27, 2017

Conversation

matobet
Copy link
Contributor

@matobet matobet commented Feb 22, 2017

Added several factories useful for writing specs using infra providers with
storages & clusters.

Also added factories for specific versions of the Red Hat infra provider.

@matobet
Copy link
Contributor Author

matobet commented Feb 22, 2017

@miq-bot assign @borod108

Copy link

@borod108 borod108 left a comment

Choose a reason for hiding this comment

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

LGTM if the tests pass :)
This seems useful!

@matobet
Copy link
Contributor Author

matobet commented Feb 22, 2017

@miq-bot assign @durandom

@miq-bot miq-bot assigned durandom and unassigned borod108 Feb 22, 2017
Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

@chrisarcand can you have a look too, I'm not too familiar with FactoryGirl traits and magic

@@ -76,6 +76,28 @@
:parent => :ext_management_system do
end

# Traits

trait :with_clusters do
Copy link
Member

Choose a reason for hiding this comment

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

can we make the traits local to ems?
IIUC this makes them available everywhere, but its only useful in :ems, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@durandom done

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

LGTM

@miq-bot assign @chrisarcand

@chrisarcand please review too and merge at your discretion

Copy link
Member

@chrisarcand chrisarcand left a comment

Choose a reason for hiding this comment

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

TBH I'd say you don't really need the transient count attributes. Normally traits just provide something that you want easy examples of that you don't need a fine grain on the content for. That is, you probably don't really care about setting the exact number of clusters or storages, you just want some, so you just say :with_storages and call it good. If a test really requires setting a certain number of storages/clusters, the test can just specifically handle that without using a general trait.

However, it's not really detrimental and you can leave them if you think it's really useful. (Note in your other PR you might as well remove the 3 count that you have in there as you aren't doing anything with different storage/cluster counts anyway)

end

trait :with_storages do
ignore do
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# Traits

trait :with_clusters do
ignore do
Copy link
Member

Choose a reason for hiding this comment

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

ignore is deprecated; prefer transient instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Added several factories useful for writing specs using infra providers with
storages & clusters.

Also added factories for specific versions of the Red Hat infra provider.
@matobet
Copy link
Contributor Author

matobet commented Feb 24, 2017

@chrisarcand Thank you, I have fixed the occurrences of ignore. Maybe it is a bit overgeneralization with the _count parameters but I wanted these convenience traits to be a bit extensible. For example when I wanted to test a provider that has exactly one cluster, but I don't care enough to create it manually. That it defaults to "several" (3) seemed to me a reasonable default.

I have also fix the dependent PR to not pass the _count parameters and use the default.

@miq-bot
Copy link
Member

miq-bot commented Feb 24, 2017

Checked commit matobet@c7be891 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@chrisarcand chrisarcand merged commit e9c7ac9 into ManageIQ:master Feb 27, 2017
@chrisarcand chrisarcand added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 27, 2017
@matobet matobet deleted the v2v-ems-factories branch February 28, 2017 09:21
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.

None yet

6 participants