[Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Milan Kubík]

On 02/09/2016 04:19 PM, Milan Kubík wrote:
> On 01/28/2016 10:42 AM, Filip Skola wrote:
>>
>> ----- Original Message -----
>>> On 01/25/2016 11:11 AM, Filip Skola wrote:
>>>> ----- Original Message -----
>>>>> On 01/15/2016 03:38 PM, Filip Skola wrote:
>>>>>> Hi,
>>>>>>
>>>>>> sending rebased patch.
>>>>>>
>>>>>> F.
>>>>>>
>>>>>> ----- Original Message -----
>>>>>>> Hello,
>>>>>>>
>>>>>>> sorry for delays. The patch no longer applies to master. Rebase it,
>>>>>>> please.
>>>>>>>
>>>>>>> Milan
>>>>>>>
>>>>>>> ----- Original Message -----
>>>>>>> From: "Filip Škola" <fskola at redhat.com>
>>>>>>> To: "Milan Kubík" <mkubik at redhat.com>
>>>>>>> Cc: freeipa-devel at redhat.com
>>>>>>> Sent: Wednesday, 9 December, 2015 7:01:02 PM
>>>>>>> Subject: Re: [Freeipa-devel] [PATCH 0002] Refactor 
>>>>>>> test_group_plugin
>>>>>>>
>>>>>>> On Mon, 7 Dec 2015 17:49:18 +0100
>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>
>>>>>>>> On 12/03/2015 08:15 PM, Filip Škola wrote:
>>>>>>>>> On Mon, 30 Nov 2015 17:18:30 +0100
>>>>>>>>> Milan Kubík <mkubik at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> On 11/23/2015 04:42 PM, Filip Škola wrote:
>>>>>>>>>>> Sending updated patch.
>>>>>>>>>>>
>>>>>>>>>>> F.
>>>>>>>>>>>
>>>>>>>>>>> On Mon, 23 Nov 2015 14:59:34 +0100
>>>>>>>>>>> Filip Škola <fskola at redhat.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Found couple of issues (broke some dependencies).
>>>>>>>>>>>>
>>>>>>>>>>>> NACK
>>>>>>>>>>>>
>>>>>>>>>>>> F.
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, 20 Nov 2015 13:56:36 +0100
>>>>>>>>>>>> Filip Škola <fskola at redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Another one.
>>>>>>>>>>>>>
>>>>>>>>>>>>> F.
>>>>>>>>>> Hi, the tests look good. Few remarks, though.
>>>>>>>>>>
>>>>>>>>>> 1. Please, use the shortes copyright notice in new modules.
>>>>>>>>>>
>>>>>>>>>>          #
>>>>>>>>>>          # Copyright (C) 2015  FreeIPA Contributors see 
>>>>>>>>>> COPYING for
>>>>>>>>>> license #
>>>>>>>>>>
>>>>>>>>>> 2. The tests `test_group_remove_group_from_protected_group` and
>>>>>>>>>> `test_group_full_set_of_objectclass_not_available_post_detach`
>>>>>>>>>> were not ported. Please, include them in the patch.
>>>>>>>>>>
>>>>>>>>>> Also, for less hassle, please rebase your patches on top of
>>>>>>>>>> freeipa-mkubik-0025-3-Separated-Tracker-implementations-into-standalone-pa.patch 
>>>>>>>>>>
>>>>>>>>>> Which changes the location of tracker implementations and 
>>>>>>>>>> prevents
>>>>>>>>>> circular imports.
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> these cases are there, in corresponding classes. They are marked
>>>>>>>>> with the original comments. (However I can move them to separate
>>>>>>>>> class if desirable.)
>>>>>>>>>
>>>>>>>>> The copyright notice is changed. Also included a few changes 
>>>>>>>>> in the
>>>>>>>>> test with user without private group.
>>>>>>>>>
>>>>>>>>> Filip
>>>>>>>> NACK
>>>>>>>>
>>>>>>>> linter:
>>>>>>>> ************* Module tracker.group_plugin
>>>>>>>> ipatests/test_xmlrpc/tracker/group_plugin.py:257:
>>>>>>>> [E0102(function-redefined), GroupTracker.check_remove_member] 
>>>>>>>> method
>>>>>>>> already defined line 253)
>>>>>>>>
>>>>>>>> Probably a leftover after the rebase made on top of my patch. 
>>>>>>>> Please
>>>>>>>> fix it. You can check youch changes by make-lint script before
>>>>>>>> sending them.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I learned to use make-lint!
>>>>>>>
>>>>>>> Thanks,
>>>>>>> F.
>>>>>>>
>>>>> Hello,
>>>>>
>>>>> NACK, pylint doesn't seem to like the way the fixtures are imported
>>>>> (pytest does a lot of runtime magic) [1].
>>>>> One possible solution would be [2]. Though, I don't think this 
>>>>> would be
>>>>> a good idea in our environment. I suggest to create the fixtures 
>>>>> on per
>>>>> module basis.
>>>>>
>>>>>
>>>>> [1]: http://fpaste.org/311949/53118942/
>>>>> [2]:
>>>>> https://pytest.org/latest/fixture.html#using-fixtures-from-classes-modules-or-projects 
>>>>>
>>>>>
>>>>> -- 
>>>>> Milan Kubik
>>>>>
>>>>>
>>>> Hi,
>>>>
>>>> the fixtures were copied into corresponding module. Please note 
>>>> that this
>>>> patch has a dependence on my patch 0001 (user plugin).
>>>>
>>>> Filip
>>> Linter:
>>> ************* Module ipatests.test_xmlrpc.tracker.group_plugin
>>> W:100,26: Calling a dict.iter*() method (dict-iter-method)
>>>
>>> please use dict.items
>>>
>>> -- 
>>> Milan Kubik
>>>
>>>
>> Hi, sorry. This has been fixed in this patch.
>>
>> Filip
> ACK, thanks for the patience. :)
>
Sorry, there are some other things I need clarified. NACK.
Mail will follow later.

-- 
Milan Kubik