Tests : User Tracker creation of user with minimal values #181

gkaihorodova · 2016-10-24T09:47:20Z

Fix provide possibility to create user-add test with minimal values,
where uid is not specified, to provide better coverage
https://fedorahosted.org/freeipa/ticket/6126

apophys · 2016-10-24T11:24:42Z

ipatests/test_xmlrpc/tracker/base.py

@@ -76,6 +76,7 @@ def __init__(self, default_version=None):
        self.api = api
        self.default_version = default_version or API_VERSION
        self._dn = None
+        self.attrs = {}


Please do not create conflict with PR #148

mirielka · 2016-10-25T06:48:53Z

The same minimal values apply for stageuser-add command, can you please modify the stageuser tracker as well? Also adding testcases for these changes would be nice.

gkaihorodova · 2016-10-25T08:54:30Z

Yes, It's a valid point to add testcases for these changes . Will do. Thank you.

mirielka · 2016-11-14T06:07:16Z

Having "None" default values for obligatory arguments does not seem to be a good idea. If the method was called with default values, it would fail. It would be best if obligatory arguments ("givenname" and "sn") were provided as positional arguments and voluntary "name" as keyword argument. Please note that such a change will cause failure of existing tests that use this tracker, therefore it's necessary to fix them as well - include this in separate commit of this PR. Also please don't forget to add testcases for which this PR was created originally - creating user without the "name" argument (both positive and negative testcases).

apophys · 2016-11-21T13:35:48Z

I think in this case we can go with keyword arguments only. Most of the uses of the tracker in the tests do it already. What I will need in the case of keyword arguments is an explicit check for some non-empty unicode string for the required attributes in the init method.

All of this applies to StageUserTracker in #210 as well

mirielka · 2016-12-06T10:05:40Z

ipatests/test_xmlrpc/tracker/user_plugin.py

