Re: [Freeipa-devel] [PATCH 0028][Tests] Fix failing user tests

[Martin Babinsky]

On 07/20/2016 04:11 PM, Lenka Doudova wrote:



On 07/20/2016 02:04 PM, Martin Babinsky wrote:

On 07/15/2016 06:10 PM, Lenka Doudova wrote:

Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka





Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems
ok.

However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something
more specific like:

"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78
character width (IIRC) but your lines are way too long.

A nice way to automate this in vim is to highlight the text, enter
command mode and run 'gq', that should format the text for you.

3.) You did not add upstream ticket URL to the end of the message,
please do so.

There is also an extraneous whitespace here:
"""
 else:
 if type(value) is list:
 self.attrs[key] = value
 else:
 self.attrs[key] = [value]
+
 for key, value in expected_updates.items():
 if value is None or value is '' or value is u'':
 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format


Hi,

thanks for review, fixed patch attached.

Lenka


Thanks, ACK.

Pushed to master: 9093647f867e49bffc9042185b1765b48fe7400a

--
Martin^3 Babinsky

--
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

Re: [Freeipa-devel] [PATCH 0028][Tests] Fix failing user tests

[Lenka Doudova]

On 07/20/2016 02:04 PM, Martin Babinsky wrote:

On 07/15/2016 06:10 PM, Lenka Doudova wrote:

Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka





Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems 
ok.


However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something 
more specific like:


"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78 
character width (IIRC) but your lines are way too long.


A nice way to automate this in vim is to highlight the text, enter 
command mode and run 'gq', that should format the text for you.


3.) You did not add upstream ticket URL to the end of the message, 
please do so.


There is also an extraneous whitespace here:
"""
 else:
 if type(value) is list:
 self.attrs[key] = value
 else:
 self.attrs[key] = [value]
+
 for key, value in expected_updates.items():
 if value is None or value is '' or value is u'':
 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format


Hi,

thanks for review, fixed patch attached.

Lenka
From b76139052fa290111642464d3256b4ff15b184eb Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Fri, 15 Jul 2016 17:57:53 +0200
Subject: [PATCH] Tests: Improve handling of rename operation by user tracker

Improving handling of rename operation by user tracker, together with
fixes for user tests, that failed as consequence.
Failures were caused by RFE Kerberos principal alias.

Some tests were rewritten, since they used "--setattr" option instead of
"--rename", and hence didn't reflect proper behaviour of the principal
aliases feature.

https://fedorahosted.org/freeipa/ticket/6024
---
 ipatests/test_xmlrpc/test_user_plugin.py| 31 ++---
 ipatests/test_xmlrpc/tracker/user_plugin.py |  9 +
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py
index def522814f6c0a894f0bd8f352e110a95e5aa09a..7c27abc56cb859eb4fb710f1ff384793dfbe453c 100644
--- a/ipatests/test_xmlrpc/test_user_plugin.py
+++ b/ipatests/test_xmlrpc/test_user_plugin.py
@@ -316,24 +316,10 @@ class TestUpdate(XMLRPC_test):
 renameduser.ensure_missing()
 olduid = user.uid
 
-# using user.update(dict(uid=value)) results in
-# OverlapError: overlapping arguments and options: ['uid']
-user.attrs.update(uid=[renameduser.uid])
-command = user.make_update_command(
-updates=dict(setattr=(u'uid=%s' % renameduser.uid))
-)
-result = command()
-user.check_update(result)
-user.uid = renameduser.uid
+user.update(updates=dict(rename=renameduser.uid))
 
 # rename the test user back so it gets properly deleted
-user.attrs.update(uid=[olduid])
-command = user.make_update_command(
-updates=dict(setattr=(u'uid=%s' % olduid))
-)
-result = command()
-user.check_update(result)
-user.uid = olduid
+user.update(updates=dict(rename=olduid))
 
 def test_rename_to_the_same_value(self, user):
 """ Try to rename user to the same value """
@@ -640,18 +626,13 @@ class TestUserWithGroup(XMLRPC_test):
 if its manager is also renamed """
 renamed_name = u'renamed_npg2'
 old_name = user_npg2.uid
-command = user_npg2.make_update_command(dict(rename=renamed_name))
-result = command()
-user_npg2.attrs.update(uid=[renamed_name])
-user_npg2.check_update(result)
+
+user_npg2.update(updates=dict(rename=renamed_name))
+
 user_npg.attrs.update(manager=[renamed_name])
 user_npg.retrieve(all=True)
 
-command = user_npg2.make_command(
-'user_mod', renamed_name, **dict(rename=old_name)
-)
-# we rename the user back otherwise the tracker is too confused
-result = command()
+user_npg2.update(updates=dict(rename=old_name))
 
 def test_check_if_manager_gets_removed(self, user_npg, user_npg2):
 """ Delete manager and check if it's gone from user's attributes """
diff --git a/ipatests/test_xmlrpc/tracker/user_plugin.py b/ipatests/test_xmlrpc/tracker/user_plugin.py
index 1a85e93327e5d517249fd67e208e83a922509002..d95c3e223106451b4114ace4ddf564085e6a0511 100644
--- 

Re: [Freeipa-devel] [PATCH 0028][Tests] Fix failing user tests

[Martin Babinsky]

On 07/15/2016 06:10 PM, Lenka Doudova wrote:

Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka





Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems ok.

However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something more 
specific like:


"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78 character 
width (IIRC) but your lines are way too long.


A nice way to automate this in vim is to highlight the text, enter 
command mode and run 'gq', that should format the text for you.


3.) You did not add upstream ticket URL to the end of the message, 
please do so.


There is also an extraneous whitespace here:
"""
 else:
 if type(value) is list:
 self.attrs[key] = value
 else:
 self.attrs[key] = [value]
+
 for key, value in expected_updates.items():
 if value is None or value is '' or value is u'':
 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format

--
Martin^3 Babinsky

--
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