ManageIQ / manageiq-providers-hawkular

ManageIQ plugin for the Hawkular provider
8 stars 33 forks source link

Add test coverage for connect and verify_credentials #102

Closed tumido closed 6 years ago

tumido commented 6 years ago

Enhance test coverage of MiddlewareManager to provide test cases for:

Based on suggestion by @israel-hdez here. A bit of competition PR to https://github.com/ManageIQ/manageiq-providers-hawkular/pull/86 from @aljesus, replacing 4 test cases with more generic ones.

Depends on https://github.com/ManageIQ/manageiq-providers-hawkular/pull/100

tumido commented 6 years ago

cc @abonas

tumido commented 6 years ago

Note: the allow_any_instance_of is used due to necessity to raise exceptions when the ::Hawkular::Client is already instantiated. The exception handling happens later in the code so we can't raise errors on :new. The way to avoid such allow_any_instance_of call would be:

let(:client) { ::Hawkular::Client.new(:entrypoint => "", :credentials => {}, :options => {}) }
...
before do
    allow(::Hawkular::Client).to receive(:new).and_return(client)
end
...
it "invalid credentials" do
    allow(client).to receive(:inventory).and_raise(...)
    except...
end

If this is prefered by the team, I'm ok to change it. Though I personally prefer to have the ::Hawkular::Client instantiated by the MiddlewareManager instead of mocking it. This would help in cases when an exception on client becomes risen during the :new call. The mocking of whole client may hide it.

tumido commented 6 years ago

@miq-bot assign @cfcosta please review, thanks! ;)

tumido commented 6 years ago

@miq-bot add_label test

tumido commented 6 years ago

@cfcosta, the part in your review about mocks - you're referring to the first part only (testing the connection) or also to the second part (for the credentials validation)?

cfcosta commented 6 years ago

@tumido for credential validations is a little better, I think a better way to do so is to actually inject the connection class into the method as an optional parameter. Then at least we can remove the dependency on allow_any_instance_of.

tumido commented 6 years ago

@cfcosta, @israel-hdez, I did a second iteration of this, which completely rewrites it and makes it able to compare against client's internal @state. Can you please review it for me? Also, it still depends on the https://github.com/ManageIQ/manageiq-providers-hawkular/pull/100 so it will fail for raw_connect.

miq-bot commented 6 years ago

Checked commit https://github.com/tumido/manageiq-providers-hawkular/commit/ca1111b1cb14f051515c6e3b83d226e8bcbd5bba with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 3 files checked, 0 offenses detected Everything looks fine. :cookie:

tumido commented 6 years ago

@miq-bot add_label gaprindashvili/no

israel-hdez commented 6 years ago

To merge this, #100 needs to go in. Is it ready? Reading the comments on it, I'm not sure.

tumido commented 6 years ago

Closing, won't merge. Repo abandoned. :no_entry: