Re: Review Request 14451: Some test for NetUtils

[Amogh Vasekar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review28417
---


Reminder-
Hi,
The review has been pending for long. Please update the patch based on the 
review comments.
Thanks

- Amogh Vasekar


On Oct. 4, 2013, 11:12 a.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 4, 2013, 11:12 a.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Bugs: CLOUDSTACK-4807
> https://issues.apache.org/jira/browse/CLOUDSTACK-4807
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Darren Shepherd]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26684
---


It seems this patch includes a lot of line length based reformatting in the 
unit tests.  Can you narrow the patch down to just what is really needed to 
change.

- Darren Shepherd


On Oct. 4, 2013, 11:12 a.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 4, 2013, 11:12 a.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Bugs: CLOUDSTACK-4807
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Laszlo Hornyak]

> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/test/com/cloud/utils/net/NetUtilsTest.java, line 225
> > 
> >
> > What I am suggesting is that add a method something like
> > boolean isEqual(var1, var2) {
> > if var1 == var2 
> > return true
> > return false
> > }
> > 
> > Now you can add cases like
> > assertFalse(isEqual(15, NetUtils.mac2Long("00:00:00:00:00:ff")))
> > 
> > This is just one implementation as there is no assertNotEquals() method 
> > provided by Junit. You could certainly add better implementations. 
> > We generally use assertFalse for boolean methods.
> > You can have a look at testIsSameIpRange():
> >  // Check for 2 different CIDRs and different IP Range
> > assertFalse(NetUtils.isSameIpRange(cidrFirst, cidrThird));
> > 
> > // Check for Incorrect format of CIDR
> > assertFalse(NetUtils.isSameIpRange(cidrFirst, "10.3.6.5/50"));
> > 
> > Just a few such cases will suffice.
> >
> 
> Laszlo Hornyak wrote:
> Ok... what I do not quite understand is what the purpose of this negative 
> test is. There is already test for mac2Long("...") must be equal long 
> , so a negative test seems to be a test that proves "15 != 16 or any 
> other long value, 15 only equals to 15" :)
> 
> Saksham Srivastava wrote:
> Actually the reason I suggested to add the test case is because we found 
> an interesting scenario, 2 CIDRs 10.0.151.0/20 and 10.0.144.0/20 look 
> different but have the same IP range, so isSameIpRange() will return true, 
> however isNetworkAWithinNetworkB() will hint you an different thing.
> 
> I just gave you a basic example of implementing assertNotEquals(), the 
> patch looks good to me even without adding these changes.
> You already have added ip2LongBadIp() which tests for a incorrect format 
> of IP address :)

Ok, thanks for the info, I will look into it!


- Laszlo


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26660
---


On Oct. 4, 2013, 11:12 a.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 4, 2013, 11:12 a.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Bugs: CLOUDSTACK-4807
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Saksham Srivastava]

> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/test/com/cloud/utils/net/NetUtilsTest.java, line 225
> > 
> >
> > What I am suggesting is that add a method something like
> > boolean isEqual(var1, var2) {
> > if var1 == var2 
> > return true
> > return false
> > }
> > 
> > Now you can add cases like
> > assertFalse(isEqual(15, NetUtils.mac2Long("00:00:00:00:00:ff")))
> > 
> > This is just one implementation as there is no assertNotEquals() method 
> > provided by Junit. You could certainly add better implementations. 
> > We generally use assertFalse for boolean methods.
> > You can have a look at testIsSameIpRange():
> >  // Check for 2 different CIDRs and different IP Range
> > assertFalse(NetUtils.isSameIpRange(cidrFirst, cidrThird));
> > 
> > // Check for Incorrect format of CIDR
> > assertFalse(NetUtils.isSameIpRange(cidrFirst, "10.3.6.5/50"));
> > 
> > Just a few such cases will suffice.
> >
> 
> Laszlo Hornyak wrote:
> Ok... what I do not quite understand is what the purpose of this negative 
> test is. There is already test for mac2Long("...") must be equal long 
> , so a negative test seems to be a test that proves "15 != 16 or any 
> other long value, 15 only equals to 15" :)

