cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rafaelweingartner <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-9298: Improve performance of r...
Date Wed, 02 Mar 2016 22:09:18 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message