Provide extensive unit tests for legacy refresher #12
Conversation
|
@gberginc may I ask you to perform a first review pass? You're know to be an experienced |
There was a problem hiding this comment.
Thanks for extending the specs @miha-plesko!
Please, review my comments.
| :orchestration_stack_id => nil, | ||
| :type => "ManageIQ::Providers::Nuage::NetworkManager::NetworkGroup" | ||
| ) | ||
| expect(g2.cloud_subnets.count).to eq(2) |
There was a problem hiding this comment.
Here you are just testing the counts - please also check that the cloud subnets and security groups are actually the correct ones, i.e. g1, s1 and s2 below.
There was a problem hiding this comment.
Nice one, thanks.
spec/spec_helper.rb
Outdated
| @@ -9,5 +9,11 @@ | |||
| config.cassette_library_dir = File.join(ManageIQ::Providers::Nuage::Engine.root, 'spec/vcr_cassettes') | |||
| end | |||
|
|
|||
| ALL_REFRESH_SETTINGS = [ | |||
There was a problem hiding this comment.
Perhaps move this into refresher_spec? I don't think helper is the correct place for this.
There was a problem hiding this comment.
Done. I was in doubt where to put this in first place, so I'm happy you've pointed it out.
miha-plesko
force-pushed
the
tests-for-legacy-refresher
branch
from
d85f4d7 to
f96bc57
Compare
There was a problem hiding this comment.
Thanks for the review @gberginc , I've applied all your suggestions and repushed.
| :orchestration_stack_id => nil, | ||
| :type => "ManageIQ::Providers::Nuage::NetworkManager::NetworkGroup" | ||
| ) | ||
| expect(g2.cloud_subnets.count).to eq(2) |
There was a problem hiding this comment.
Nice one, thanks.
spec/spec_helper.rb
Outdated
| @@ -9,5 +9,11 @@ | |||
| config.cassette_library_dir = File.join(ManageIQ::Providers::Nuage::Engine.root, 'spec/vcr_cassettes') | |||
| end | |||
|
|
|||
| ALL_REFRESH_SETTINGS = [ | |||
There was a problem hiding this comment.
Done. I was in doubt where to put this in first place, so I'm happy you've pointed it out.
| let(:network_group_ref1) { "713d0ba0-dea8-44b4-8ac7-6cab9dc321a7" } | ||
| let(:network_group_ref2) { "e0819464-e7fc-4a37-b29a-e72da7b5956c" } | ||
| let(:security_group_ref) { "02e072ef-ca95-4164-856d-3ff177b9c13c" } | ||
| let(:cloud_subnet_ref1) { "d60d316a-c1ac-4412-813c-9652bdbc4e41" } |
| :ipv6_address_mode => nil, | ||
| :type => "ManageIQ::Providers::Nuage::NetworkManager::CloudSubnet", | ||
| :network_router_id => nil, | ||
| :network_group_id => NetworkGroup.find_by(:ems_ref => network_group_ref2).id, |
There was a problem hiding this comment.
Can you use @data_index.fetch_path here as well?
There was a problem hiding this comment.
I don't think I have access to refresh parser internals here in the spec. Also I think the spec should really check database status after the refresh, not the parser internals.
This commit is a preparation to be able to support graph inventory refresh with the next commit. Basically we provide unit tests for refresh parser so that we'll be able to assure that the new graph inventory refresh works identically to the current (legacy) one. Writing tests we've also discovered that there is a bug in current parser - the network groups were not associated correctly with the security groups. Fixed. God bless the unit tests! :D Signed-off-by: Miha Pleško <[email protected]>
miha-plesko
force-pushed
the
tests-for-legacy-refresher
branch
from
f96bc57 to
3c67961
Compare
There was a problem hiding this comment.
Thanks @gberginc, done. Please see my answer regarding @data_index.
| let(:network_group_ref1) { "713d0ba0-dea8-44b4-8ac7-6cab9dc321a7" } | ||
| let(:network_group_ref2) { "e0819464-e7fc-4a37-b29a-e72da7b5956c" } | ||
| let(:security_group_ref) { "02e072ef-ca95-4164-856d-3ff177b9c13c" } | ||
| let(:cloud_subnet_ref1) { "d60d316a-c1ac-4412-813c-9652bdbc4e41" } |
| :ipv6_address_mode => nil, | ||
| :type => "ManageIQ::Providers::Nuage::NetworkManager::CloudSubnet", | ||
| :network_router_id => nil, | ||
| :network_group_id => NetworkGroup.find_by(:ems_ref => network_group_ref2).id, |
There was a problem hiding this comment.
I don't think I have access to refresh parser internals here in the spec. Also I think the spec should really check database status after the refresh, not the parser internals.
|
Checked commit miha-plesko@3c67961 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
There was a problem hiding this comment.
My bad re data_index, thanks for pointing it out!
This change LGTM.
@juliancheal can you also have a look? Please note that this does not require Qpid because we are only trying to bring the new graph refresh into the provider.
|
@juliancheal I'm confident that this PR could get merged, especially as it does nothing special but adds unit tests. But if you have any suggestions on how to improve tests, I'd be glad to listen! |
|
LGTM 👍 |
This commit is a preparation to be able to support graph inventory refresh with the next commit. Basically we provide unit tests for refresh parser so that we'll be able to assure that the new graph inventory refresh works identically to the current (legacy) one.
Writing tests we've also discovered that there is a bug in current parser - the network groups were not associated correctly with the security groups. Fixed. God bless the unit tests! :D
@miq-bot add_label enhancement
@miq-bot assign @juliancheal
/cc @gberginc