[1LP][RFR] modify snapshot testing #5126

apagac · 2017-08-09T13:27:46Z

This modifies some core components used in snapshot testing. Features:

testing VM gets always deployed with a different name [1]
able to choose a template for testing VM [2]
able to choose to create a snapshot with/without memory

[1] This is needed due to a known (?) issue with pytest scopes. Solves the intermittent error that sometimes appears when destroying and then again deploying a VM with the same name.

[2] Some of the snapshot tests need a testing VM that has network configured, sshd running and automation can connect to it. This is not possible when the VM is deployed from the smallest template. On the other hand, most of the snapshot tests do not need bigger template, so it makes no sense to run them all with default template. Thus, we can now choose what template to use in each test.

quarckster · 2017-08-13T08:02:05Z

cfme/tests/infrastructure/test_snapshot.py

+            vm.create_on_provider(find_in_cfme=True, allow_skip="default")
+            request.addfinalizer(vm.delete_from_provider)
+        return vm
+    return _test_vm


REQUIRED
I would make two separate fixtures: small_test_vm and full_test_vm:

def provision_vm(request, name, provider, small_template=True): if small_template: vm = VM.factory(name, provider, template_name=provider.data['small_template']) else: vm = VM.factory(name, provider, template_name=provider.data['full_template']['name']) if not provider.mgmt.does_vm_exist(name): vm.create_on_provider(find_in_cfme=True, allow_skip="default") request.addfinalizer(vm.delete_from_provider) return vm @pytest.fixture(scope="module") def small_test_vm(request, setup_provider_modscope, provider, vm_name): return provision_vm(request, vm_name(), provider) @pytest.fixture(scope="module") def full_test_vm(request, setup_provider_modscope, provider, vm_name): return provision_vm(request, vm_name(), provider, small_template=False)

apagac · 2017-08-15T08:50:04Z

cfme/tests/infrastructure/test_snapshot.py

    """Tests snapshot crud

    Metadata:
        test_flag: snapshot, provision
    """
+    test_vm = small_test_vm


I can refactor if requested.

Would be nice...

Although I personally think this is a) faster and b) nicer, I did expect that I would have to refactor :) On it.

quarckster · 2017-08-17T09:20:49Z

cfme/tests/infrastructure/test_snapshot.py

@@ -31,7 +31,9 @@

 @pytest.fixture(scope="module")
 def vm_name():
-    return "test_snpsht_" + fauxfactory.gen_alphanumeric()
+    def _vm_name():
+        return "test_snpsht_" + fauxfactory.gen_alphanumeric()


REQUIRED
Please use format

psav

Some changes

psav · 2017-08-17T10:47:41Z

cfme/tests/infrastructure/test_snapshot.py

@@ -31,7 +31,9 @@

 @pytest.fixture(scope="module")
 def vm_name():
-    return "test_snpsht_" + fauxfactory.gen_alphanumeric()


REQUIRED: If I understand correctly, you just need to change the scope of the fixture to function to get a new vm name every time right?

I do need to change the scope, yes. I did try, it didn't work. What you see here is just a workaround, the underlying problem is a bit different.

There's a PR with new snapshot test #5003 . The test is parametrized and it's using module-scoped test_vm. Either I'm not getting the scopes at all, or it doesn't work, because instead of creating one VM for the test with all it's parametrization, it creates a new VM each time the test is run with different parameter. This means that the VM gets created, the test is run once with one parameter, then the VM is destroyed and immediately created again, the test is run the second time, and so on. Because of this, there were issues with multiple VMs in CFME with the same name, some of which were orphaned, and the automation was choosing the already destroyed one.

This of course isn't enough trouble, while running this I was hitting and issue with virtualcenter.delete_vm() RedHatQE/wrapanapi#154 . Can't reproduce that locally, it does happen with PRT. Can't run PRT, because I need this PR to be merged first.

My solution is what you are looking at now. I turned my back on scopes, changed fixtures so that the VMs have always different name, problem went away.

Please use the existing utils.generators.random_vm_name() for this

https://github.com/ManageIQ/integration_tests/blob/master/utils/generators.py#L6

There's no need for this to be a fixture, provision_vm should be the function scoped fixture that just uses this generator to specify a unique name.

