Refactoring the cloud orchestration check_provisioned method #12712
Conversation
|
@miq-bot add_labels automate, enhancement, refactoring, wip |
|
cc @bzwei |
|
@miq-bot remove_label wip |
pkomanek
changed the title
| true | ||
| end | ||
|
|
||
| def state_and_root_vars(service) |
There was a problem hiding this comment.
This method name is not meaningful. The main logic in this method to prepare and call provider refresh.
| provider = service.orchestration_manager | ||
| def main | ||
| task = @handle.root["service_template_provision_task"] | ||
| service = task.try(:destination) |
There was a problem hiding this comment.
We want to test whether service exists and is a type of ServiceOrchestration. Log and exit if not.
| end | ||
| end | ||
|
|
||
| before(:each) do |
There was a problem hiding this comment.
no need for (:each), just before { }
|
|
||
| it "completes check_provisioned step when refresh is done" do | ||
| allow(svc_model_service).to receive(:orchestration_stack).and_return(svc_model_amazon_stack) | ||
| allow(ae_service).to receive(:state_var_exist?) |
There was a problem hiding this comment.
We should not mock ae_service because this is the class we are testing. Methods such as #state_var_exists?, #refresh_may_have_completed? are internal implementation details that should not be mocked.
There was a problem hiding this comment.
@pkomanek
https://github.com/ManageIQ/manageiq/blob/master/spec/support/miq_ae_mock_service.rb#L15
We can pass in all of these as a hash into the miq_ae_mock_service
| expect(ae_service.root['ae_reason']).to eq('Stack was rolled back') | ||
| end | ||
|
|
||
| it "refreshes the provider and waits for it to complete" do |
There was a problem hiding this comment.
Please group all refresh related tests in a context and share the same before block where you can prepare for example to return svc_model_amazon_stack
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
| unless service && service.instance_of?(MiqAeMethodService::MiqAeServiceServiceOrchestration) | ||
| @handle.log(:error, 'Service does not exist or has a different' \ | ||
| ' type from MiqAeServiceServiceOrchestration') | ||
| exit(MIQ_STOP) |
There was a problem hiding this comment.
@pkomanek
We can't use an exit in Automate methods because when we are running in test mode, the exit will end rspec.
You can raise an exception if the service is invalid
There was a problem hiding this comment.
@mkanoor
I know about the ending rspec tests after the getting exit, I was trying to find a way how to catch it today and this line of code expect { described_class.new(ae_service).main }.to_not raise_error seems working for me. I can change that from exit to raise an exception if you want. What do you think?
There was a problem hiding this comment.
@pkomanek yes lets do that.
There was a problem hiding this comment.
@pkomanek
Should we check for the service first and then check the type of service.
Otherwise for nil we would be saying it is a different kind of service
|
Checked commits pkomanek/manageiq@73af5ef~...349df1b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
|
@mkanoor |
pkomanek
deleted the
refactoring_cloud_orchestration_check_provisioned_method
branch
Purpose or Intent
Refactoring the check_provisioned method for the miq_ae_orchestration based on the issue below.
Links [Optional]
#12038