[GitHub] cloudstack pull request: CLOUDSTACK-9298: Improve performance of r...

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1425#issuecomment-191455410
  
    @nvazquez I have reviewed your code.
    Class “VolumeJoinDaoImpl” at line 46, why did you change the get Logger 
method to use “VolumeJoinDaoImplTest”?
    
    The method “updateTemplateTagInformation” is duplicated (I would say 
triplicated) at “VolumeJoinDaoImpl”, “UserVmJoinDaoImpl” and 
“TemplateJoinDaoImpl”. I know, you may argue that the method 
“updateTemplateTagInformation” has a conditional in 
“TemplateJoinDaoImpl” and “VolumeJoinDaoImpl”; still, that conditional 
is executed with some additional checks in “UserVmJoinDaoImpl”. The point 
here is that you should let that conditional out of the method scope, and then 
extract the method to a place that can be reused in all of those classes. All 
of them (VolumeJoinDaoImpl, UserVmJoinDaoImpl, and TemplateJoinDaoImpl) extend 
“GenericDaoBase” class. You can easily create a new class that extend 
“GenericDaoBase”, with that “updateTemplateTagInformation” and make all 
of those classes extend this new one. Please, keep that in mind in your next 
PRs.
    
    That same is happening for VolumeJoinVO, UserVmJoinVO, and TemplateJoinVO 
classes. All of them have attributes “tagAccountName”, “tagDomainUuid” 
and “tagDomainName” and their respective getters and setters. They all 
extend “BaseViewVO”, you can do that same trick I mentioned you above here.
    
    Now about the tests. First, when writing tests, you should ask yourself, 
“What am I going to test?”, “If this method changes the behavior would I 
like to catch it?”, “What is the behavior I want to monitor the behavior 
of?”. Those are the basic questions.
    I believe your tests, should test the method 
“updateTemplateTagInformation” that you wrote. So, what do you want to 
detect if that method stops setting a property? Do you want to detect if it 
sets a property in a wrong place? If you want that, your tests now are not 
adequate, if you do not want that, you should delete the tests classes.
    
    I personally believe that the method deserves tests, to check if it is 
working as expected. Meaning, that it sets the properties you want in the right 
place.
    
    Additionally, when writing tests try to code as little as possible. 
Variables “accountService” and “configDao” are not needed to test the 
“updateTemplateTagInformation”; keep in mind that is a unit test. To test 
“updateTemplateTagInformation” you only need a simple instance of 
“TemplateJoinDaoImpl” and a mock of 
“ApiDBUtils.newResourceTagResponse(vtag, false)”


---
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.
---