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

[thierry bordaz]

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 ?
  * 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)
  * 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 ?
  * In TestActive.test_delete_preserve, does check_delete check the
    active container or the stageuser container ?
  * Does test_delete_preserved checks that the deleted entry has been
    permanently removed ?
  * Is test_preserved_membership the test case for
    https://fedorahosted.org/freeipa/ticket/5170 ?

Thanks
thierry

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