On 12/09/2015 10:13 AM, Martin Basti wrote:


On 09.12.2015 09:41, Lenka Doudova wrote:
Hi,

attaching fixed patches for master and ipa-4-2 branch.
Also fixes test accordingly to https://fedorahosted.org/freeipa/ticket/5387.

Lenka

On 11/20/2015 12:13 PM, Martin Babinsky wrote:
On 11/19/2015 10:34 AM, Petr Viktorin wrote:
On 11/19/2015 09:30 AM, Lenka Doudova wrote:
On 11/18/2015 04:51 PM, Martin Babinsky wrote:
On 11/18/2015 02:16 PM, Lenka Doudova wrote:
Hi,

here's a patch that adds a few comments to stageuser tests in order to
allow easier determining of a problem when tests fail.

Lenka



Hi Lenka,

Firstly a technical detail: Python indexes lists from 0, so the
comments in 'options_ok' do not correctly map to the test names anyway.

I am also not sure if this patch is worth reviewing and pushing as it
IMHO doesn't help in the identification of failed tests at all.

This should be solved at more fundamental level.

Ouch, good point, I didn't realize. Sorry.

Anyway, the issue is that even if tests are run in verbose mode, you get
output like this:

ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser27]
PASSED

ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser28]
PASSED

ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser29]
PASSED

ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser210]
PASSED

ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser211]
PASSED

ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser212]
PASSED


If some test fails, you can't really tell which command was the one
responsible for the fail. This should be solved by
https://fedorahosted.org/freeipa/ticket/5449. Until that's done, though, the only way to find out which command failed is looking at the code and finding which parameters were put into the command, which was not much
possible without better commenting the test case (as I realized last
week when David Kupka asked me to help him find the reason for failed
tests).
Obviously I can rewrite the tests so there's 27 separate test cases, one
for each parameter, instead of one method that 'unwraps' into 27 test
cases, which would entirely eliminate the confusion. So far I don't know
of a way to put 27 similar test cases in one method which would allow
easy recognition of the test cases.
I'll wait with fixing the patch until further discussion.

Hello,
Pytest wants you to be a bit more explicit about how to name the
parameters. (It "hides" dicts by default, because large dicts would make
the output even more confusing than the numbers.)

Please try the attached patch.
Docs are at https://pytest.org/latest/fixture.html#parametrizing-a-fixture



This looks like a better approach IMHO, you can then see which attribute/value was being checked.

I would very much favor more descriptive test/fixture names in the beginning.




Hello,

we use usually bottom posting on freeipa-devel please try to keep reply in this way.

Patch:

I do not like the idea of separated lists, IMO it is hard to manage and is easy to create mistakes.

How about this (untested, just from top of my head): http://fpaste.org/298994/65184014/

Martin

Great idea, thanks. Fixed patches attached.

Lenka
From 646db80b7cc0912c310615d804fcb1561e19961f Mon Sep 17 00:00:00 2001
From: Lenka Doudova <ldoud...@redhat.com>
Date: Mon, 23 Nov 2015 10:27:07 +0100
Subject: [PATCH] Adding descriptive IDs to stageuser tests

Adding descriptive IDs to parametrized stageuser test for better identification of test cases.
---
 ipatests/test_xmlrpc/test_stageuser_plugin.py    | 84 ++++++++++++++----------
 ipatests/test_xmlrpc/tracker/stageuser_plugin.py |  2 +-
 ipatests/test_xmlrpc/tracker/user_plugin.py      |  9 ++-
 3 files changed, 56 insertions(+), 39 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_stageuser_plugin.py b/ipatests/test_xmlrpc/test_stageuser_plugin.py
index 4eb968451f926ca0ee8fa5aeae1a96770f56eb45..42ecf046884369bd74b03194937c425215b99f6c 100644
--- a/ipatests/test_xmlrpc/test_stageuser_plugin.py
+++ b/ipatests/test_xmlrpc/test_stageuser_plugin.py
@@ -15,6 +15,7 @@ import pytest
 
 import six
 
+from collections import OrderedDict
 from ipalib import api, errors
 
 from ipatests.test_xmlrpc import objectclasses
