[GitHub] cloudstack issue #1605: CLOUDSTACK-9428: Fix for CLOUDSTACK-9211 - Improve p...

[rafaelweingartner]

Github user rafaelweingartner commented on the issue:

    https://github.com/apache/cloudstack/pull/1605
  
    @nv, I am sorry, my bad; last time I read the code, I overlooked the method 
com.cloud.hypervisor.vmware.mo.VirtualMachineMO.getAllDeviceList(). Having said 
that, how do you feel about extracting the try/catch block to 
“getAllDeviceList”, then this method would not need to throw a checked 
exception. It could re-throw a runtime exception such as the CloudRuntime.
    
    From my understanding the test method “testStartVm3dgpuEnabled” is used 
to test the method “configureVideoCard”, right? What about changing that 
method name to “testConfigureVideoCard”?
    
    Moreover, do not you think that the method “configureVideoCard” needs 
at least two test cases? One that is already written, and the other to test the 
condition in which the  call 
“vmSpec.getDetails().containsKey(VmDetailConstants.SVGA_VRAM_SIZE)” returns 
false, and then the method “setNewVRamSizeVmVideoCard” is never called.
    
    The same happens for the method “modifyVmVideoCardVRamSize”, I think it 
has another condition “videoCard.getVideoRamSizeInKB().longValue() == 
svgaVmramSize” to be tested.
    
    And finally, for the test method 
“testConfigureSpecVideoCardNewVRamSize”, what about using the 
“Mockito.InOrder” to assure that the methods are being called in the proper 
order. If someone changes that order of call in the future, that can affect the 
method behavior.
    
    I also have one small comment about the title of the PR and the commit 
message. “Improve performance” seems a little too vague for a title. The 
idea is very well explained on the PR, but the title does not reflect much. 
What about, “improve the performance of cloud-plugin-hypervisor-vmware” or 
something like that.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---