psav · 2017-08-17T10:49:46Z

cfme/tests/infrastructure/test_snapshot.py

-    vm = VM.factory(vm_name, provider, template_name=provider.data['full_template']['name'])
+def provision_vm(provider, vm_name, request, small_template=True):
+    if small_template:
+        vm = VM.factory(vm_name, provider, template_name=provider.data['small_template'])


small_template doesn't follow the same format and have a name attribute? That seems poor. (not blaming you for this). Would be nice to fix this up so it looks like

template_source = 'small_template' if small_template else 'full_template': vm = VM.factory(vm_name, provider, template_name=provider.data[template_source]['name'])

This requires change in the structure of yaml file if I understand your request correctly. Probably affecting a lot of stuff. It is not my ambition to change lot of stuff in this PR, I just need to change bits of snapshot testing.

@psav @apagac There is inconsistency in the template naming throughout providers in cfme_data.yaml, which has been recorded in an issue, and that I have an open card to address. I'll be adding a 'templates' section to each provider, with consistent structure for small_template, full_template arrays that define name and creds for each template.

At that time I'll be auditing tests to update their access of the templates in the yaml, and can collapse this to a consistent access path at that time.

#5071

psav · 2017-08-17T10:51:10Z

cfme/tests/infrastructure/test_snapshot.py

        )
    return new_snapshot


 @pytest.mark.uncollectif(lambda provider:
    not provider.one_of(RHEVMProvider, VMwareProvider) or
    (provider.one_of(RHEVMProvider) and provider.version < 4))
-def test_snapshot_crud(test_vm, provider):
+def test_snapshot_crud(small_test_vm, provider):


This obviously won't get a new vm name each time, I assume this is OK?

Yep, this works just fine.

psav · 2017-08-17T10:51:19Z

cfme/tests/infrastructure/test_snapshot.py

    """Tests snapshot crud

    Metadata:
        test_flag: snapshot, provision
    """
+    test_vm = small_test_vm


Would be nice...

psav · 2017-08-17T10:51:44Z

cfme/tests/infrastructure/test_snapshot.py

        'username': credentials[provider.data['full_template']['creds']]['username'],
-        'password': credentials[provider.data['full_template']['creds']]['password']


Any reason for the reorder?

None whatsoever. I thought dictionaries are unordered.

psav

@mshriver has some replies to comments that need addressing

mshriver · 2017-08-22T12:54:16Z

cfme/tests/infrastructure/test_snapshot.py

-    """Fixture to provision appliance to the provider being tested if necessary"""
-    vm = VM.factory(vm_name, provider, template_name=provider.data['full_template']['name'])
+def provision_vm(provider, request, small_template=True):
+    vm_name = random_vm_name(context="snapshot")


mshriver · 2017-08-22T13:51:50Z

@psav My concerns have been addressed and latest PRT looks good, leaving to you to merge since you've got outstanding 'Requested changes' review.

apagac mentioned this pull request Aug 9, 2017

[1LP][RFR] test vm state after revert snapshot #5003

Merged

dajoRH added lint-ok WIP labels Aug 9, 2017

apagac changed the title ~~[WIP] modify snapshot testing~~ [WIPTEST] modify snapshot testing Aug 10, 2017

dajoRH added WIP-testing and removed WIP labels Aug 10, 2017

apagac force-pushed the modify_snapshot_testing branch from 7130124 to 5db2391 Compare August 10, 2017 12:59

apagac changed the title ~~[WIPTEST] modify snapshot testing~~ [RFR] modify snapshot testing Aug 10, 2017

dajoRH removed the WIP-testing label Aug 10, 2017

quarckster suggested changes Aug 13, 2017

View reviewed changes

quarckster changed the title ~~[RFR] modify snapshot testing~~ [WIP] modify snapshot testing Aug 13, 2017

dajoRH added the WIP label Aug 13, 2017

apagac force-pushed the modify_snapshot_testing branch from 5db2391 to b8f9ba0 Compare August 15, 2017 08:45

apagac commented Aug 15, 2017

View reviewed changes

apagac changed the title ~~[WIP] modify snapshot testing~~ [WIPTEST] modify snapshot testing Aug 15, 2017