+         in the init method """
+
+        if not isinstance(givenname, (str, unicode)) and len(givenname) > 0:
+            raise ValueError("No name provided: %s" % givenname)


The condition does not fit the description, it should contain some more parentheses:
if not (isinstance(givenname, (str, unicode)) and len(givenname) > 0):
Also you can use 'basestring' instead of '(str, unicode)'.
Please change the error message so that it would unambiguously tell which value is incorrect (e.g. "No or invalid first name provided").

basestring doesn't work with Py3, please use six.string_types

I agree with @mirielka the if statement looks suspicious

please use and givenname instead of and len(givenname) > 0, empty string is automatically False

please use %r in error message to get more information about object

Thank you for review, will check it and do required changes.

mirielka · 2016-12-06T10:05:55Z

ipatests/test_xmlrpc/tracker/user_plugin.py

+        if not isinstance(givenname, (str, unicode)) and len(givenname) > 0:
+            raise ValueError("No name provided: %s" % givenname)
+        if not isinstance(sn, (str, unicode)) and len(sn) > 0:
+            raise ValueError("No name provided: %s" % sn)


same as above

mirielka · 2016-12-06T10:09:37Z

Please check inline comments. Also suggestion for more test cases:

try to create a user whose automatically generated uid would be too long (>32 characters)
try to create a user whose automatically generated uid would contain invalid characters (other than letters, numbers, _, -, . and $).
Also goes for PR Tests: Stage User Tracker implementation #210

martbab · 2016-12-12T12:10:14Z

Bump for review.

MartinBasti · 2017-01-05T08:57:52Z

PR needs rebase

gkaihorodova · 2017-01-05T13:22:39Z

will do, but before let me do small changes that was requested by @stlaz in #210, to use str.format() instead of " %r "

MartinBasti · 2017-01-05T16:44:01Z

Then, @stlaz must give final ACK

stlaz · 2017-01-06T07:27:03Z

The changes introduce different behavior than in the previous change where repr() of the given strings would have been printed. To have repr() of the given string, use '{!r}' instead of '{}' in string format if that is what's desired. CondACK if it's not but please tell :) Same for #210

gkaihorodova · 2017-01-06T10:05:49Z

Yes, the intention was to have repr() of the given string , so I'll use ''{!r} instead of '{}', and apply that change to #210 also. Thank you.

stlaz · 2017-01-09T07:18:04Z

Thank you for the changes!

gkaihorodova · 2017-01-09T09:05:45Z

Thank you for review!

MartinBasti · 2017-01-12T09:59:21Z

This PR still needs rebase, it is not possible to apply patch without 3way merge, please pull the latest master and do rebase, we merge only patches that can be merged without 3-way merge

$ hub am https://github.com/freeipa/freeipa/pull/181 -3
Applying: Unaccessible variable self.attrs in Tracker
Using index info to reconstruct a base tree...
M	ipatests/test_xmlrpc/tracker/base.py
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: User Tracker: creation of user with minimal values
Using index info to reconstruct a base tree...
M	ipatests/test_xmlrpc/tracker/user_plugin.py
Falling back to patching base and 3-way merge...
Auto-merging ipatests/test_xmlrpc/tracker/user_plugin.py
Applying: User Tracker: Test to create user with minimal values

Fix provide possibility to create user-add test with minimal values, where uid is not specified, to provide better coverage. Also provide check for non-empty unicode string for attributes required in init method https://fedorahosted.org/freeipa/ticket/6126

Test to create user with minimal values, where uid is not specified https://fedorahosted.org/freeipa/ticket/6126

gkaihorodova · 2017-01-12T14:12:30Z

@mbasti-rh done. hope now it's fine

MartinBasti · 2017-01-18T08:36:45Z

@gkaihorodova you haven't pushed the changes to github repo

git push <github-remote> --force

MartinBasti · 2017-01-19T16:39:38Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/fa7aaef1de2c97ac9d24925ca9adb25c7151055f
https://fedorahosted.org/freeipa/changeset/91c050b4e093802d8c6b510a22d6e435faba965f

apophys suggested changes Oct 24, 2016

View reviewed changes

gkaihorodova force-pushed the fix-6126 branch from ba7fce8 to 27d87c9 Compare October 24, 2016 11:53

apophys approved these changes Oct 24, 2016

View reviewed changes

gkaihorodova force-pushed the fix-6126 branch from 27d87c9 to e568990 Compare October 24, 2016 14:43

gkaihorodova force-pushed the fix-6126 branch from e568990 to 66376aa Compare November 2, 2016 10:44

mirielka self-assigned this Nov 9, 2016

mirielka mentioned this pull request Nov 14, 2016

Tests: Stage User Tracker implementation #210

Closed

gkaihorodova force-pushed the fix-6126 branch from 66376aa to 6560828 Compare November 30, 2016 11:20

mirielka reviewed Dec 6, 2016

View reviewed changes

gkaihorodova force-pushed the fix-6126 branch from 79a9b65 to b0d51d7 Compare December 8, 2016 14:12

mirielka added the ack Pull Request approved, can be merged label Dec 15, 2016

gkaihorodova force-pushed the fix-6126 branch from b0d51d7 to 48ca442 Compare January 5, 2017 14:46

MartinBasti removed the ack Pull Request approved, can be merged label Jan 5, 2017

gkaihorodova force-pushed the fix-6126 branch from 48ca442 to 7205d9f Compare January 6, 2017 11:04

stlaz added the ack Pull Request approved, can be merged label Jan 9, 2017

gkaihorodova added 2 commits January 12, 2017 15:08

User Tracker: Test to create user with minimal values

43ee2ff

Test to create user with minimal values, where uid is not specified https://fedorahosted.org/freeipa/ticket/6126

gkaihorodova force-pushed the fix-6126 branch from 7205d9f to 43ee2ff Compare January 18, 2017 20:27

MartinBasti added the pushed Pull Request has already been pushed label Jan 19, 2017

MartinBasti closed this Jan 19, 2017

Tests : User Tracker creation of user with minimal values #181

Tests : User Tracker creation of user with minimal values #181

gkaihorodova commented Oct 24, 2016 •

edited by MartinBasti

apophys Oct 24, 2016

mirielka commented Oct 25, 2016

gkaihorodova commented Oct 25, 2016

mirielka commented Nov 14, 2016

apophys commented Nov 21, 2016

mirielka Dec 6, 2016

MartinBasti Dec 6, 2016 •

edited

gkaihorodova Dec 6, 2016

mirielka Dec 6, 2016

mirielka commented Dec 6, 2016

martbab commented Dec 12, 2016

MartinBasti commented Jan 5, 2017

gkaihorodova commented Jan 5, 2017

MartinBasti commented Jan 5, 2017

stlaz commented Jan 6, 2017

gkaihorodova commented Jan 6, 2017

stlaz commented Jan 9, 2017

gkaihorodova commented Jan 9, 2017

MartinBasti commented Jan 12, 2017

gkaihorodova commented Jan 12, 2017

MartinBasti commented Jan 18, 2017

MartinBasti commented Jan 19, 2017

Tests : User Tracker creation of user with minimal values #181

Tests : User Tracker creation of user with minimal values #181

Conversation

gkaihorodova commented Oct 24, 2016 • edited by MartinBasti

apophys Oct 24, 2016

Choose a reason for hiding this comment

mirielka commented Oct 25, 2016

gkaihorodova commented Oct 25, 2016

mirielka commented Nov 14, 2016

apophys commented Nov 21, 2016

mirielka Dec 6, 2016

Choose a reason for hiding this comment

MartinBasti Dec 6, 2016 • edited

Choose a reason for hiding this comment

gkaihorodova Dec 6, 2016

Choose a reason for hiding this comment

mirielka Dec 6, 2016

Choose a reason for hiding this comment

mirielka commented Dec 6, 2016

martbab commented Dec 12, 2016

MartinBasti commented Jan 5, 2017

gkaihorodova commented Jan 5, 2017

MartinBasti commented Jan 5, 2017

stlaz commented Jan 6, 2017

gkaihorodova commented Jan 6, 2017

stlaz commented Jan 9, 2017

gkaihorodova commented Jan 9, 2017

MartinBasti commented Jan 12, 2017

gkaihorodova commented Jan 12, 2017

MartinBasti commented Jan 18, 2017

MartinBasti commented Jan 19, 2017

gkaihorodova commented Oct 24, 2016 •

edited by MartinBasti

MartinBasti Dec 6, 2016 •

edited