----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10828/#review19846 -----------------------------------------------------------
test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40950> Also - please separate the suite into the following files. In this patch we can do: test_resource_limits (regular CPU tests/Max CPU tests) test_project_limits (resource limit tests for projects) test_domain_subdomain_limits (resource limit tests related to domain-admin, sub-domains etc) Having these in separate modules also helps the test runner drive the suites in parallel. test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40928> Move this dictionary to utils? and rename to utils.get_resource_type()? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40929> `delete` is not handled? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40930> There should be an API named findHostsForMigration. This was recently added to admin roles so they can identify the hosts suitable for migration. test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40931> The service_offering however has only 2vCPUs in the dictionary. Can we just test for multicore? Any 2/3/4 multicore setting should work? test_deploy_vm_with_multiplecores test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40935> Should these utility methods be included in every test class? Can we not include them in a single class and import as necessary? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40943> method name doesn't reflect intent. you are doing a checkResourceCount but doing an updateCount within? Was this intended? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40938> Can you add the "simulator" tag? This will allow us to run it against the simulator quickly. VM operations take longer on hardware. test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40936> service_offering has only 2 cores test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40939> There's some confusion I have here: 1. you are using the apiClient of the admin role and the limits apply on the account (domadmin, chlilddomadmin) 2. use the api client of the account you created instead to establish the limits? createUserApiClient from marvin's library can be used here. test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40937> can we name either camel_case or camelCase. Let's not mix both? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40940> Can you also put in a docstring @configuration with the value the configuration setting is expected to take in this test? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40941> Can this be used to udpdate resoruce counts of 1-8? If not put in an assert that rtype in [9, 10, 11, 12, 13, 14] only test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40942> Not sure the comment reflects the test being done. Seems to be the same as previous tests? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40944> How are these tests different from the ones in the previous class? TestCPULimitsUpdateResources .test_02_deploy_multiple_vm_with_5_cpus vs TestDomainCPULimits.test_02_deploy_multiple_vm_with_5_cpus? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40945> Can we have createDefaultTestAccounts([domadmin, admin, childadmin]) , updateLimits(accountname, domainid) and updateCounts(accountname, domainid) as methods instead of the ones used in all the classes here? test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40946> This can be broken down further. There are too many tests within this single test. 1. two domains sharing a single pool of cores 2. a heirarchy of root-parent-child sharing the same pool of cores 3. In each - deploying the VM, deletion of account affecting the resources test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40948> setupProjectAccounts test/integration/component/test_cpu_limits.py <https://reviews.apache.org/r/10828/#comment40949> Assign is not a regular vm lifecycle operation so please separate this test from the rest. Also - should the test be named test_(role=account/domain/project)_counts_vmlifecycle? It's the effect of vmlifecycle being tested on the resource counts. The name you've given is a bit generic. - Prasanna Santhanam On April 29, 2013, 2:29 a.m., Chirag Jog wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10828/ > ----------------------------------------------------------- > > (Updated April 29, 2013, 2:29 a.m.) > > > Review request for cloudstack, Prasanna Santhanam and Girish Shilamkar. > > > Description > ------- > > Here is the detailed test plan: > https://issues.apache.org/jira/secure/attachment/12571551/LimitResourcesTestPlan1.xlsx > > > This addresses bug https://issues.apache.org/jira/browse/CLOUDSTACK-1466. > > > Diffs > ----- > > test/integration/component/test_cpu_limits.py PRE-CREATION > > Diff: https://reviews.apache.org/r/10828/diff/ > > > Testing > ------- > > > Thanks, > > Chirag Jog > >