dajoRH added WIP-testing and removed WIP labels Aug 15, 2017

apagac changed the title ~~[WIPTEST] modify snapshot testing~~ [RFR] modify snapshot testing Aug 15, 2017

dajoRH removed the WIP-testing label Aug 15, 2017

apagac mentioned this pull request Aug 16, 2017

[1LP][RFR] test snapshot operations on powered off vm #5159

Merged

quarckster approved these changes Aug 17, 2017

View reviewed changes

quarckster changed the title ~~[RFR] modify snapshot testing~~ [1LP][RFR] modify snapshot testing Aug 17, 2017

quarckster added the enhancement label Aug 17, 2017

apagac force-pushed the modify_snapshot_testing branch from b8f9ba0 to 787e7e1 Compare August 17, 2017 10:00

psav suggested changes Aug 17, 2017

View reviewed changes

psav changed the title ~~[1LP][RFR] modify snapshot testing~~ [1LP][WIP] modify snapshot testing Aug 17, 2017

dajoRH added the WIP label Aug 17, 2017

apagac force-pushed the modify_snapshot_testing branch from 787e7e1 to d5e7c3a Compare August 17, 2017 12:35

apagac changed the title ~~[1LP][WIP] modify snapshot testing~~ [1LP][WIPTEST] modify snapshot testing Aug 17, 2017

dajoRH removed the WIP label Aug 17, 2017

dajoRH added the WIP-testing label Aug 17, 2017

apagac changed the title ~~[1LP][WIPTEST] modify snapshot testing~~ [1LP][RFR] modify snapshot testing Aug 18, 2017

dajoRH removed the WIP-testing label Aug 18, 2017

psav suggested changes Aug 22, 2017

View reviewed changes

psav changed the title ~~[1LP][RFR] modify snapshot testing~~ [1LP][WIP] modify snapshot testing Aug 22, 2017

jkrocil assigned jkrocil and unassigned jkrocil Aug 22, 2017

dajoRH added the WIP label Aug 22, 2017

modify snapshot testing

e5d319b

apagac force-pushed the modify_snapshot_testing branch from d5e7c3a to e5d319b Compare August 22, 2017 12:18

apagac changed the title ~~[1LP][WIP] modify snapshot testing~~ [1LP][WIPTEST] modify snapshot testing Aug 22, 2017

dajoRH added WIP-testing and removed WIP labels Aug 22, 2017

mshriver reviewed Aug 22, 2017

View reviewed changes

apagac changed the title ~~[1LP][WIPTEST] modify snapshot testing~~ [1LP][RFR] modify snapshot testing Aug 22, 2017

dajoRH removed the WIP-testing label Aug 22, 2017

mshriver approved these changes Aug 22, 2017

View reviewed changes

mfalesni approved these changes Aug 23, 2017

View reviewed changes

mshriver merged commit 8829f87 into ManageIQ:master Aug 23, 2017

apagac deleted the modify_snapshot_testing branch September 13, 2017 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1LP][RFR] modify snapshot testing #5126

[1LP][RFR] modify snapshot testing #5126

apagac commented Aug 9, 2017

quarckster Aug 13, 2017

apagac Aug 15, 2017

psav Aug 17, 2017

apagac Aug 17, 2017

quarckster Aug 17, 2017

psav left a comment

psav Aug 17, 2017

apagac Aug 17, 2017

mshriver Aug 18, 2017 •

edited

Loading

psav Aug 17, 2017

apagac Aug 17, 2017

mshriver Aug 18, 2017 •

edited

Loading

psav Aug 17, 2017

apagac Aug 17, 2017

psav Aug 17, 2017

psav Aug 17, 2017

apagac Aug 17, 2017

psav left a comment

mshriver Aug 22, 2017

mshriver commented Aug 22, 2017

		'username': credentials[provider.data['full_template']['creds']]['username'],
		'password': credentials[provider.data['full_template']['creds']]['password']

[1LP][RFR] modify snapshot testing #5126

[1LP][RFR] modify snapshot testing #5126

Conversation

apagac commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshriver Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshriver Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshriver commented Aug 22, 2017

mshriver Aug 18, 2017 •

edited

Loading

mshriver Aug 18, 2017 •

edited

Loading