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

fix shared example path in provider generator spec_helper.rb template #15416

Merged
merged 1 commit into from
Jun 23, 2017

Conversation

jameswnl
Copy link
Contributor

The generated spec_helper would include from the wrong path and so providers would not find the existing shared examples (as in this PR

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

@durandom @bdunne Thoughts? See the rationale in ManageIQ/manageiq-providers-ansible_tower#6

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

I'm wondering if the plugins should add Rails.root.join("spec/support") as well as Engine.root.join("spec/support") .

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

Note that this removes Rails.root.join("spec/shared"), but what I'm questioning is if we need to include Rails.root.join("spec/support")

@durandom
Copy link
Member

The rationale behind this is explained in the README.md
And also in this PR #11414

what I'm questioning is if we need to include Rails.root.join("spec/support")

spec/support from the core is already loaded via the spec_helper from the core.
But then @jameswnl is right, that spec/shared from the core is also loaded via the core spec_helper
But (again :sigh:) I think we should not do this, hence it's listed in the pluggable providers todo dont use spec_helper from core in the providers

So, long story short.
I'd keep this (require core spec/shared)
But also add that require spec/support from the plugin.
Can you also create a spec/support dir then, like here

@jameswnl jameswnl force-pushed the generator-spec-helper branch from 731ad69 to 4a00f91 Compare June 23, 2017 13:17
@jameswnl
Copy link
Contributor Author

I am all for that a plugin's shared examples/cassettes should stay in it's own repo and hence this PR and that and that (although I don't quite understand how/why it is at current state).

@durandom now what you suggested changes are 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.

👍 from me

@@ -10,3 +10,4 @@
# end

Dir[Rails.root.join("spec/shared/**/*.rb")].each { |f| require f }
Dir[File.join(ManageIQ::Providers::<%= class_name %>::Engine.root, 'spec/support/**/*.rb')].each { |f| require f }
Copy link
Member

Choose a reason for hiding this comment

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

for consistency you could. but 🤷‍♂️

Dir[ManageIQ::Providers::<%= class_name %>::Engine.root.join("spec/support/**/*.rb")].each { |f| require f }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, consistency is good, done!

@jameswnl jameswnl force-pushed the generator-spec-helper branch from 4a00f91 to f13f75f Compare June 23, 2017 14:12
@miq-bot
Copy link
Member

miq-bot commented Jun 23, 2017

Checked commit jameswnl@f13f75f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@bdunne bdunne merged commit d5b9dd4 into ManageIQ:master Jun 23, 2017
@bdunne bdunne added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 23, 2017
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