----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19039/#review37538 -----------------------------------------------------------
test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69098> Change this name of services to some other better name? I know this has been convention, but services name is little misnomer for test data. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69101> This can be moved to a util function? test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69103> Dont add fail\asserts in non test functions. Check the return value and assert in tests. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69104> Move this to a utility function i believe. Also, do we have a class in base.py for Router to verify state. If not update or use that. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69106> We have a utility function under utils.py "verifyElementInList", this can help remove two asserts in to one i believe, please check. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69108> The validation comment at step2 mentions about to verify that the guest vm ip is not changed. I believe we are just listing the vms, should that satisfy to verify ip as well, i mean is listing vms is equal to verify that ip is not changed? test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69109> Move the test case comments as doc strings rather as comments. This has been used, but moving to doc strings helps in extracting them automatically if required, example while logging bug automatically for a failure, doc strings for that test case be extracted automatically. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69110> Step1 is not validated it seems? test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69111> is 10.1.1.9 outside of 10.1.1.0/29? test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69112> Add them as doc strings. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69113> Correct spelling in steps. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69114> check order of steps 2,2 test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69096> Will it be possible to check the update state of router rather sleep? It will help other test cases as well. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69097> Add these to codes. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69128> Change this to one single assert? test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69129> There is no relation with outputof stop to destroy, what if stop failed? Instead put the stop code inside of destroy and verify there itself if it is successful and proceed further. Also, return code for destroy, so that we can check it in tests and fail accordingly. I know some of these methods may be existing earlier , but changing them going ahead will be useful test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69131> Wrap setup in exception and call tear down explicitly in case of exception so proper clean up will happen. Skip tests in case of exception in setup failures. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69132> Rename the services to better name say testData or some thing more comprehensible. Services is little misnomer test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69130> in case of exception in setup tear down wont be called. Call tear down explicitly by catching exceptions otherwise it will effect further tests. Also, in case of exceptions, dont run tests skip tests. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69126> If setup throws an error, tear down wont be called. So, account will still remain. The better would be to see if CreateVPC... function is successful and set a flag, this way all tests further will run based upon this flag otherwise all tests will run despite of setUp failure. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69133> This function seems to be used in many classes, so if possible move it as a util\library function for better usage. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69116> Will this be cleaned automatically? test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69124> Its basically asserting in setup. Please avoid that, setup should throw an error rather a failure. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69127> Is this ok to hardcode these values? instead put it in one place, this way, the values can be altered at one place and effect takes place at all rather changing all test cases in future. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69134> Verify the proper number of validation steps. test/integration/component/test_ip_reservation.py <https://reviews.apache.org/r/19039/#comment69135> Are all validation steps covered as part of tests? - Santhosh Edukulla On March 14, 2014, 4:44 p.m., Ashutosh Kelkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19039/ > ----------------------------------------------------------- > > (Updated March 14, 2014, 4:44 p.m.) > > > Review request for cloudstack, Santhosh Edukulla and SrikanteswaraRao Talluri. > > > Bugs: CLOUDSTACK-2266 > https://issues.apache.org/jira/browse/CLOUDSTACK-2266 > > > Repository: cloudstack-git > > > Description > ------- > > Adding first set of automation tests for IP reservation feature. > > > Diffs > ----- > > test/integration/component/test_ip_reservation.py 224212f > tools/marvin/marvin/config/config.cfg 356a291 > > Diff: https://reviews.apache.org/r/19039/diff/ > > > Testing > ------- > > Yes. Log below. > > test_RVR_network (test_ip_reservation.TestIpReservation) ... SKIP: Skip - WIP > test_ip_reservation_in_multiple_networks_same_account > (test_ip_reservation.TestIpReservation) ... ok > test_nat_rules_nat rule (test_ip_reservation.TestIpReservation) ... ok > test_nat_rules_static nat rule (test_ip_reservation.TestIpReservation) ... ok > test_update_cidr_multiple_vms_not_all_inclusive > (test_ip_reservation.TestIpReservation) ... ok > test_update_cidr_single_vm_not_inclusive > (test_ip_reservation.TestIpReservation) ... ok > test_user_defined_cidr (test_ip_reservation.TestIpReservation) ... ok > test_vm_create_after_reservation_LB-NS > (test_ip_reservation.TestIpReservation) ... SKIP: Skipping - this test > required netscaler configured in the network > test_vm_create_after_reservation_LB-VR > (test_ip_reservation.TestIpReservation) ... ok > test_vm_create_outside_cidr_after_reservation_LB-NS > (test_ip_reservation.TestIpReservation) ... SKIP: Skipping - this test > required netscaler configured in the network > test_vm_create_outside_cidr_after_reservation_LB-VR > (test_ip_reservation.TestIpReservation) ... ok > ---------------------------------------------------------------------- > Ran 11 tests in 3753.613s > > OK > > Two tests which require Netscaler configured are skipped due to netscaler was > not available on setup. > > > Thanks, > > Ashutosh Kelkar > >
