-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR] Automate test to verify key pair visibility in child tenants #9768
[1LP][RFR] Automate test to verify key pair visibility in child tenants #9768
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I have some small questions
cfme/cloud/keypairs.py
Outdated
@@ -184,6 +185,41 @@ def download_private_key(self): | |||
view.toolbar.configuration.item_select('Download private key') | |||
view.flash.assert_no_error() | |||
|
|||
def set_ownership(self, owner=None, group=None, click_cancel=False, click_reset=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional
def set_ownership(self, owner=None, group=None, click_cancel=False, click_reset=False): | |
def set_ownership(self, owner=None, group=None, cancel=False, reset=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required
Entity/collection methods need to be consistent with their kwargs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that common/vm has these kwargs - but its inconsistent with the rest of the framework kwargs for these methods.
cfme/tests/cloud/test_keypairs.py
Outdated
view = navigate_to(appliance.collections.cloud_keypairs, 'All') | ||
key_pair = view.entities.get_first_entity().data['name'] | ||
key_pair_obj = appliance.collections.cloud_keypairs.instantiate(key_pair) | ||
key_pair_obj.set_ownership("", new_tenant_admin.groups[0].description) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the empty string here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just setting group ownership and not user ownership, so updated the statement so that it's done the right way.
cfme/fixtures/multi_tenancy.py
Outdated
from cfme.utils.update import update | ||
|
||
|
||
def new_user(appliance, groups, name=None, credential=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q I would expect this to be a fixture, is there a reason it isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to be a fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have so many of these fixtures defined in multiple test modules, it would be great to consolidate them, as many of them repeat the same kwargs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be nice to consolidate the fixtures. I will work on it in a follow up PR.
cfme/cloud/keypairs.py
Outdated
@@ -184,6 +185,41 @@ def download_private_key(self): | |||
view.toolbar.configuration.item_select('Download private key') | |||
view.flash.assert_no_error() | |||
|
|||
def set_ownership(self, owner=None, group=None, click_cancel=False, click_reset=False): | |||
"""Set instance ownership |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keypair, not instance :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated :)
else: | ||
# save the form | ||
view.form.save_button.click() | ||
view = self.create_view(navigator.get_class(self, 'Details').VIEW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
cfme/cloud/keypairs.py
Outdated
@@ -184,6 +185,41 @@ def download_private_key(self): | |||
view.toolbar.configuration.item_select('Download private key') | |||
view.flash.assert_no_error() | |||
|
|||
def set_ownership(self, owner=None, group=None, click_cancel=False, click_reset=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like something that a mixin implementation would be appropriate for, like with the Tagging classes.
We have multiple entities that can have ownership set, and they use a common form and form data. We can define an ownership class that can be inherited by any entity classes that support ownership.
I'd call it out of scope for this PR, but would be a good enhancement so we don't continue to re-implement this method on multiple entities.
|
||
|
||
@pytest.fixture(scope='module') | ||
def child_tenant(appliance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving these out to a separate fixture file!
@pytest.fixture(scope='module') | ||
def tenant_role(appliance, request): | ||
role = appliance.collections.roles.instantiate(name='EvmRole-tenant_administrator') | ||
tenant_role = role.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
cfme/fixtures/multi_tenancy.py
Outdated
|
||
|
||
@pytest.fixture(scope='module') | ||
def new_tenant_admin(appliance, request, child_tenant, tenant_role): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a name like child_tenant_admin_user
so its more obvious that its producing a user entity. With the current name I was kind of expecting a tenant entity.
Docblocks for these fixtures would be very helpful :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfme/tests/cloud/test_keypairs.py
Outdated
|
||
@pytest.mark.meta(automates=[1741635, 1747179]) | ||
@test_requirements.multi_tenancy | ||
def test_tenantadmin_view_keypair(appliance, new_tenant_admin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a test function name that better indicates that its an ownership test - test_tenant_keypair_ownership
?
The assertion scope for the test case could be expanded, to do more assertion around the setting of ownership for a keypair (flash message asserts), as well as adding some parametrization to cover both a tenant that should not have visibility and one that does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the test name to test_keypair_visibility_in_tenants
since this is more of a keypair visibility test based on key pair ownership.
I will add parametrization to cover a tenant that doesn't have visibility in a follow up PR.
cfme/tests/cloud/test_keypairs.py
Outdated
@@ -29,7 +30,6 @@ def keypair(appliance, provider): | |||
|
|||
@pytest.mark.meta(blockers=[BZ(1718833, forced_streams=["5.10", "5.11"], | |||
unblock=lambda provider: provider.one_of(OpenStackProvider))]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This BZ is closed.
In [1]: BZ(1718833, forced_streams=["5.10", "5.11"]).blocks
.....
....
...
Found matching bug for 1718833 by release - #1718833
Out[1]: False
- Can we remove the blockers marker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, we should remove this @nachandr, and include it as automates
or docblock metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfme/tests/cloud/test_keypairs.py
Outdated
|
||
Steps: | ||
1. Copy the EvmRole_tenant_admin role to a new role (Since this role does not have the | ||
Auth Key Pairs feature enabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add steps under polarion data under tag - testSteps:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -1574,7 +1532,8 @@ def test_superadmin_tenant_admin_crud(appliance): | |||
|
|||
@pytest.mark.tier(2) | |||
@test_requirements.multi_tenancy | |||
def test_tenantadmin_group_crud(new_tenant_admin, tenant_role, child_tenant, request, appliance): | |||
def test_tenantadmin_group_crud(child_tenant_admin_user, tenant_role, child_tenant, request, | |||
appliance): | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please improve formatting.
@@ -1641,7 +1600,8 @@ def test_tenant_unique_catalog(appliance, request, catalog_obj): | |||
|
|||
@pytest.mark.ignore_stream("upstream") | |||
@test_requirements.multi_tenancy | |||
def test_tenantadmin_user_crud(new_tenant_admin, tenant_role, child_tenant, request, appliance): | |||
def test_tenantadmin_user_crud(child_tenant_admin_user, tenant_role, child_tenant, request, | |||
appliance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments, please address @ganeshhubale's comments as well. Thanks for your work on this @nachandr
cfme/tests/cloud/test_keypairs.py
Outdated
@@ -29,7 +30,6 @@ def keypair(appliance, provider): | |||
|
|||
@pytest.mark.meta(blockers=[BZ(1718833, forced_streams=["5.10", "5.11"], | |||
unblock=lambda provider: provider.one_of(OpenStackProvider))]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, we should remove this @nachandr, and include it as automates
or docblock metadata.
I detected some fixture changes in commit a8d6b60 The global fixture
The global fixture
The global fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks @nachandr!
…ts (ManageIQ#9768) * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to view key pair as tenant admin * Automate test to verify key pair visibility in child tenants * Automate test to verify key pair visibility in child tenants
Purpose or Intent
1.Automated test to verify key pair visibility in child tenants
2.Moved fixtures related to multi tenancy to a separate file
3.Added set_ownership method and SetOwnership view to cloud key pair page
PRT Run
{{ pytest: cfme/tests/cloud/test_keypairs.py::test_keypair_visibility_in_tenants cfme/tests/configure/test_access_control.py::test_tenantadmin_user_crud }}
PRT results:
510 - PASS
511- PASS