Removed unnecessary code from getGuestOsType in CitrixResourceBase

[cristofolini]

Considering that all mapping of Guest OS Names to their respective hypervisor compatible types is made thorugh accessing a database, we've decided to remove a bit of code in the XcpOssResource class which was doing that same thing for 2 different OS's (both of which ARE in the database). That has led us to a bunch of unused parameters in the getGuestOsType method from its superclass, which we've also decided to remove. Test cases were added for four different possibilities for the platformEmulator String: one for a null String, one for a blank String, one for an empy String and one for a random case with a valid String.

[rafaelweingartner]

@cristofolini could you please remove the @Local annotation from CitrixResourceBase ?
That annotation is not needed there.

[cristofolini]

@rafaelweingartner Done.

[bvbharatk]

ACS CI BVT Run

Sumarry:
Build Number 148
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=0
Skipped=4

Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso

Passed test suits:
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering

[rafaelweingartner]

@cristofolini, I can give an LGTM for this PR.
Thanks for the tests and the removal of unused code.

[rafaelweingartner]

@swill another one that seems to be ready to be merged.
What do you think?

[swill]

@rafaelweingartner yes, I think this is ready too.

I am trying to get two LGTM for every PR, so can someone else from the dev list give me a review?

[rafaelweingartner]

I agree with you

[DaanHoogland]

Do we need the tests in all child-test classes?

otherwise LGTM

[rafaelweingartner]

@DaanHoogland that is a good question. The point here is that it is an abstract class being tested. So, instead of removing the abstract from that class, we tested the method in each of its implementations.

[DaanHoogland]

@rafaelweingartner how about creating a test-child instead of testing in all child-tests ;)

[rafaelweingartner]

@DaanHoogland, I did not understand what you meant with test-child.

We could do the test a little bit different if we had a newer version of Mockito using the spy method. But, with the one we are using, we did not find much ways to write tests for that class, given that it is an abstract class.

Could you elaborate a little bit more your idea?

[DaanHoogland]

Well @rafaelweingartner you can make a test class for each of the child classes of the abstract class, as done now or you can make a CitrixResourceTest-class child of the abstract class and test with that one. You could even make it an anonymous class in the test. I am not sure but maybe you can even mock/spy an abstract class and have the testable method be called without any child. Does that explain?

[rafaelweingartner]

Sure it does.

And yes we can create a spy from an abstract class. The problem is that with the current version of Mockito the spy method only works for Objects and not for classes; we would need to create an anonymous class. Using the mock method would no work.

I also thought about using an anonymous class, but that seemed not elegant to me. However, if you find it interesting, we could do that way (with an anonymous class). Then, we would not even need to work with mocks.

[DaanHoogland]

Well, I am not interested in elegant at this stage, just in reducing the code base if it make sense. The simplest solution is an named test resource class as child of CitrixResourceBase and test on that one.

[rafaelweingartner]

Got your point.
@cristofolini is a little without time now, so I will do that.

[rafaelweingartner]

@DaanHoogland Done.
What do you think now?

[DaanHoogland]

@rafaelweingartner a lot less code (not in the diff but in the result) 👍

[rafaelweingartner]

I agree with you.
We did not use this solution at first because of this:

protected CitrixResourceBase citrixResourceBase = new CitrixResourceBase() {
  @Override
        protected String getPatchFilePath() {
            return null;
        }
    };

Probably I was trying to be too perfectionist; I did not like that. Anyways, I hope we bump Mockito version soon, so I can fix that.

[swill]

I am going to add this to my CI queue. Please reconfirm the LGTM's votes based on the updated code. Thanks...

[DaanHoogland]

LGTM

[swill]

Can I get the commits squashed and repushed with push -f. Thanks...

[DaanHoogland]

Will, I don't like manic squashing, why not preserve history.

[swill]

Some history is fine, but we don't need the whole history for every change in master.

I don't think this adds much by adding it to the master commit tree: c7b9739

Also, I need jenkins to run again, so I was killing two birds with one stone... :)

[rafaelweingartner]

@DaanHoogland, I agree with Will. The last commit should be squashed. I will do that.
@swill I would not like to give an LGTM here, because @cristofolini is my colleague and I have helped him with the code. Maybe someone else could review the code?

[rafaelweingartner]

@DaanHoogland, @swill commits squashed.
I preserved the @cristofolini commit, though.

[swill]

I need one more LGTM code review of this one, otherwise it is looking to be in good order...

[rafaelweingartner]

In this situation, I do not know if mine would count, giving that I also have helped with some code changes.

[swill]

@rafaelweingartner I understand you feel like you have a conflict of interests, but at the same time, you are also very familiar with the code so are in a good position to review it. If you trust yourself to be critical of the code in your review, I trust your review. :P

[rafaelweingartner]

@swill thanks for the support.
I have already been very critical with this PR even before it was opened. So, the way it is now, it is good to be merged. There is only one thing I did not like at "CitrixResourceBaseTest .java", but as I discussed with Daan, we will be able to fix that once we use a newer version of Mockito.

I sure can give an LGTM to it.

[swill]

Ok perfect, thanks @rafaelweingartner. I will get this merged...