Actually the reason I suggested to add the test case is because we found an 
interesting scenario, 2 CIDRs 10.0.151.0/20 and 10.0.144.0/20 look different 
but have the same IP range, so isSameIpRange() will return true, however 
isNetworkAWithinNetworkB() will hint you an different thing.

I just gave you a basic example of implementing assertNotEquals(), the patch 
looks good to me even without adding these changes.
You already have added ip2LongBadIp() which tests for a incorrect format of IP 
address :)


- Saksham


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26660
---


On Oct. 4, 2013, 11:12 a.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 4, 2013, 11:12 a.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Bugs: CLOUDSTACK-4807
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Laszlo Hornyak]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/
---

(Updated Oct. 4, 2013, 11:12 a.m.)


Review request for cloudstack.


Bugs: CLOUDSTACK-4807


Repository: cloudstack-git


Description
---

- tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
- isWindows static method removed - it was not used
- minor changes in the implementations of the tested methods


Diffs
-

  utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
  utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 

Diff: https://reviews.apache.org/r/14451/diff/


Testing
---

yes


Thanks,

Laszlo Hornyak

Re: Review Request 14451: Some test for NetUtils

[Laszlo Hornyak]

> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/src/com/cloud/utils/net/NetUtils.java, line 149
> > 
> >
> > I have created https://issues.apache.org/jira/browse/CLOUDSTACK-4807. 
> > You could assign the bug to yourself and resolve it when your patch is 
> > merged.
> > Just add the bug id in the bug section in the review board, also add 
> > the branch(master) in the review board, it helps people applying the patch.

Can't assign, I need some permission in jira, but the bug id is set in 
reviewboard. Thx!


> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/src/com/cloud/utils/net/NetUtils.java, line 366
> > 
> >
> > Thanks for the explanation.
> > Can you keep this variable with all other static variables at the 
> > beginning of the class and not at this place.

sure, I will send an update.


> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/test/com/cloud/utils/net/NetUtilsTest.java, line 225
> > 
> >
> > What I am suggesting is that add a method something like
> > boolean isEqual(var1, var2) {
> > if var1 == var2 
> > return true
> > return false
> > }
> > 
> > Now you can add cases like
> > assertFalse(isEqual(15, NetUtils.mac2Long("00:00:00:00:00:ff")))
> > 
> > This is just one implementation as there is no assertNotEquals() method 
> > provided by Junit. You could certainly add better implementations. 
> > We generally use assertFalse for boolean methods.
> > You can have a look at testIsSameIpRange():
> >  // Check for 2 different CIDRs and different IP Range
> > assertFalse(NetUtils.isSameIpRange(cidrFirst, cidrThird));
> > 
> > // Check for Incorrect format of CIDR
> > assertFalse(NetUtils.isSameIpRange(cidrFirst, "10.3.6.5/50"));
> > 
> > Just a few such cases will suffice.
> >

Ok... what I do not quite understand is what the purpose of this negative test 
is. There is already test for mac2Long("...") must be equal long , so a 
negative test seems to be a test that proves "15 != 16 or any other long value, 
15 only equals to 15" :)


- Laszlo


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26660
---


On Oct. 2, 2013, 9:19 p.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 2, 2013, 9:19 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Saksham Srivastava]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26660
---



utils/src/com/cloud/utils/net/NetUtils.java


I have created https://issues.apache.org/jira/browse/CLOUDSTACK-4807. You 
could assign the bug to yourself and resolve it when your patch is merged.
Just add the bug id in the bug section in the review board, also add the 
branch(master) in the review board, it helps people applying the patch.



utils/src/com/cloud/utils/net/NetUtils.java


