Manual assertions are not in play. These are not in the tests. Regular
assertEquals calls are made.
Investigating the hierarchy once again to give you a reference I found one
thing that is less then optimal with them. The tests are in testclasses not
generic to the tested resources: XcpOssResourcePathTest or
XenServer650ResourcePathTest
they should be in
XcpOssResourceTest or XenServer650ResourceTest
along with other tests for those classes.
Also more extensive tests should include runs of getPatchFiles() for the
classes, mocking the call to Script.findScript("", patch);. If this were
your objections I could understand. The tests as they are are quite trivial
but not completely senseless.
the hierarchy of the test targets is the best reference.
CitrixResourceBase
> XcpOssResource
> XcpServerResource
> XenServer56Resource
> XenServer56FP1Resource
> XenServer56SP2Resource
> XenServer600Resource
> XenServer610Resource
> XenServer620Resource
etc
but also
XenServer625ResourcePathTest it tests for a path that does not belong
intuitively obvious to the target of this class :
"scripts/vm/hypervisor/xenserver/xenserver62/patch".
What is the harm in testing for this?
On Wed, Aug 19, 2015 at 6:37 PM, Anshul Gangwar <anshul.gang...@citrix.com>
wrote:
>
> Please mention lines in tests which is justifying your statement "They
> prove that the fragile integrity of the class hierarchy that has been
> meddled with so often is still intact”
>
> I have no problem with solution. I have problem with tests.
>
> For reference see section Superficial Test Coverage @
> http://www.exubero.com/junit/antipatterns.html#Manual_Assertions .
>
>
>
> On 19-Aug-2015, at 7:59 PM, Daan Hoogland <daan.hoogl...@gmail.com> wrote:
>
> Anshul,
>
> I do not think a reference for the intricate problems we face with the
> hierarchy of the CitrixResourceBase and descendants is fair to ask. I think
> the burdon of proof is with you and not Rafael.
> The tests do not just prove that the assignment works as in you example.
> They prove that the fragile integrity of the class hierarchy that has been
> meddled with so often is still intact. Rafael and Lucas had a problem at
> hand and dealt with it according to circumstances. Your example to
> discredit this is over simplified and does not include the essence of what
> can go wrong with the way the xenserver resource hierarchy is set up. The
> solution choosen works, solves a problem and is a reference in it self. If
> you think it is bad practice, please provide with a reference as to why it
> is bad.
>
>
> On Wed, Aug 19, 2015 at 3:39 PM, Rafael Weingärtner <
> rafaelweingart...@gmail.com> wrote:
>
>> @anshul1886 I agree that we disagree.
>>
>> Folks I do not know what to do in this case, I will not looking for
>> references to support what I said and did. Whatever you guys decided I will
>> do (remove or let the test cases there).
>>
>> On Wed, Aug 19, 2015 at 9:47 AM, Anshul Gangwar <
>> anshul.gang...@citrix.com> wrote:
>>
>>> Let me summarise the tests
>>> class A {
>>> x(){
>>> return “d”; a constant
>>> }
>>> }
>>>
>>> test
>>>
>>> p=“d”
>>> Assert( A.x() = p)
>>>
>>> Which can be reduced to
>>>
>>> class A {
>>> q=“d";
>>> }
>>>
>>> Here q is replacement for x method as it is only returning a constant
>>>
>>> now test is
>>> p=“d"
>>> assert (p=q)
>>>
>>> To me this basically proves that java assignment works and nothing more
>>> than that.
>>>
>>> Here A can be any class. But in this context it is subclass of
>>> something.
>>>
>>> I can’t even figure out how super class coming into picture in tests
>>> which you are trying to say.
>>>
>>> Please explain in context of above example how it is testing more than
>>> java assignment.
>>>
>>> Regards,
>>> Anshul
>>>
>>> On 19-Aug-2015, at 5:16 pm, Anshul Gangwar <anshul.gang...@citrix.com>
>>> wrote:
>>>
>>> Can you point to any reference/blog which justifies writing tests for
>>> this kind of scenario?
>>>
>>> What I can infer from these tests is that that there are two scenarios
>>> 1) Method will not change
>>> In that case it doesn’t make sense to put test for never changing
>>> method.
>>> 2) Method will change
>>> In that case you have to change test to make it pass and then also it
>>> doesn’t make sense as you have to change test to make it actually pass.
>>>
>>> Regards,
>>> Anshul
>>>
>>>
>>> On 19-Aug-2015, at 4:18 pm, Rafael Weingärtner <
>>> rafaelweingart...@gmail.com<mailto:rafaelweingart...@gmail.com
>>> <rafaelweingart...@gmail.com>>> wrote:
>>>
>>> @anshul1886 I totally agree with you that tests are meant to test
>>> individual, and as you pointed the individual code that we want to test is
>>> “getPatchFilePath”. However, that method is abstract, and its
>>> “implementation” that is as simple as returning a constant, changes in few
>>> subclasses of CitrixResourceBase. I am not testing the constant per se; I
>>> am testing if each one of the implementation of that method is returning
>>> what I expect.
>>>
>>> In one hand, I agree with you that the method could have documentation.
>>> I just have not done that because I really do not know what that String
>>> that the method is returning is. On the other hand, documentation will not
>>> save us from future bugs, as a consequence of some change in those methods.
>>> Those tests can do that automatically.
>>>
>>> If that method had a conditional statement and it was coded into
>>> CitrixResourceBase would you think different? The point here is that object
>>> orientation removed those ifs.
>>>
>>> On Wed, Aug 19, 2015 at 7:26 AM, Daan Hoogland <daan.hoogl...@gmail.com<
>>> mailto:daan.hoogl...@gmail.com <daan.hoogl...@gmail.com>>> wrote:
>>> a unit can be defined at more then just the method level and in this case
>>> those paths have changed from under us in the past. I am not justifying
>>> testing any constant this way. I am justifying just this work.
>>>
>>> On Wed, Aug 19, 2015 at 9:03 AM, Anshul Gangwar <
>>> anshul.gang...@citrix.com<mailto:anshul.gang...@citrix.com
>>> <anshul.gang...@citrix.com>>>
>>> wrote:
>>>
>>> What I mean to say is that unit test are meant to test individual unit
>>> which here is getPatchFilePath and not meant to test hierarchy as you are
>>> pointing out here. By individual unit I mean it doesn’t matter for test
>>> that it is in class A or class B. This way you are kind of justifying
>>> that
>>> we should write test for any constant which you have defined has the
>>> value
>>> which you have given. Because constant under different classes can have
>>> different values.
>>>
>>> Can you point to any reference which justifies writing tests for this
>>> kind
>>> of scenario?
>>>
>>>
>>> Regards,
>>> Anshul
>>>
>>> On 19-Aug-2015, at 11:34 am, Daan Hoogland <daan.hoogl...@gmail.com<
>>> mailto:daan.hoogl...@gmail.com <daan.hoogl...@gmail.com>>>
>>> wrote:
>>>
>>>
>>> On Wed, Aug 19, 2015 at 5:56 AM, anshul1886 <g...@git.apache.org<
>>> mailto:g...@git.apache.org <g...@git.apache.org>>> wrote:
>>>
>>> If the purpose is to make sure that path is not modified by other
>>> developer then adding note/comment on top of that line makes more sense.
>>> Even adding note is kind of implicit as paths are kind of constants which
>>> any developer would think before changing. Tests are not meant for that
>>> purpose.
>>>
>>>
>>> Anshul, I hope I don't understand you when you say, 'tests are not meant
>>> for that pupose'. When the hierarchy is changed and this leads to the
>>> constants to be used in a different way in different classes, an error
>>> occurs due to this that will be caught by these tests. This is exactly
>>> what
>>> unit tests are for.
>>>
>>> class A has constant pth="/root".
>>> class B:A has constant pth="/root/bla".
>>> class C:B has no constant hence pth="/root/bla".
>>>
>>> now C is changed to C:A and its pth is there fore changed to "/root".
>>> This is uninteded and a mistake that will be caught by such tests.
>>>
>>>
>>>
>>> --
>>> Daan
>>>
>>>
>>>
>>>
>>>
>>> --
>>> Daan
>>>
>>>
>>>
>>> --
>>> Rafael Weingärtner
>>>
>>>
>>>
>>
>>
>> --
>> Rafael Weingärtner
>>
>
>
>
> --
> Daan
>
>
>
--
Daan
