> 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 > >