@@ -53,35 +54,40 @@ sshpubkey = (u'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDGAX3xAeLeaJggwTqMjxNwa6X'
 sshpubkeyfp = (u'13:67:6B:BF:4E:A2:05:8E:AE:25:8B:A1:31:DE:6F:1B '
                'public key test (ssh-rsa)')
 
-options_ok = [
-    {u'cn': u'name'},
-    {u'initials': u'in'},
-    {u'displayname': u'display'},
-    {u'homedirectory': u'/home/homedir'},
-    {u'gecos': u'gecos'},
-    {u'loginshell': u'/bin/shell'},
-    {u'mail': u'email@email.email'},
-    {u'title': u'newbie'},
-    {u'krbprincipalname': u'kerberos@%s' % api.env.realm},
-    {u'krbprincipalname': u'KERBEROS@%s' % api.env.realm},
-    {u'street': u'first street'},
-    {u'l': u'prague'},
-    {u'st': u'czech'},
-    {u'postalcode': u'12345'},
-    {u'telephonenumber': u'123456789'},
-    {u'facsimiletelephonenumber': u'123456789'},
-    {u'mobile': u'123456789'},
-    {u'pager': u'123456789'},
-    {u'ou': u'engineering'},
-    {u'carlicense': u'abc1234'},
-    {u'ipasshpubkey': sshpubkey},
-    {u'manager': u'auser1'},
-    {u'uidnumber': uid},
-    {u'gidnumber': gid},
-    {u'uidnumber': uid, u'gidnumber': gid},
-    {u'userpassword': u'Secret123'},
-    {u'random': True},
-    ]
+options_def = OrderedDict([
+    ('full name', {u'cn': u'name'}),
+    ('initials', {u'initials': u'in'}),
+    ('display name', {u'displayname': u'display'}),
+    ('home directory', {u'homedirectory': u'/home/homedir'}),
+    ('GECOS', {u'gecos': u'gecos'}),
+    ('shell', {u'loginshell': u'/bin/shell'}),
+    ('email address', {u'mail': u'email@email.email'}),
+    ('job title', {u'title': u'newbie'}),
+    ('kerberos principal', {
+        u'krbprincipalname': u'kerberos@%s' % api.env.realm}),
+    ('uppercase kerberos principal', {
+        u'krbprincipalname': u'KERBEROS@%s' % api.env.realm}),
+    ('street address', {u'street': u'first street'}),
+    ('city', {u'l': u'prague'}),
+    ('state', {u'st': u'czech'}),
+    ('zip code', {u'postalcode': u'12345'}),
+    ('telephone number', {u'telephonenumber': u'123456789'}),
+    ('fax number', {u'facsimiletelephonenumber': u'123456789'}),
+    ('mobile tel. number', {u'mobile': u'123456789'}),
+    ('pager number', {u'pager': u'123456789'}),
+    ('organizational unit', {u'ou': u'engineering'}),
+    ('car license', {u'carlicense': u'abc1234'}),
+    ('SSH key', {u'ipasshpubkey': sshpubkey}),
+    ('manager', {u'manager': u'auser1'}),
+    ('user ID number', {u'uidnumber': uid}),
+    ('group ID number', {u'gidnumber': gid}),
+    ('UID and GID numbers', {u'uidnumber': uid, u'gidnumber': gid}),
+    ('password', {u'userpassword': u'Secret123'}),
+    ('random password', {u'random': True}),
+    ])
+
+options_ok = options_def.values()
+options_ids = options_def.keys()
 
 
 @pytest.fixture(scope='class')
@@ -90,13 +96,19 @@ def stageduser(request):
     return tracker.make_fixture(request)
 
 
-@pytest.fixture(scope='class', params=options_ok)
+@pytest.fixture(scope='class', params=options_ok, ids=options_ids)
 def stageduser2(request):
     tracker = StageUserTracker(u'suser2', u'staged', u'user', **request.param)
     return tracker.make_fixture_activate(request)
 
 
 @pytest.fixture(scope='class')
+def user_activated(request):
+    tracker = UserTracker(u'suser2', u'staged', u'user')
+    return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
 def stageduser3(request):
     tracker = StageUserTracker(name=u'suser3', givenname=u'staged', sn=u'user')
     return tracker.make_fixture_activate(request)
@@ -220,7 +232,7 @@ class TestStagedUser(XMLRPC_test):
     def test_showall_stageduser(self, stageduser):
         stageduser.retrieve(all=True)
 
-    def test_create_attr(self, stageduser2, user, user6):
+    def test_create_with_attr(self, stageduser2, user, user_activated):
         """ Tests creating a user with various valid attributes listed
         in 'options_ok' list"""
         # create staged user with specified parameters
@@ -233,14 +245,14 @@ class TestStagedUser(XMLRPC_test):
 
         # activate user, verify that specified values were preserved
         # after activation
-        user6.ensure_missing()
-        user6 = UserTracker(
+        user_activated.ensure_missing()
+        user_activated = UserTracker(
             stageduser2.uid, stageduser2.givenname,
             stageduser2.sn, **stageduser2.kwargs)
-        user6.create_from_staged(stageduser2)
+        user_activated.create_from_staged(stageduser2)
         command = stageduser2.make_activate_command()
         result = command()
-        user6.check_activate(result)
+        user_activated.check_activate(result)
 
         # verify the staged user does not exist after activation
         command = stageduser2.make_retrieve_command()
@@ -248,7 +260,7 @@ class TestStagedUser(XMLRPC_test):
                 reason=u'%s: stage user not found' % stageduser2.uid)):
             command()
 
