You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Laszlo Hornyak <la...@gmail.com> on 2013/10/02 23:19:52 UTC
Review Request 14451: Some test for NetUtils
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14451/
-----------------------------------------------------------
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
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Oct. 3, 2013, 10:10 a.m., Saksham Srivastava wrote:
> > utils/src/com/cloud/utils/net/NetUtils.java, line 366
> > <https://reviews.apache.org/r/14451/diff/1/?file=360597#file360597line366>
> >
> > 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
> > <https://reviews.apache.org/r/14451/diff/1/?file=360597#file360597line149>
> >
> > 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
> > <https://reviews.apache.org/r/14451/diff/1/?file=360598#file360598line225>
> >
> > 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
Posted by Saksham Srivastava <sa...@citrix.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/14451/#comment51907>
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
<https://reviews.apache.org/r/14451/#comment51905>
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
<https://reviews.apache.org/r/14451/#comment51906>
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
>
>
Re: Review Request 14451: Some test for NetUtils
Posted by Darren Shepherd <da...@gmail.com>.
-----------------------------------------------------------
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
Posted by Amogh Vasekar <am...@citrix.com>.
-----------------------------------------------------------
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
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
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
Posted by Saksham Srivastava <sa...@citrix.com>.
> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/test/com/cloud/utils/net/NetUtilsTest.java, line 225
> > <https://reviews.apache.org/r/14451/diff/1/?file=360598#file360598line225>
> >
> > 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 <value>, 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
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/src/com/cloud/utils/net/NetUtils.java, line 149
> > <https://reviews.apache.org/r/14451/diff/1/?file=360597#file360597line149>
> >
> > 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
> > <https://reviews.apache.org/r/14451/diff/1/?file=360597#file360597line366>
> >
> > 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
> > <https://reviews.apache.org/r/14451/diff/1/?file=360598#file360598line225>
> >
> > 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 <value>, 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
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Oct. 4, 2013, 6:43 a.m., Saksham Srivastava wrote:
> > utils/test/com/cloud/utils/net/NetUtilsTest.java, line 225
> > <https://reviews.apache.org/r/14451/diff/1/?file=360598#file360598line225>
> >
> > 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 <value>, 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
Posted by Saksham Srivastava <sa...@citrix.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/14451/#comment51942>
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
<https://reviews.apache.org/r/14451/#comment51944>
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
<https://reviews.apache.org/r/14451/#comment51947>
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
>
>