[6.3 Feature] Canned Role Org Admin API stubs automated #5592

jyejare · 2017-11-19T18:13:16Z

20 much needed Canned Role and Org Admin feature stubs are automated from API end.

jyejare · 2017-11-19T18:22:22Z

This PR is dependent on SatelliteQE/nailgun#458

codecov · 2017-11-19T18:22:44Z

Codecov Report

Merging #5592 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5592      +/-   ##
==========================================
- Coverage    61.3%   61.25%   -0.06%     
==========================================
  Files          33       33              
  Lines        3672     3680       +8     
==========================================
+ Hits         2251     2254       +3     
- Misses       1421     1426       +5

Impacted Files	Coverage Δ
robottelo/datafactory.py	`83.75% <0%> (-5.3%)`	⬇️
robottelo/constants.py	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f6ac14...adb9828. Read the comment docs.

jyejare · 2017-11-19T18:24:51Z

The test Run result:

# py.test -m 'not stubbed' tests/foreman/api/test_role.py::CannedRoleTestCases
=============================================================================================== test session starts ===============================================================================================
platform linux2 -- Python 2.7.11, pytest-3.1.3, py-1.4.34, pluggy-0.4.0
rootdir: /home/jitendrayejare2/Desktop/RedHat/RoboTelloNew/robottelo, inifile:
plugins: xdist-1.15.0, services-1.2.1, mock-1.6.2, cov-2.4.0
collected 43 items 
2017-11-19 23:22:03 - conftest - DEBUG - Deselect of WONTFIX BZs is disabled in settings


tests/foreman/api/test_role.py ....................

=============================================================================================== 23 tests deselected ===============================================================================================
============================================================================== 20 passed, 23 deselected in 274.80 seconds ===============================================================================

jyejare · 2017-11-21T11:00:23Z

@SatelliteQE/robottelo-automation , Please help this PR to get merge having 20 tests automated of Canned Role feature. This will help us in release criteria as 20 automated tests are being added.

abalakh

Pending comments. Please check all the tests as they're basically applicable to all of them, not only the places i've pointed to.

abalakh · 2017-11-21T11:07:40Z

tests/foreman/api/test_role.py

@@ -100,7 +100,48 @@ def test_positive_update(self):
 class CannedRoleTestCases(APITestCase):
    """Implements Canned Roles tests from API"""

-    @stubbed()
+    def create_org_admin(self, name=None, orgs=None, locs=None):


nitpick: personally my first guess when reading create_org_admin is about admin user, not admin role. Maybe it's better to rename to create_org_admin_role to avoid confusion?

abalakh · 2017-11-21T11:10:43Z

tests/foreman/api/test_role.py