Thanks for the explanation.
Can you keep this variable with all other static variables at the beginning 
of the class and not at this place.



utils/test/com/cloud/utils/net/NetUtilsTest.java


What I am suggesting is that add a method something like
boolean isEqual(var1, var2) {
if var1 == var2 
return true
return false
}

Now you can add cases like
assertFalse(isEqual(15, NetUtils.mac2Long("00:00:00:00:00:ff")))

This is just one implementation as there is no assertNotEquals() method 
provided by Junit. You could certainly add better implementations. 
We generally use assertFalse for boolean methods.
You can have a look at testIsSameIpRange():
 // Check for 2 different CIDRs and different IP Range
assertFalse(NetUtils.isSameIpRange(cidrFirst, cidrThird));

// Check for Incorrect format of CIDR
assertFalse(NetUtils.isSameIpRange(cidrFirst, "10.3.6.5/50"));

Just a few such cases will suffice.



- Saksham Srivastava


On Oct. 2, 2013, 9:19 p.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 2, 2013, 9:19 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Laszlo Hornyak]

> On Oct. 3, 2013, 10:10 a.m., Saksham Srivastava wrote:
> > utils/src/com/cloud/utils/net/NetUtils.java, line 366
> > 
> >
> > Any particular reason for making dot as private static ? Making it 
> > local to ip2Long could also solve the purpose as I don't see it being used 
> > anywhere else.
> > If u still want to make it as private static it is better to keep the 
> > variable with all other static variables at the beginning of the class.
> >

static: the point is that it should be created only once, avoiding creating the 
pattern again and again when it is just a local var Pattern is thread safe


> On Oct. 3, 2013, 10:10 a.m., Saksham Srivastava wrote:
> > utils/src/com/cloud/utils/net/NetUtils.java, line 149
> > 
> >
> > Can you mention against which branch the patch was created. Also create 
> > a bug/enhancement in Jira to track the issue.

this test is against master branch

create a jira ticket for what issue?


> On Oct. 3, 2013, 10:10 a.m., Saksham Srivastava wrote:
> > utils/test/com/cloud/utils/net/NetUtilsTest.java, line 225
> > 
> >
> > Can you add few negative test cases like (15, "00:00:00:00:00:ff") in 
> > each of these methods.

Could you add those tests? I have no idea for those tests right now but I will 
read your patch.


- Laszlo


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26645
---


On Oct. 2, 2013, 9:19 p.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 2, 2013, 9:19 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>

Re: Review Request 14451: Some test for NetUtils

[Saksham Srivastava]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/#review26645
---



utils/src/com/cloud/utils/net/NetUtils.java


Can you mention against which branch the patch was created. Also create a 
bug/enhancement in Jira to track the issue.



utils/src/com/cloud/utils/net/NetUtils.java


Any particular reason for making dot as private static ? Making it local to 
ip2Long could also solve the purpose as I don't see it being used anywhere else.
If u still want to make it as private static it is better to keep the 
variable with all other static variables at the beginning of the class.




utils/test/com/cloud/utils/net/NetUtilsTest.java


Can you add few negative test cases like (15, "00:00:00:00:00:ff") in each 
of these methods.


- Saksham Srivastava


On Oct. 2, 2013, 9:19 p.m., Laszlo Hornyak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14451/
> ---
> 
> (Updated Oct. 2, 2013, 9:19 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> - tests added for mac2Long, byte2Mac, long2Mac, ip2Long and long2Ip
> - isWindows static method removed - it was not used
> - minor changes in the implementations of the tested methods
> 
> 
> Diffs
> -
> 
>   utils/src/com/cloud/utils/net/NetUtils.java 1e72e22 
>   utils/test/com/cloud/utils/net/NetUtilsTest.java 3cfc98f 
> 
> Diff: https://reviews.apache.org/r/14451/diff/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> Thanks,
> 
> Laszlo Hornyak
> 
>