Re: Review Request 14451: Some test for NetUtils
--- 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
---
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
Re: Review Request 14451: Some test for NetUtils
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
--- 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
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
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
--- 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
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
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