-        user6.delete()
+        user_activated.delete()
 
     def test_delete_stageduser(self, stageduser):
         stageduser.delete()
diff --git a/ipatests/test_xmlrpc/tracker/stageuser_plugin.py b/ipatests/test_xmlrpc/tracker/stageuser_plugin.py
index 09ad282e417058f40f1e7d0b9ae36e90c78af8e1..0f7eadd040d5fd12958a0651ddf00cbc4055fa1f 100644
--- a/ipatests/test_xmlrpc/tracker/stageuser_plugin.py
+++ b/ipatests/test_xmlrpc/tracker/stageuser_plugin.py
@@ -128,7 +128,7 @@ class StageUserTracker(Tracker):
                     (self.kwargs[key].split('@'))[0].lower(),
                     (self.kwargs[key].split('@'))[1])]
             elif key == u'manager':
-                self.attrs[key] = [unicode(get_user_dn(self.kwargs[key]))]
+                self.attrs[key] = [self.kwargs[key]]
             elif key == u'ipasshpubkey':
                 self.attrs[u'sshpubkeyfp'] = [sshpubkeyfp]
                 self.attrs[key] = [self.kwargs[key]]
diff --git a/ipatests/test_xmlrpc/tracker/user_plugin.py b/ipatests/test_xmlrpc/tracker/user_plugin.py
index af7d85836a5c514c4e208696398a68ab341e825f..bcae2ec787b3d3a54e34917f125be006094e07b0 100644
--- a/ipatests/test_xmlrpc/tracker/user_plugin.py
+++ b/ipatests/test_xmlrpc/tracker/user_plugin.py
@@ -5,9 +5,10 @@
 from ipalib import api, errors
 from ipapython.dn import DN
 
-from ipatests.util import assert_deepequal, get_group_dn
+from ipatests.util import assert_deepequal, get_group_dn, get_user_dn
 from ipatests.test_xmlrpc import objectclasses
-from ipatests.test_xmlrpc.xmlrpc_test import fuzzy_digits, fuzzy_uuid, raises_exact
+from ipatests.test_xmlrpc.xmlrpc_test import (
+    fuzzy_digits, fuzzy_uuid, raises_exact)
 from ipatests.test_xmlrpc.tracker.base import Tracker
 
 
@@ -258,6 +259,10 @@ class UserTracker(Tracker):
             summary=u'Stage user %s activated' % self.uid,
             result=self.filter_attrs(self.activate_keys))
 
