----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11626/#review21967 -----------------------------------------------------------
Gaurav, I think this diff suffers from the same problems as the previous diff for memory limit tests. The patch is incomplete as it doesn't include changes to the library for memory limit objects - Resources, get_resouce_type etc. 1. Please split each class into a different module. This allows them to be processed in parallel as they have no dependency between each other. VPC related suites suffer the same problem. It takes over 3 hrs to run them because we've put too many tests into the same suite 2. You can organize the tests into test/integration/component/memory_limits/test_domain_limits.py|test_project_limits.py etc. The test discoverer will drill down to find the tests to be run 3. There are a lot of repeated use of small methods like checkUpdateResourceCount, createInstance etc. This is good for refactoring but it makes it harder for someone reading the test to quickly understand what each method is doing. Instead looking at VM.create, ResourceCount.update will make it easier to look in one place - the libraries. test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45284> The entity 'Resource' needs to be added to integration.lib.base test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45285> get_resource_type is missing in utils test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45286> remove the `pass` here test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45288> Do we need this method? I think keeping the VM creation step (VirtualMachine.create, asserts1, assert2) is easier than refactoring this into a test specific method for each test suite. test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45287> account reference is account.account test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45289> Same here, anyone reading the test should be able to make sense of Network.list followed by assert faster than having to hunt down what get_network does each time. test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45290> what if networks is an empty list? [] test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45291> account dereferences as account.name only. test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45292> account dereferences as account.name only. test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45293> This calls listHosts and not findHostSuitableForMigration I think? test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45295> Does it make sense to break this test down into smaller steps? class Test_LimitsInVmLifeCycle(): test_migrate_limitcheck test_stop_limitcheck test_reboot_limitcheck Putting in so many life cycle operations into the same test is complicating it. test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45294> When you use an rtype can you also add a comment? checkUpdatedResourceCount(account, rtype=9)#RAM test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45296> Reuse service offerings? test/integration/component/test_memory_limits.py <https://reviews.apache.org/r/11626/#comment45297> I'm confused - are we checking the RAM usage or are we updating the resource count for memory usage? The comment in the test confuses me. - Prasanna Santhanam On June 4, 2013, 11:39 a.m., Gaurav Aradhye wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11626/ > ----------------------------------------------------------- > > (Updated June 4, 2013, 11:39 a.m.) > > > Review request for cloudstack and Prasanna Santhanam. > > > Description > ------- > > Adding resource limit tests related to Memory. Changes suggested in CPU > resource limit tests which are applicable here are incorporated too. > > Updated test plan is available here: > https://cwiki.apache.org/confluence/download/attachments/30757590/LimitResourcesTestPlanUpdate5.xlsx?version=1&modificationDate=1366952352000 > > > Diffs > ----- > > test/integration/component/test_memory_limits.py PRE-CREATION > > Diff: https://reviews.apache.org/r/11626/diff/ > > > Testing > ------- > > > Thanks, > > Gaurav Aradhye > >