+        """
+        name = gen_string('alpha') if not name else name
+        orgs = [] if not orgs else orgs
+        locs = [] if not locs else locs


There's shorter form with no extra vars:

'role': { 'name': name, 'organization_ids': orgs or [], 'location_ids': locs or [], }

If orgs or locs is None - [] will be selected instead

Must only be careful when the var is a number because 0 will be evaluated to False and some times you just want to filter None. But in case of a list, this is fine.

abalakh · 2017-11-21T11:12:58Z

tests/foreman/api/test_role.py

+        cls.loc1 = entities.Location(name=gen_string('alpha')).create()
+        # These two will be used as filter taxonomies
+        cls.org2 = entities.Organization(name=gen_string('alpha')).create()
+        cls.loc2 = entities.Location(name=gen_string('alpha')).create()


nitpick: no need to provide names for orgs and locs explicitly, nailgun will generate them

abalakh · 2017-11-21T11:13:30Z

tests/foreman/api/test_role.py

@@ -115,8 +156,16 @@ def test_positive_create_role_with_taxonomies(self):



Pls remove :caseautomation: notautomated

Yeah, I have taken care of that

abalakh · 2017-11-21T11:17:14Z

tests/foreman/api/test_role.py

@@ -131,8 +180,16 @@ def test_positive_create_role_without_taxonomies(self):



:caseautomation: notautomated yet again. Pls check all the tests

abalakh · 2017-11-21T11:32:28Z

tests/foreman/api/test_role.py

+        self.assertEqual(role1.id, filter1.role.id)
+        # Creating new Taxonomies
+        org3 = entities.Organization(name=gen_string('alpha')).create()
+        loc3 = entities.Location(name=gen_string('alpha')).create()


nitpick: no need to provide names for orgs and locs explicitly, nailgun will generate them

abalakh · 2017-11-21T11:41:27Z

tests/foreman/api/test_role.py

+            query={'search': u'name="Organization admin"'})
+        org_admin = self.create_org_admin()
+        default_filters = entities.Role(
+            id=default_org_admin[0].id).read_json()['filters']


Pls avoid using .read_json() where it's possible. If filters field is not present in entities.Role() - then pls add that field and use regular .read()

abalakh · 2017-11-21T11:45:32Z

tests/foreman/api/test_role.py

+        user1 = entities.User(
+            login=user1_login,
+            password=user1_pass,
+            auth_source=1,


This line looks weird, and, basically there's no need in it since auth source with id=1 is automatically populated by nailgun, you may remove this line.

abalakh · 2017-11-21T11:47:21Z

tests/foreman/api/test_role.py

+                    'organization-id': self.org1.id,
+                    'location-id': self.loc1.id
+                }
+            )


Checking no errors were risen is not enough - please also ensure there're some results returned.

Actually, Its the test of user be able to access Domains, no matter if there is present anything. IF user doesnt have access to that org and location, it will return access error as HTTP error.

Ok, lemme rephrase - what if no errors are returned but the resulting list is always empty? Pretty often happens to CLI tests - if user doesn't have enough permissions some list command may return him empty list instead of failing. That's what i'm trying to avoid and cover.

Ok, I will create some entities and see if those are returned.

abalakh · 2017-11-21T11:52:26Z

tests/foreman/api/test_role.py

+            }
+        )
+        self.assertEqual(org_admin['name'], name)
+        return org_admin


result which user is receiving after creating an entity is usually entity read from the server (by EntityName(id=#).read() command), not some helper's raw output. Please read created entity and return it instead to avoid confusion and different styles like self.assertNotEqual(org_admin['id'], role.id)

renzon

ACK, but check and fix @abalakh comments

renzon · 2017-11-21T13:13:42Z

tests/foreman/api/test_role.py

+            location=[]
+        ).create()
+        self.assertEqual(role1.name, role_name)
+        self.assertEqual(role1.organization, [])


Well, I've never noticed that before. But I think asserting against an empty list is more explicit. assertFalse will pass for [], 0, '', None and empty dict. So one would need to execute the code to really know what's the type of role1.organization. Besides that the name role1.organizagion is odd. If a list is expected shouldn't the attribute be called "organizations"?

jyejare · 2017-11-22T11:36:34Z

@abalakh @renzon ,

Results after Fixing comments:

py.test -m 'not stubbed' tests/foreman/api/test_role.py::CannedRoleTestCases
=============================================================================================== test session starts ===============================================================================================
platform linux2 -- Python 2.7.11, pytest-3.2.3, py-1.4.34, pluggy-0.4.0
rootdir: /home/jitendrayejare2/Desktop/RedHat/RoboTelloNew/robottelo, inifile:
plugins: xdist-1.15.0, services-1.2.1, mock-1.6.3, cov-2.4.0
collected 43 items                                                                                                                                                                                                 
2017-11-22 17:01:32 - conftest - DEBUG - BZ deselect is disabled in settings


tests/foreman/api/test_role.py ....................

=============================================================================================== 23 tests deselected ===============================================================================================
=================================================================================== 20 passed, 23 deselected in 202.43 seconds ====================================================================================

jyejare · 2017-11-22T11:42:58Z

@abalakh , @renzon ,

Please review and merge -> SatelliteQE/nailgun#459

Then merge this one.

oshtaier · 2017-11-22T11:47:10Z

I will try to check that PR today, please do not merge it till tomorrow

jyejare · 2017-11-22T11:52:50Z

@oshtaier , Anyways 2 people have already reviewed it. You can feel free to opt out.

oshtaier · 2017-11-22T11:54:27Z

nope, I have some expertise in that subject, so please wait

oshtaier

pending questions

oshtaier · 2017-11-22T15:18:50Z

tests/foreman/api/test_role.py

@@ -30,6 +30,7 @@
    tier3,
    upgrade
 )
+from fauxfactory import gen_netmask, gen_ipaddr


put alphabetically

oshtaier · 2017-11-22T15:20:38Z

tests/foreman/api/test_role.py

+                }
+            }
+        )
+        self.assertEqual(org_admin['name'], name)


we usually do not put assertion into helper functions. We verified that clone work as it should in specific test cases, so here it should work by default

oshtaier · 2017-11-22T15:20:54Z

tests/foreman/api/test_role.py

+            }
+        )
+        self.assertEqual(org_admin['name'], name)
+        org_admin_role = entities.Role(id=org_admin['id']).read()


return entities.Role(id=org_admin['id']).read()

oshtaier · 2017-11-22T15:22:18Z

tests/foreman/api/test_role.py

+        """
+        super(CannedRoleTestCases, cls).setUpClass()
+        # These two will be used as role taxonomies
+        cls.org1 = entities.Organization().create()


