-
Notifications
You must be signed in to change notification settings - Fork 898
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
[WIP] Use common context for setting up local miq server #19507
Conversation
a732a72
to
7455a28
Compare
rebasing hoping to clear up the sporadic errors |
21d400a
to
fb5b050
Compare
fb5b050
to
9ed0a4e
Compare
Checked commit kbrock@9ed0a4e with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some suggestions. I think it works just fine as is, so I am nit-picking here.
before do | ||
MiqRegion.seed | ||
|
||
@zone = EvmSpecHelper.local_miq_server.zone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we possibly use rspec
constructs for this?
RSpec.shared_context 'with local server', :with_local_miq_server do
let(:local_miq_server) { EvmSpecHelper.local_miq_server }
let(:zone) { local_miq_server }
before do
MiqRegion.seed
zone
end
end
I just see that you are changing zone
to @zone
in some places, and this would be an option for avoiding that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 places ignore the zone
8 places use zone
10 places use @zone
(which I don't like)
others use both miq_server
and zone
but I didn't convert because this code only did @zone
. (but those support the let()
syntax)
Is the idea to avoid assigning the @zone
? Or assign it and slowly go away from it? Not sure how the variable syntax would help us here
before do
_, _, @zone = EvmSpecHelper.create_guid_miq_server_zone
end
@@ -0,0 +1,7 @@ | |||
RSpec.shared_context 'with local server', :with_local_miq_server do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: But could we possibly add some documentation around this?
I know it seems stupid, but it relates to my other comment about using instances variables. Say someone "did" want to use a @zone
variable, and you went along with the changes I suggested. To make modifications to the context, you can do a:
include_context :with_local_miq_server do |var_1, var_2|
# ...
end
(pretty sure... I forget the DSL for it...)
Then you can even pass variables in at runtime as part of the block, allowing this to be somewhat configurable. But without making that obvious in the docs, it might not be apparent to those who are less familiar with the whole feature set of rspec
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: Does this make sense to live in EvmSpecHelper
? Just would make it easier to find the code that this context eventually calls, and there is no reason it has to live in support/contexts/
as far as rspec
is concerned.
That said, if you disagree, I would argue maybe calling this spec/support/context/with_local_miq_server.rb
instead, but that is my two cents.
re-evaluating this change. Does seem it would be better to pull out an |
No description provided.