[Freeipa-devel] [PATCH 0002] TEST: Stageuser plugin

Tue Aug 11 08:57:02 UTC 2015


On 08/11/2015 10:06 AM, thierry bordaz wrote:
> On 08/04/2015 01:37 PM, Lenka Doudova wrote:
>>
>>
>> Dne 30.7.2015 v 16:10 Martin Basti napsal(a):
>>> On 30/07/15 16:09, Martin Basti wrote:
>>>> On 29/07/15 16:10, Martin Basti wrote:
>>>>> On 29/07/15 15:29, Lenka Doudova wrote:
>>>>>> Hi,
>>>>>>
>>>>>> thanks a lot for the comments, will work on it tomorrow.
>>>>>>
>>>>>> Lenka
>>>>>>
>>>>>> Dne 29.7.2015 v 15:27 Martin Basti napsal(a):
>>>>>>> On 27/07/15 16:47, Lenka Doudova wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I'm attaching a patch with automated tests for stageuser plugin 
>>>>>>>> (https://fedorahosted.org/freeipa/ticket/3813). The user plugin 
>>>>>>>> test is affected as well (one class was added).
>>>>>>>> The tests seem a bit of a mess even to myself, but what with 
>>>>>>>> the way freeipa behaves I didn't know how else to implement 
>>>>>>>> them, but I'm eager to learn how to do it in a nicer way, if 
>>>>>>>> someone has a better idea.
>>>>>>>>
>>>>>>>> Lenka
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> I just applied patches:
>>>>>>>
>>>>>>> 1) Please remove whitespace errors
>>>>>>> $ git am 
>>>>>>> freeipa-lryznaro-0002-Automated-test-for-stageuser-plugin.patch
>>>>>>> Applying: Automated test for stageuser plugin
>>>>>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:110: 
>>>>>>> trailing whitespace.
>>>>>>>     """ Tracker class for staged user LDAP object
>>>>>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:113: 
>>>>>>> trailing whitespace.
>>>>>>>         StageUserTracker object stores information about the user.
>>>>>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:121: 
>>>>>>> trailing whitespace.
>>>>>>>         u'krbprincipalexpiration', u'usercertificate', u'dn', 
>>>>>>> u'has_keytab', u'has_password',
>>>>>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:122: 
>>>>>>> trailing whitespace.
>>>>>>>         u'street', u'postalcode', u'facsimiletelephonenumber', 
>>>>>>> u'carlicense',
>>>>>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:125: 
>>>>>>> trailing whitespace.
>>>>>>>         u'cn', u'ipauniqueid', u'objectclass', u'description',
>>>>>>> warning: squelched 50 whitespace errors
>>>>>>> warning: 55 lines add whitespace errors.
>>>>>>>
>>>>>>> 2)
>>>>>>> Please use new shorter format of license header
>>>>>>>
>>>>>>> 3) can you fix some of the most serious PEP8 errors
>>>>>>> $ git show -U0 | pep8 --diff | wc -l
>>>>>>> 198
>>>>>>>
>>>>>>> 4)
>>>>>>> if options != None:
>>>>>>>
>>>>>>> Please use "options *is not* None"
>>>>>>>
>>>>>>> 5)
>>>>>>> For consistency it should be u'random'
>>>>>>> if key == 'random':
>>>>>>>                     self.attrs[u'randompassword'] = fuzzy_string
>>>>>>>
>>>>>>> Otherwise it looks good
>>>>>>> Martin^2
>>>>>>> -- 
>>>>>>> Martin Basti
>>>>>>
>>>>> And also fix this please
>>>>>
>>>>> ./make-lint
>>>>> ************* Module ipatests.test_xmlrpc.test_stageuser_plugin
>>>>> ipatests/test_xmlrpc/test_stageuser_plugin.py:337: 
>>>>> [E0102(function-redefined), user2] function already defined line 44)
>>>>>
>>>>> -- 
>>>>> Martin Basti
>>>>>
>>>>>
>>>> Ahoj, v patchi mas este uvedene svoje stare meno, mala by si v gite 
>>>> nastavit redhat email
>>>>
>>>> -- 
>>>> Martin Basti
>>>>
>>>>
>>> Sorry for spam, you can safely ignore this. :)
>>>
>>> -- 
>>> Martin Basti
>>>
>>>
>> Attaching new patch - (hopefully) fixed the errors from the old one + 
>> few test cases were added.
>>
>> Lenka
>>
>>
>
>
> Hello Lenka,
>
> This is a very impressive work and test framework. I have not 
> understood all the details of the implementation so I just focus on 
> the reading the tests body.
> The patch is looking great to me and I have really few minors comments.
>
>   * About non existing stage user, you may try to activate a non
>     existing one. Is it what TestStagedUser.test_activate does ?
>
No, it didn't occur to me to test activation of nonexistent user, I can 
add it.
>
>   * In test_create_attr, I can see that user6 is activated. How is
>     checked that the specified values are preserved ? (sorry my python
>     skill is still very low)
>
The values are checked - in the test itself, using 'create_from_staged' 
I first copy attributes of stage user object to a new active user object 
(just object, not the ipa user itself) and in 'check_activate' I verify 
that values of ipa user correspond with those taken from staged user
>
>   * Many testcases
>     (http://www.freeipa.org/page/V4/User_Life-Cycle_Management/Test_Plan#Test_case:_Try_to_search_for_a_nonexistent_user)
>     are about creating a stage user with various attributes
>     (initial,shell, homedir...). I found uid/gid, are the others
>     implemented ?
>
Yes, all the options are implemented using parametrized fixture 
'stageduser2' + 'options_ok' list, tests are performed by 
'test_create_attr' method. These tests also contain activation check 
(users can be activated and values are preserved).
>
>   * In TestActive.test_delete_preserve, does check_delete check the
>     active container or the stageuser container ?
>
check_delete just checks the result of 'user-del' command (it just says 
that a certain user has been deleted). But after that, there is a 
verification that the user is no longer in _active_ container performed 
by running 'user-show' command with 'not found' as expected result.
>
>   * Does test_delete_preserved checks that the deleted entry has been
>     permanently removed ?
>
If you mean whether there is a check like running 'user-show' command 
and expecting 'not found', no, there is not. I haven't thought about it. 
Can add.

>   * Is test_preserved_membership the test case for
>     https://fedorahosted.org/freeipa/ticket/5170 ?
>
I'd say that partly - it just verifies if membership can be added to a 
preserved user (expects failure of such attempt).
It does not check whether a user with thus successfully assigned 
membership preserves it after 'user-undel' (but that is part of 
TestPreserved.test_reactivate_preserved - the 'check_attr_preservation' 
method verifies that reactivated user is member of 'ipausers' group only).

Lenka
>
> Thanks
> thierry
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150811/1d881941/attachment.htm>