+        if 'manager' in expected['result']:
+            expected['result']['manager'] = [
+                unicode(get_user_dn(expected['result']['manager'][0]))]
+
         # work around to eliminate inconsistency in returned objectclass
         # (case sensitive assertion)
         expected['result']['objectclass'] = [item.lower() for item in
-- 
2.4.3

From 18fef36b9ed4aa365683cb8e7baa3f37fb5295bc Mon Sep 17 00:00:00 2001
From: Lenka Doudova <ldoud...@redhat.com>
Date: Wed, 9 Dec 2015 08:49:53 +0100
Subject: [PATCH] Adding descriptive IDs to stageuser tests

Adding descriptive IDs to parametrized stageuser test for better identification of test cases.
---
 ipatests/test_xmlrpc/test_stageuser_plugin.py | 87 +++++++++++++++------------
 ipatests/test_xmlrpc/test_user_plugin.py      |  4 ++
 2 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_stageuser_plugin.py b/ipatests/test_xmlrpc/test_stageuser_plugin.py
index d645c4470d28c845bc609bdc6cb3a2ca7d4ca004..ed5fc8b72a944e89a35e1960fee54852955d1a58 100644
--- a/ipatests/test_xmlrpc/test_stageuser_plugin.py
+++ b/ipatests/test_xmlrpc/test_stageuser_plugin.py
@@ -13,6 +13,8 @@ import re
 import functools
 import pytest
 
+from collections import OrderedDict
+
 from ipalib import api, errors
 
 from ipatests.test_xmlrpc.ldaptracker import Tracker
@@ -48,35 +50,40 @@ sshpubkey = (u'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDGAX3xAeLeaJggwTqMjxNwa6X'
 sshpubkeyfp = (u'13:67:6B:BF:4E:A2:05:8E:AE:25:8B:A1:31:DE:6F:1B '
                'public key test (ssh-rsa)')
 
-options_ok = [
-    {u'cn': u'name'},
-    {u'initials': u'in'},
-    {u'displayname': u'display'},
-    {u'homedirectory': u'/home/homedir'},
-    {u'gecos': u'gecos'},
-    {u'loginshell': u'/bin/shell'},
-    {u'mail': u'email@email.email'},
-    {u'title': u'newbie'},
-    {u'krbprincipalname': u'kerberos@%s' % api.env.realm},
-    {u'krbprincipalname': u'KERBEROS@%s' % api.env.realm},
-    {u'street': u'first street'},
-    {u'l': u'prague'},
-    {u'st': u'czech'},
-    {u'postalcode': u'12345'},
-    {u'telephonenumber': u'123456789'},
-    {u'facsimiletelephonenumber': u'123456789'},
-    {u'mobile': u'123456789'},
-    {u'pager': u'123456789'},
-    {u'ou': u'engineering'},
-    {u'carlicense': u'abc1234'},
-    {u'ipasshpubkey': sshpubkey},
-    {u'manager': u'auser1'},
-    {u'uidnumber': uid},
-    {u'gidnumber': gid},
-    {u'uidnumber': uid, u'gidnumber': gid},
-    {u'userpassword': u'Secret123'},
-    {u'random': True},
-    ]
+options_def = OrderedDict([
+    ('full name', {u'cn': u'name'}),
+    ('initials', {u'initials': u'in'}),
+    ('display name', {u'displayname': u'display'}),
+    ('home directory', {u'homedirectory': u'/home/homedir'}),
+    ('GECOS', {u'gecos': u'gecos'}),
+    ('shell', {u'loginshell': u'/bin/shell'}),
+    ('email address', {u'mail': u'email@email.email'}),
+    ('job title', {u'title': u'newbie'}),
+    ('kerberos principal', {
+        u'krbprincipalname': u'kerberos@%s' % api.env.realm}),
+    ('uppercase kerberos principal', {
+        u'krbprincipalname': u'KERBEROS@%s' % api.env.realm}),
+    ('street address', {u'street': u'first street'}),
+    ('city', {u'l': u'prague'}),
+    ('state', {u'st': u'czech'}),
+    ('zip code', {u'postalcode': u'12345'}),
+    ('telephone number', {u'telephonenumber': u'123456789'}),
+    ('fax number', {u'facsimiletelephonenumber': u'123456789'}),
+    ('mobile tel. number', {u'mobile': u'123456789'}),
+    ('pager number', {u'pager': u'123456789'}),
+    ('organizational unit', {u'ou': u'engineering'}),
+    ('car license', {u'carlicense': u'abc1234'}),
+    ('SSH key', {u'ipasshpubkey': sshpubkey}),
+    ('manager', {u'manager': u'auser1'}),
+    ('user ID number', {u'uidnumber': uid}),
+    ('group ID number', {u'gidnumber': gid}),
+    ('UID and GID numbers', {u'uidnumber': uid, u'gidnumber': gid}),
+    ('password', {u'userpassword': u'Secret123'}),
+    ('random password', {u'random': True}),
+    ])
+
+options_ok = options_def.values()
+options_ids = options_def.keys()
 
 
 class StageUserTracker(Tracker):
@@ -179,7 +186,7 @@ class StageUserTracker(Tracker):
                     (self.kwargs[key].split('@'))[0].lower(),
                     (self.kwargs[key].split('@'))[1])]
             elif key == u'manager':
