[Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin

Martin Basti mbasti at redhat.com
Mon Jan 25 15:06:00 UTC 2016 


On 25.01.2016 15:12, Aleš Mareček wrote:
> Tested + several other dependent tests executed as well - PASS.
> The patch looks good, ACK.
>
> ----- Original Message -----
>> From: "Filip Skola" <fskola at redhat.com>
>> To: "Milan Kubík" <mkubik at redhat.com>
>> Cc: freeipa-devel at redhat.com, "Aleš Mareček" <amarecek at redhat.com>
>> Sent: Monday, January 25, 2016 11:55:35 AM
>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
>>
>>
>>
>> ----- Original Message -----
>>> 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
>>>
>>>
>> To be honest, I have no idea why I did use setattr in those places. Update of
>> 'rename' attribute was used instead in this version of the patch and that
>> seems to work.
>>
>> Filip
Pushed to:
ipa-4-3: a278a74695ef9df381d2668ce6d249ced555aad2
master: 3d1adb325512cf14de8b88ca8178dd4a754256ec

More information about the Freeipa-devel mailing list