so name it in that way:

cls.role_org cls.role_loc and cls.filter_org cls.filter_loc

I cannot divide them as filter org and role orgs, because in some test cases I used 'filter_orgs or org2' as role orgs as well. E.g to update the role orgs or so. So lets stick to this

oshtaier · 2017-11-22T15:23:14Z

tests/foreman/api/test_role.py

        :CaseImportance: Critical
        """
+        role_name = gen_string('alpha')
+        role1 = entities.Role(


use simple role =

oshtaier · 2017-11-22T15:51:57Z

tests/foreman/api/test_role.py

+        subnet1_name = gen_string('alpha')
+        subnet1 = entities.Subnet(
+            name=subnet1_name,
+            mask=gen_netmask(),


'mask': entity_fields.NetmaskField(required=True), 'name': entity_fields.StringField( required=True, str_type='alpha', length=(6, 12), unique=True ), 'network': entity_fields.IPAddressField(required=True),

so

entities.Subnet( organization=[self.org1.id], location=[self.loc1.id]).create()

will be enough

oshtaier · 2017-11-22T15:52:37Z

tests/foreman/api/test_role.py

+            organization=[self.org1.id],
+            location=[self.loc1.id],
+        ).create()
+        self.assertEqual(subnet1_name, subnet1.name)


redundant assertion

oshtaier · 2017-11-22T15:52:49Z

tests/foreman/api/test_role.py

+            organization=[self.org1.id],
+            location=[self.loc1.id]
+        ).create()
+        self.assertEqual(domain1_name, domain1.name)


redundant assertion

oshtaier · 2017-11-22T15:53:02Z

tests/foreman/api/test_role.py

+            location=[self.loc1.id],
+        ).create()
+        self.assertEqual(subnet1_name, subnet1.name)
+        domain1_name = gen_string('alpha')


redundant variable

oshtaier · 2017-11-22T16:01:55Z

tests/foreman/api/test_role.py


-        :expectedresults: Org Admin should not have access to create taxonomies
+            1. Org Admin should not have access to create organizations


+ 1. Org Admin should not have access to create organizations + 2. Org Admin should have access to create locations

why?

Because Org Admin is 'admin for org' but he can create the location. Thats the behavior feature provides.

jyejare · 2017-11-23T10:14:53Z

@oshtaier , I finished working on your comments, please revisit.

jyejare · 2017-11-28T14:12:53Z

@oshtaier , I am still waiting.

oshtaier

I need to spend double time just to check what I was asking and look into fixed code and that is not easy for >500 lines. Please, next time address comments in separate commits

oshtaier · 2017-11-28T19:06:59Z

and it is not only me waited as requested changes

jyejare · 2017-11-30T10:47:46Z

@oshtaier , I have addressed all your comments and raised a new commit. Please revisit.

abalakh

ACK, couple of suggestions. Still pending @oshtaier approval

abalakh · 2017-12-01T19:41:14Z

tests/foreman/api/test_role.py

+            )
+        self.assertIn(
+            domain.id,
+            [dom.id for dom in entities.Domain(sc).search()]


Why do you perform search for the second time here? You could just store results from lines above and ensure there's desired entity in output.

The steps and check below are completely the same, you can use for loop to avoid duplication. Smth like that:

for login, password in ( (userone_login, userone_password), (usertwo_login, usertwo_password)): with self.subTest(login): sc = ServerConfig( auth=(login, password), url=ServerConfig.get().url, verify=False ) [...]

oshtaier · 2017-12-04T09:34:16Z

That comment was not addressed

role = entities.Role(
    name=role_name,
    organization=[self.role_org],
    location=[self.role_loc]
).create()
self.assertEqual(role.name, role_name)
self.assertIn(self.role_org.id, [org.id for org in role.organization])
self.assertIn(self.role_loc.id, [loc.id for loc in role.location])

You created role with exactly one organization and location, so please, please, write proper assertions
And that through mostly all test cases

oshtaier · 2017-12-04T09:34:50Z

tests/foreman/api/test_role.py

+        ).create()
+        self.assertEqual(role.name, role_name)
+        dom_perm = entities.Permission(resource_type='Domain').search()
+        filtr = entities.Filter(


any reason to use word filtr instead filter?

FYI ... filter is reserved keyword in python

oshtaier · 2017-12-04T09:35:20Z

ACK pending 2 comments and comment from @abalakh

jyejare · 2017-12-04T16:05:52Z

@oshtaier and @abalakh All your comments has been addressed. Please have a look

oshtaier

ACK pending test results after all changes

jyejare · 2017-12-05T09:41:08Z

@oshtaier , @abalakh , The logs after all the changes,

 py.test -m 'not stubbed' tests/foreman/api/test_role.py::CannedRoleTestCases
=============================================================================================== test session starts ===============================================================================================
platform linux2 -- Python 2.7.11, pytest-3.3.0, py-1.5.2, pluggy-0.6.0
rootdir: /home/jitendrayejare2/Desktop/RedHat/RoboTelloNew/robottelo, inifile:
plugins: xdist-1.15.0, services-1.2.1, mock-1.6.3, cov-2.4.0
collected 43 items                                                                                                                                                                                                
2017-12-05 15:05:10 - conftest - DEBUG - BZ deselect is disabled in settings


tests/foreman/api/test_role.py ....................                                                                                                                                                         [100%]

=============================================================================================== 23 tests deselected ===============================================================================================
=================================================================================== 20 passed, 23 deselected in 275.65 seconds ====================================================================================

oshtaier

ACK

jyejare added the DO NOT MERGE label Nov 19, 2017

jyejare changed the title ~~[6.3 Feature] Canned Role Org Admin API stubs automated~~ [DND_untill_robotello-ci_458]+[6.3 Feature]: Canned Role Org Admin API stubs automated Nov 19, 2017

jyejare changed the title ~~[DND_untill_robotello-ci_458]+[6.3 Feature]: Canned Role Org Admin API stubs automated~~ [DND_untill_robotello-ci_458][6.3 Feature] Canned Role Org Admin API stubs automated Nov 19, 2017

jyejare requested review from sghai and svtkachenko November 19, 2017 18:23

jyejare changed the title ~~[DND_untill_robotello-ci_458][6.3 Feature] Canned Role Org Admin API stubs automated~~ [DNM_untill_robotello-ci_458][6.3 Feature] Canned Role Org Admin API stubs automated Nov 20, 2017

jyejare added the review label Nov 20, 2017

jyejare mentioned this pull request Nov 21, 2017

Entity classes updated to automate 6.3 Canned Role feature SatelliteQE/nailgun#458

Merged

jyejare changed the title ~~[DNM_untill_robotello-ci_458][6.3 Feature] Canned Role Org Admin API stubs automated~~ [6.3 Feature] Canned Role Org Admin API stubs automated Nov 21, 2017

jyejare removed the DO NOT MERGE label Nov 21, 2017

jyejare requested review from oshtaier and removed request for oshtaier November 21, 2017 11:02

abalakh suggested changes Nov 21, 2017

View reviewed changes

renzon approved these changes Nov 21, 2017

View reviewed changes

jyejare force-pushed the api_canned_role_automated branch from 7b70997 to 19796bb Compare November 21, 2017 14:48

SatelliteQE deleted a comment from abalakh Nov 22, 2017

jyejare force-pushed the api_canned_role_automated branch from 19796bb to 8a03c5a Compare November 22, 2017 11:39

jyejare added the DO NOT MERGE label Nov 22, 2017

jyejare changed the title ~~[6.3 Feature] Canned Role Org Admin API stubs automated~~ [DNM_untill_Nailgun_459][6.3 Feature] Canned Role Org Admin API stubs automated Nov 22, 2017

jyejare mentioned this pull request Nov 22, 2017

Added Filters field in Role entity SatelliteQE/nailgun#459

Merged

oshtaier suggested changes Nov 22, 2017

View reviewed changes

oshtaier removed the request for review from svtkachenko November 22, 2017 21:34

jyejare changed the title ~~[DNM_untill_Nailgun_459][6.3 Feature] Canned Role Org Admin API stubs automated~~ Canned Role Org Admin API stubs automated Nov 23, 2017

jyejare force-pushed the api_canned_role_automated branch from 8a03c5a to 741bb0d Compare November 23, 2017 09:17

jyejare changed the title ~~Canned Role Org Admin API stubs automated~~ [6.3 Feature] Canned Role Org Admin API stubs automated Nov 23, 2017

[6.3 Feature] Canned Role Org Admin API stubs automated

7f87dd2

jyejare force-pushed the api_canned_role_automated branch from 741bb0d to 7f87dd2 Compare November 24, 2017 14:51

jyejare removed the DO NOT MERGE label Nov 28, 2017

oshtaier reviewed Nov 28, 2017

View reviewed changes

Addressed special comments from Alex

d686865

jyejare requested review from oshtaier and removed request for sghai November 30, 2017 11:33

abalakh approved these changes Dec 1, 2017

View reviewed changes

oshtaier reviewed Dec 4, 2017

View reviewed changes

Comments from Alex and Abalakht adressed

adb9828

oshtaier approved these changes Dec 4, 2017

View reviewed changes

oshtaier approved these changes Dec 5, 2017

View reviewed changes

oshtaier merged commit 22b0e61 into SatelliteQE:master Dec 5, 2017

		@@ -115,8 +156,16 @@ def test_positive_create_role_with_taxonomies(self):

		@@ -131,8 +180,16 @@ def test_positive_create_role_without_taxonomies(self):


		:expectedresults: Org Admin should not have access to create taxonomies
		1. Org Admin should not have access to create organizations

[6.3 Feature] Canned Role Org Admin API stubs automated #5592

[6.3 Feature] Canned Role Org Admin API stubs automated #5592

Conversation

jyejare commented Nov 19, 2017

jyejare commented Nov 19, 2017

codecov bot commented Nov 19, 2017 • edited Loading

Codecov Report

jyejare commented Nov 19, 2017

jyejare commented Nov 21, 2017

abalakh left a 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

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

abalakh Nov 21, 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

Choose a reason for hiding this comment

renzon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyejare commented Nov 22, 2017

jyejare commented Nov 22, 2017

oshtaier commented Nov 22, 2017

jyejare commented Nov 22, 2017

oshtaier commented Nov 22, 2017

oshtaier left a 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

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

Choose a reason for hiding this comment

jyejare Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyejare commented Nov 23, 2017

jyejare commented Nov 28, 2017

oshtaier left a comment • edited Loading

Choose a reason for hiding this comment

oshtaier commented Nov 28, 2017

jyejare commented Nov 30, 2017

abalakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshtaier commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshtaier commented Dec 4, 2017

jyejare commented Dec 4, 2017

oshtaier left a comment

Choose a reason for hiding this comment

jyejare commented Dec 5, 2017

oshtaier left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2017 •

edited

Loading

abalakh Nov 21, 2017 •

edited

Loading

jyejare Nov 23, 2017 •

edited

Loading

oshtaier left a comment •

edited

Loading