-                self.attrs[key] = [unicode(get_user_dn(self.kwargs[key]))]
+                self.attrs[key] = [self.kwargs[key]]
             elif key == u'ipasshpubkey':
                 self.attrs[u'sshpubkeyfp'] = [sshpubkeyfp]
                 self.attrs[key] = [self.kwargs[key]]
@@ -324,13 +331,19 @@ def stageduser(request):
     return tracker.make_fixture(request)
 
 
-@pytest.fixture(scope='class', params=options_ok)
+@pytest.fixture(scope='class', params=options_ok, ids=options_ids)
 def stageduser2(request):
     tracker = StageUserTracker(u'suser2', u'staged', u'user', **request.param)
     return tracker.make_fixture_activate(request)
 
 
 @pytest.fixture(scope='class')
+def user_activated(request):
+    tracker = UserTracker(u'suser2', u'staged', u'user')
+    return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
 def stageduser3(request):
     tracker = StageUserTracker(name=u'suser3', givenname=u'staged', sn=u'user')
     return tracker.make_fixture_activate(request)
@@ -454,7 +467,7 @@ class TestStagedUser(XMLRPC_test):
     def test_showall_stageduser(self, stageduser):
         stageduser.retrieve(all=True)
 
-    def test_create_attr(self, stageduser2, user, user6):
+    def test_create_with_attr(self, stageduser2, user, user_activated):
         """ Tests creating a user with various valid attributes listed
         in 'options_ok' list"""
         # create staged user with specified parameters
@@ -467,14 +480,14 @@ class TestStagedUser(XMLRPC_test):
 
         # activate user, verify that specified values were preserved
         # after activation
-        user6.ensure_missing()
-        user6 = UserTracker(
+        user_activated.ensure_missing()
+        user_activated = UserTracker(
             stageduser2.uid, stageduser2.givenname,
             stageduser2.sn, **stageduser2.kwargs)
-        user6.create_from_staged(stageduser2)
+        user_activated.create_from_staged(stageduser2)
         command = stageduser2.make_activate_command()
         result = command()
-        user6.check_activate(result)
+        user_activated.check_activate(result)
 
         # verify the staged user does not exist after activation
         command = stageduser2.make_retrieve_command()
@@ -482,7 +495,7 @@ class TestStagedUser(XMLRPC_test):
                 reason=u'%s: stage user not found' % stageduser2.uid)):
             command()
 
-        user6.delete()
+        user_activated.delete()
 
     def test_delete_stageduser(self, stageduser):
         stageduser.delete()
diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py
index 81185e449acaa127aa9429fff9587d39a2be81e6..cce769f8f31fc05df218fd4f5afa8978bce091be 100644
--- a/ipatests/test_xmlrpc/test_user_plugin.py
+++ b/ipatests/test_xmlrpc/test_user_plugin.py
@@ -1903,6 +1903,10 @@ class UserTracker(Tracker):
             summary=u'Stage user %s activated' % self.uid,
             result=self.filter_attrs(self.activate_keys))
 
+        if 'manager' in expected['result']:
+            expected['result']['manager'] = [
+                unicode(get_user_dn(expected['result']['manager'][0]))]
+
         # work around to eliminate inconsistency in returned objectclass
         # (case sensitive assertion)
         expected['result']['objectclass'] = [item.lower() for item in
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to