[Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin

Tue Jan 19 14:49:31 UTC 2016

On 01/15/2016 03:41 PM, Filip Skola wrote:
> Hi,
>
> sending rebased patch on top of 58c42ddac0964a8cce7c1e1faa7516da53f028ad.
>
> Includes a "fix" for the rename-to-invalid-username issue for the new version.
>
> F.
>
> ----- Original Message -----
>> Hi,
>>
>> I don't know what is causing the \r\n issue. I use vim and than send each
>> email with claws-mail. Didn't spot this issue when trying emailing the patch
>> to my other address. I'm trying to send it from zimbra now, let me know if
>> that helped pls.
>>
>> Fix for the stageuser plugin issues caused by this patch should have been
>> included in the last update; I think the remaining issue is not caused by
>> UserTracker changes. Please correct me, if I'm wrong.
>>
>>> There is some issue with "test_rename_to_too_long_login" test. It fails but
>>> actually this is false positive because it is possible to create login upto
>>> 255 characters. I don't know why test mentions 32 characters without any
>>> other modified setup.
>>> NACK for now.
>>>   - alich -
>> This has been changed. This test still fails, though.
>>
>> Filip
>>
>>>
>>> ----- Original Message -----
>>>> From: "Aleš Mareček" <amarecek at redhat.com>
>>>> To: "Filip Škola" <fskola at redhat.com>
>>>> Cc: freeipa-devel at redhat.com, "Milan Kubík" <mkubik at redhat.com>
>>>> Sent: Thursday, December 10, 2015 4:11:47 PM
>>>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
>>>>
>>>> Ah, sorry, haven't realized there had been devel list attached.
>>>> Ok, there is some problem with \r\n in the patch.
>>>> Filip, please take a look at it...
>>>> Thanks...
>>>>   - alich -
>>>>
>>>> ----- Original Message -----
>>>>> From: "Filip Škola" <fskola at redhat.com>
>>>>> To: "Aleš Mareček" <amarecek at redhat.com>
>>>>> Cc: freeipa-devel at redhat.com, "Milan Kubík" <mkubik at redhat.com>
>>>>> Sent: Thursday, December 10, 2015 11:29:52 AM
>>>>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
>>>>>
>>>>> Hi,
>>>>>
>>>>> this if fixed. Also issues with test_stageuser_plugin caused by
>>>>> UserTracker changes should be fixed here.
>>>>>
>>>>> Filip
>>>>>
>>>>>
>>>>> On Mon, 7 Dec 2015 09:29:31 -0500 (EST)
>>>>> Aleš Mareček <amarecek at redhat.com> wrote:
>>>>>
>>>>>> NACK.
>>>>>>
>>>>>> $ ./make-lint
>>>>>> ************* Module ipatests.test_xmlrpc.test_user_plugin
>>>>>> ipatests/test_xmlrpc/test_user_plugin.py:42:
>>>>>> [E0611(no-name-in-module), ] No name 'ldaptracker' in module
>>>>>> 'ipatests.test_xmlrpc')
>>>>>>
>>>>>> $ grep ldaptracker ipatests/test_xmlrpc/test_user_plugin.py
>>>>>> from ipatests.test_xmlrpc.ldaptracker import Tracker
>>>>>> $ ls ipatests/test_xmlrpc/ldaptracker*
>>>>>> ls: cannot access ipatests/test_xmlrpc/ldaptracker*: No such file or
>>>>>> directory
>>>>>>
>>>>>>
>>>>>> ----- Original Message -----
>>>>>>> From: "Filip Škola" <fskola at redhat.com>
>>>>>>> To: "Milan Kubík" <mkubik at redhat.com>
>>>>>>> Cc: freeipa-devel at redhat.com
>>>>>>> Sent: Thursday, December 3, 2015 5:38:43 PM
>>>>>>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> sending corrected version.
>>>>>>>
>>>>>>> F.
>>>>>>>
>>>>>>> On Thu, 12 Nov 2015 14:03:19 +0100
>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>
>>>>>>>> On 11/10/2015 12:13 PM, Filip Škola wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> fixed.
>>>>>>>>>
>>>>>>>>> F.
>>>>>>>>>
>>>>>>>>> On Tue, 10 Nov 2015 10:52:45 +0100
>>>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> On 11/09/2015 04:35 PM, Filip Škola wrote:
>>>>>>>>>>> Another patch was applied in the meantime.
>>>>>>>>>>>
>>>>>>>>>>> Attaching an updated version.
>>>>>>>>>>>
>>>>>>>>>>> F.
>>>>>>>>>>>
>>>>>>>>>>> On Mon, 9 Nov 2015 13:35:02 +0100
>>>>>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 11/06/2015 11:32 AM, Filip Škola wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>> the patch doesn't apply.
>>>>>>>>>>>>
>>>>>>>>>> Please fix this.
>>>>>>>>>>
>>>>>>>>>>        ipatests/test_xmlrpc/test_user_plugin.py:1419:
>>>>>>>>>> [E0602(undefined-variable),
>>>>>>>>>> TestDeniedBindWithExpiredPrincipal.teardown_class] Undefined
>>>>>>>>>> variable 'user1')
>>>>>>>>>>
>>>>>>>>>> Also, use the version numbers for your changed patches.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>> Thanks for the patch. Several issues:
>>>>>>>>
>>>>>>>> 1. Use dict.items instead of dict.iteritems, for python3
>>>>>>>> compatibility
>>>>>>>>
>>>>>>>> 2. What is the purpose of TestPrepare class? The 'purge' methods
>>>>>>>> do not call any ipa commands.
>>>>>>>> Tracker.make_fixture should be used to make the Tracked resources
>>>>>>>> clean themselves up when they're out of scope.
>>>>>>>>
>>>>>>>> 3. Why reference the resources by hardcoded name if they have a
>>>>>>>> fixture representation?
>>>>>>>>
>>>>>>>> 4. Rewrite {create,delete}_test_group to a fixture. You may want
>>>>>>>> to use different scope (or not).
>>>>>>>>
>>>>>>>> 5. In `def atest_rename_to_invalid_login(self, user):` - use
>>>>>>>> pytest.skipif decorator and provide a reason if you must,
>>>>>>>> do not obfuscate method name in order not to run 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
>>>>>
>> --
>> 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
NACK, there are errors occuring that do not appear in the respective 
test cases in the declarative test.
In the original module the ` Test a login name that is too long` and 
`Try to rename to a username that is too long` do not use {add,set}attr. 
Why do you use them?

I'm also postponing the review of your other patches as they depend on 
changes in this one.

-- 
Milan Kubik

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160119/7516b1ad/attachment.htm>