On 08/11/2015 10:57 AM, Lenka Doudova wrote:


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




NACK

syntax error, missing ')'
-from ipatests.util import assert_equal, assert_not_equal, raises
+from ipatests.util import (
+    assert_equal, assert_not_equal, raises, assert_deepequal

I cannot apply this patch, please check it
-- 
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